Skip to content

Conversation

@zhangvia
Copy link

@zhangvia zhangvia commented Jul 1, 2024

What does this PR do?

this pr is talking about when the is_sequential_cpu_offload should be set to True.

before we got device_map feature for pipline, if any component in pipeline has AlignDevicesHook which is used to move input data to the model device, we will set the is_sequential_cpu_offload = True . but when using device_map, we will also add AlignDevicesHook to model.

and besides, if someone want to add AignDevicesHook to model manually, is_sequential_cpu_offload will also be set to True.
that would trigger a bug in load_lora_weights() method

so maybe we should set is_sequential_cpu_offload=True when some component is on cpu and has AlignDevicesHook simultaneously

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@sayakpaul
Copy link
Member

Let us know when this PR is ready for a review.

@zhangvia
Copy link
Author

zhangvia commented Jul 2, 2024

alright, i will modify all stuff that is correspoding to is_sequential_cpu_offload

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@zhangvia zhangvia force-pushed the modify_is_sequential_off_load branch from 5ca5291 to eebed22 Compare July 4, 2024 01:55
@zhangvia zhangvia marked this pull request as ready for review July 4, 2024 01:58
@zhangvia
Copy link
Author

zhangvia commented Jul 4, 2024

i have check all stuff is correspoding to is_sequential_cpu_offload, please take a look @sayakpaul ,thanks!

@sayakpaul sayakpaul requested a review from yiyixuxu July 4, 2024 02:02
@zhangvia
Copy link
Author

a gentle ping here @sayakpaul @yiyixuxu

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Sep 14, 2024
@a-r-r-o-w a-r-r-o-w removed the stale Issues that haven't received updates label Nov 20, 2024
@a-r-r-o-w a-r-r-o-w requested a review from sayakpaul November 20, 2024 00:20
@a-r-r-o-w
Copy link
Contributor

Hi, sorry for the delay here. I've asked Sayak for a review on this

@sayakpaul
Copy link
Member

Hi,

Thanks for your PR. Could you demonstrate your use-case with some minimal code for us to understand this better?

@sayakpaul sayakpaul added the needs-code-example Waiting for relevant code example to be provided label Nov 20, 2024
Comment on lines +381 to +385
if is_model_cpu_offload or is_sequential_cpu_offload:
logger.info(
"Pipeline offload enabled and Accelerate hooks detected. Since you have called `load_lora_weights()`, the previous offload hooks will be first removed. Then the LoRA parameters will be loaded and the hooks will be applied again."
)
remove_hook_from_module(component, recurse=is_sequential_cpu_offload)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks alright to me.

return False

return hasattr(module, "_hf_hook") and (
return hasattr(module, "_hf_hook") and hasattr(module,'device') and module.device.type == "cpu" and (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the expansion?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manually add a hook to the model and attempting to move it to another GPU does not mean that sequential_cpu_offload is enabled. but the original code will still return true

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions bot added the stale Issues that haven't received updates label Dec 14, 2024
@zhangvia
Copy link
Author

zhangvia commented Mar 11, 2025

sorry for delay.

Hi,

Thanks for your PR. Could you demonstrate your use-case with some minimal code for us to understand this better?

there are two cases that might get error with the current verison of diffusers:

when you want to manually add align_device_hook.
(1) the present device_map feature is not granular enough to achieve precise memory control. sometimes it's better to decide which model should be on which gpu by user instead of "balanced" or "auto".
(2) or when you want to implement some custom offloading strategy like block swap on some model in the pipeline

@github-actions github-actions bot removed the stale Issues that haven't received updates label Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-code-example Waiting for relevant code example to be provided

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants