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 cache_position initialisation for generation with use_cache=False #30485

Merged
merged 3 commits into from May 7, 2024

Conversation

nurlanov-zh
Copy link
Contributor

What does this PR do?

Fixes #30482

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 @younesbelkada @gante

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! Let's maybe avoid relying on the model's cache position initialization !

src/transformers/generation/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@gante gante left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the fix 👍

@ArthurZucker tbh, when we have use_cache=False, we shouldn't even be creating or using cache_positions at all (as the name of the variable indicates, its purpose is cache-related). ATM, the only dependency is the causal mask creation function, which uses cache_positions as a proxy for the sequence length (i.e. we can easily rearrange the code to avoid using it when there is no cache :) )

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.

Yes @gante, agreed would split both paths a bit better

@ArthurZucker
Copy link
Collaborator

Can you just rebase on main? 🤗

@HuggingFaceDocBuilderDev

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.

@nurlanov-zh
Copy link
Contributor Author

Can you just rebase on main? 🤗

@ArthurZucker , done ✔️

@ArthurZucker ArthurZucker merged commit 4fda78c into huggingface:main May 7, 2024
20 checks passed
@ArthurZucker
Copy link
Collaborator

Thanks 🤗

zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request May 10, 2024
…lse` (huggingface#30485)

* Fix cache_position init for generation

* Update src/transformers/generation/utils.py

Co-authored-by: Arthur <[email protected]>

* Fix cache position update

---------

Co-authored-by: Arthur <[email protected]>
itazap pushed a commit that referenced this pull request May 14, 2024
…lse` (#30485)

* Fix cache_position init for generation

* Update src/transformers/generation/utils.py

Co-authored-by: Arthur <[email protected]>

* Fix cache position update

---------

Co-authored-by: Arthur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants