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 config + attn_implementation in AutoModelForCausalLM.from_pretrained #30299

Merged
merged 5 commits into from
Apr 19, 2024
Merged

Fix config + attn_implementation in AutoModelForCausalLM.from_pretrained #30299

merged 5 commits into from
Apr 19, 2024
+39 −1

Conversation

hiyouga
Copy link
Contributor

@hiyouga hiyouga commented Apr 17, 2024

What does this PR do?

Fixes #30298

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?

@ArthurZucker and @younesbelkada

Sorry, something went wrong.

@hiyouga
Update modeling_utils.py

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
31bd865
fxmarty
fxmarty approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@fxmarty fxmarty 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.

This looks very reasonable, thank you for the fix. Ideally, could you add a test for this? For example, in tests/test_modeling_utils.py's TestAttentionImplementation.

@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.

@fxmarty fxmarty requested a review from amyeroberts April 17, 2024 21:13
@hiyouga
Copy link
Contributor Author

hiyouga commented Apr 18, 2024

This looks very reasonable, thank you for the fix. Ideally, could you add a test for this? For example, in tests/test_modeling_utils.py's TestAttentionImplementation.

Ok I'll do it

@hiyouga
Copy link
Contributor Author

hiyouga commented Apr 18, 2024

@fxmarty I have done, could you review it again?

amyeroberts
amyeroberts reviewed Apr 18, 2024
View reviewed changes
Copy link
Collaborator

@amyeroberts amyeroberts 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 working on this and fixing!

Just a question about why this works

helloworld1
helloworld1 approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@helloworld1 helloworld1 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 fixing this. I just encountered the same bug.

@hiyouga
Update test_modeling_utils.py
Loading
Loading status checks…
c474454
fxmarty
fxmarty approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@fxmarty fxmarty left a comment

Choose a reason for hiding this comment

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

LGTM

@fxmarty fxmarty requested a review from amyeroberts April 19, 2024 15:35
@fxmarty
Copy link
Contributor

fxmarty commented Apr 19, 2024

@hiyouga something is wrong with the CI could you try merging main?

amyeroberts
amyeroberts approved these changes Apr 19, 2024
View reviewed changes
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks!

@hiyouga
Copy link
Contributor Author

hiyouga commented Apr 19, 2024

@hiyouga something is wrong with the CI could you try merging main?

Done

@amyeroberts amyeroberts merged commit 21c912e into huggingface:main Apr 19, 2024
20 checks passed
ydshieh pushed a commit that referenced this pull request Apr 23, 2024
@hiyouga @ydshieh
Fix config + attn_implementation in AutoModelForCausalLM.from_pretrai…
691d2c6 
…ned (#30299)

* Update modeling_utils.py

* Update test_modeling_utils.py

* Update test_modeling_utils.py

* Update test_modeling_utils.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@helloworld1 helloworld1

@fxmarty fxmarty

@amyeroberts amyeroberts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Attention implementation cannot work together with config in AutoModel
5 participants
@hiyouga @HuggingFaceDocBuilderDev @fxmarty @helloworld1 @amyeroberts