-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Fix CPU offload + disk offload tests #27204
Conversation
The documentation is not available anymore as the PR was closed or merged. |
e7651c9
to
fed5e54
Compare
@amyeroberts @patrickvonplaten if you feel uneasy with merging this right before the release, I'm fine with reverting the safetensors serialization by default to let it sit on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding the fix so quickly!
@LysandreJik The change LGTM and seems to address some underlying issues. Re default safetensors serialization, I'm happy for it to be part of this release as long as some of the slow tests on the most popular models (bert, llama, wav2vec2, whisper, clip etc.) are good. |
@@ -1125,6 +1125,11 @@ def __init__(self, config: BartConfig): | |||
# Initialize weights and apply final processing | |||
self.post_init() | |||
|
|||
def _tie_weights(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice, noticed this as well actually 😅
https://github.com/huggingface/transformers/pull/27203/files#r1378948159
@@ -4520,7 +4520,9 @@ def expand_device_map(device_map, param_names): | |||
""" | |||
new_device_map = {} | |||
for module, device in device_map.items(): | |||
new_device_map.update({p: device for p in param_names if p == module or p.startswith(f"{module}.")}) | |||
new_device_map.update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean!
Thanks both for your reviews! I'll go ahead and merge this, sorry but you'll have the conflict Patrick 😁 |
Fix disk offload tests + weight sharing issues
Passing to safetensors serialization by default highlighted a few issues that we have with safetensors.
This PR fixes the issue, which is principally linked to weight sharing.