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

Handle multiple tied parameters #1241

Merged
merged 5 commits into from
Mar 24, 2023
Merged

Handle multiple tied parameters #1241

merged 5 commits into from
Mar 24, 2023

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Mar 23, 2023

This PR reworks the internal of big model inference to handle multiple tied parameters (instead of just a pair). It contains a breaking change in the util function find_tied_parameters (which is used once in Transformers, so we will need an update there). This is because its results as a dictionary doesn't really make sense when we can have groups of three/four tied parameters all together.

The logic in infer_auto_device_map is getting a lot more complicated, so I added a verbose argument to leave plenty of print statements and help future me or others debug what's happening. Tests and integration tests are added as well.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 23, 2023

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! This all looks good to me. Test failure is because test_infer_auto_device_map_on_t0pp(self) needs to use the @require_huggingface_suite decorator I believe.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Cool! Should there be 🚨 at the top to mention the breaking change?

src/accelerate/utils/modeling.py Outdated Show resolved Hide resolved
current_memory_used = 0
else:
# Otherwise, we replace the tied module by its children.
elif len(tied_params) > 0:
Copy link
Member

Choose a reason for hiding this comment

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

Scary :)

@sgugger sgugger merged commit a826e44 into main Mar 24, 2023
@sgugger sgugger deleted the multiple_tied_params branch March 24, 2023 13:53
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