Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offload and modules with unused submodules #442

Merged
merged 4 commits into from
Jun 18, 2022
Merged

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Jun 13, 2022

This PR deals with models with unused submodules combined with offload (on CPU and disk). By unused submodules, we mean a something like the ModuleWithUnusedSubModules introduced in the test file in this PR: a module that defines a submodule, but does not call it during its forward, just does some operations using its weights.

Currently, the offload will fail for such modules because the weights are fetched in a pre-forward hook and the forward method is never called. This PR fixes that by introducing a new argument named load_all_weights_classes (if you have any better suggestion, please let me know as I'm not super happy with this name) which contains the name of such submodules used (like no_split_module_classes contains the names of the modules that shouldn't be split across devices).

@sgugger sgugger requested review from LysandreJik and muellerzr June 13, 2022 19:56
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 13, 2022

The documentation is not available anymore as the PR was closed or merged.

Copy link
Collaborator

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thanks! The fix makes sense to me, left one naming suggestion/alternative

src/accelerate/hooks.py Outdated Show resolved Hide resolved
@sgugger sgugger requested a review from pacman100 June 14, 2022 14:27
Copy link
Contributor

@pacman100 pacman100 left a comment

Choose a reason for hiding this comment

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

In attach_align_device_hook_on_block, I think below change is needed

if not isinstance(execution_device, Mapping):
-        execution_device = {key: offload for key in offload.keys()}
+        execution_device = {key: execution_device for key in offload.keys()}

Left a minor suggestion too.
Apart from that everything LGTM!

src/accelerate/hooks.py Outdated Show resolved Hide resolved
@sgugger sgugger merged commit eeaba59 into main Jun 18, 2022
@sgugger sgugger deleted the cpu_offload_unused_subs branch June 18, 2022 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants