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

generation utils update (minor) #1468

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

yafshar
Copy link
Contributor

@yafshar yafshar commented Nov 1, 2024

What does this PR do?

  • Fix import path for streamers module from transformers.streamers -> transformers.generation.streamers
  • Fix the _prepare_decoder_attention_mask interface interface
    • return x.index_fill(1, torch.tensor(0), 1) uses the wrong index of torch.tensor(0), it is fixed to the correct index on the correct device index = torch.tensor(0, device=device)
  • Improve the _pad_past_key_values function

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

- Fix the type hint, dtype can not be a str
- Fix the device hint
- Remove the pad token id arg, the decoder_attention_mask is a binary of
  0, and 1
- Added an early return
- Extracted is_mqa_model and lazy_mode to avoid repeated dictionary
  lookups
- Used more descriptive variable names and simplified the nested
  loops for better readability
@yafshar yafshar marked this pull request as ready for review November 4, 2024 17:36
@yafshar yafshar changed the title generation utils update generation utils update (minor) Nov 5, 2024
@yafshar
Copy link
Contributor Author

yafshar commented Nov 8, 2024

The text-generation CI has been executed and will be compared with the main branch once the run is complete.

Copy link
Contributor

@emascarenhas emascarenhas left a comment

Choose a reason for hiding this comment

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

@yafshar , Just a couple of comments below.
Please post results of CI, before and after change.

@emascarenhas
Copy link
Contributor

@yafshar , Makes sense.
Please post the CI results when available and we can move it further along.

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.

2 participants