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.

Sign up for GitHub

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

Jump to bottom

Fix: take into account meta device #34134

Merged
merged 5 commits into from
Nov 20, 2024
Merged

Fix: take into account meta device #34134

merged 5 commits into from
Nov 20, 2024

Conversation

tibor-reiss
Copy link
Contributor

@tibor-reiss tibor-reiss commented Oct 13, 2024 •
edited
Loading

Fixes #34091

The models given in the issues' example have different torch_dtype which results in a different handling in check_support_param_buffer_assignment.
fxmarty/small-llama-testing: torch_dtype=float32
fxmarty/tiny-llama-fast-tokenizer: torch_dtype=float16

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@LysandreJik @fxmarty @muellerzr

@LysandreJik
Copy link
Member

cc @SunMarc for initial review

SunMarc
SunMarc reviewed Oct 15, 2024
View reviewed changes
Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @tibor-reiss! Can you also add a test so that this doesn't happen in the future ?

@HuggingFaceDocBuilderDev
Copy link

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.

@tibor-reiss
Copy link
Contributor Author

Thanks for adding this @tibor-reiss! Can you also add a test so that this doesn't happen in the future ?

@SunMarc I was not sure where to add the test, so please feel free to move it around. Additionally, the TINY_* models are float32, so I went with the models from the issues' example (they are quite small) - let me know if there is a better way.

SunMarc
SunMarc approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member

@SunMarc SunMarc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for iterating ! Left a few suggestions. Does this solve your issue @fxmarty-amd =) ?

tests/utils/test_modeling_utils.py Outdated Show resolved Hide resolved
tests/utils/test_modeling_utils.py Outdated Show resolved Hide resolved
@SunMarc SunMarc requested a review from muellerzr October 16, 2024 10:28
@tibor-reiss
Copy link
Contributor Author

Friendly ping @fxmarty-amd

@SunMarc SunMarc requested a review from ArthurZucker November 4, 2024 17:20
ArthurZucker
ArthurZucker approved these changes Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Looks good! Sorry for being so late on my review and thanks for the contribution! 🤗

@ArthurZucker ArthurZucker merged commit f297af5 into huggingface:main Nov 20, 2024
24 of 26 checks passed
@tibor-reiss tibor-reiss deleted the fix-34091-meta-device branch November 21, 2024 19:09
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
@tibor-reiss @SunMarc @BernardZach
Fix: take into account meta device (huggingface#34134)
9ec16a6 
* Do not load for meta device

* Make some minor improvements

* Add test

* Update tests/utils/test_modeling_utils.py

Update test parameters

Co-authored-by: Marc Sun <[email protected]>

* Make the test simpler

---------

Co-authored-by: Marc Sun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SunMarc SunMarc SunMarc approved these changes

@ArthurZucker ArthurZucker ArthurZucker approved these changes

@muellerzr muellerzr Awaiting requested review from muellerzr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@tibor-reiss @LysandreJik @HuggingFaceDocBuilderDev @ArthurZucker @SunMarc