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

[FIX] resize_token_embeddings #26102

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

passaglia
Copy link
Contributor

@passaglia passaglia commented Sep 12, 2023

What does this PR do?

When resize_token_embeddings(new_num_tokens, pad_to_multiple) is called with new_num_tokens a multiple of pad_to_multiple, the model should be resized to new_num_tokens. Due to a math error, it is instead resized to new_num_tokens+pad_to_multiple. This PR fixes that bug.

@ArthurZucker #25088

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

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.

Thanks for the catch! Could you add a test? 😉

@passaglia
Copy link
Contributor Author

I'm not so familiar with the transformers repo -- where should a test for this code go?

@ArthurZucker
Copy link
Collaborator

SHould be part of this tests

@passaglia
Copy link
Contributor Author

@ArthurZucker please confirm this test works, I couldn't run it myself since pytest tests/test_modeling_common.py returned 0 collected tests.

@ArthurZucker
Copy link
Collaborator

Sure, I'll test this, can you run make style as well? To have green cis

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.

For simplicity let's just use 168 as the target 😉 Tests are green locally
Make sure to run make style and we can merge this!

tests/test_modeling_common.py Outdated Show resolved Hide resolved
passaglia and others added 2 commits September 19, 2023 17:34
Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
@passaglia
Copy link
Contributor Author

@ArthurZucker Ready for merge

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.

Good job! 🚀

@ArthurZucker ArthurZucker merged commit 8e3980a into huggingface:main Sep 19, 2023
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
* fix roundup command

* add test for resize_token_embeddings

* Update tests/test_modeling_common.py

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>

* style

---------

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
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.

3 participants