Fix ExponentialDecayLengthPenalty negative logits issue #25594
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ArthurZucker
approved these changes
Aug 21, 2023
|
Thanks @gante and @ArthurZucker ! Could you please approve the documentation workflow? I’d like to verify the documentation looks as expected |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
|
@gante Do I need to do anything more to get this merged? |
|
You could take into account the changes I suggested 😉 code related variable name are always put aroung codeblocks in our doc! |
|
@ArthurZucker oops, totally missed those! Committed those fixes now, Thanks! |
ArthurZucker
approved these changes
Sep 4, 2023
Fix tests which were broken and add validation of negative scores. Current test didn't take into account that ExponentialDecayLengthPenalty updates the score inplace, resulting in updates to base tested Tensor. In addition, the gt assert had empty Tensors due to indexing along the batch dimension. Test is currently expected to fail to show ExponentialDecayLengthPenalty issues with negative scores
In cases where the scores are negative, ExponentialDecayLengthPenalty decreases the score of eos_token_id instead of increasing it. To fix this issue we compute the penalty of the absolute value and add it to the original score.
Co-authored-by: Arthur <[email protected]>
pokjay
force-pushed
the
fix-exp-decay-penalty-neg-logits
branch
from
188fb67 to
e27c496
Compare
|
@pokjay since we last spoke, we've added this file to the list of files to be doctested in our PR CI -- it seems like your example's outputs don't match the hardcoded outputs. Would you be able to double-check that? :) (as soon as this gets fixed, we can merge) |
|
@gante Fixed the issues, all checks pass! |
|
@pokjay thank you for iterating 💛 |
parambharat
pushed a commit
to parambharat/transformers
that referenced
this pull request
Sep 26, 2023
…25594) * Fix issues in test_exponential_decay_length_penalty Fix tests which were broken and add validation of negative scores. Current test didn't take into account that ExponentialDecayLengthPenalty updates the score inplace, resulting in updates to base tested Tensor. In addition, the gt assert had empty Tensors due to indexing along the batch dimension. Test is currently expected to fail to show ExponentialDecayLengthPenalty issues with negative scores * Fix ExponentialDecayLengthPenalty negative logits issue In cases where the scores are negative, ExponentialDecayLengthPenalty decreases the score of eos_token_id instead of increasing it. To fix this issue we compute the penalty of the absolute value and add it to the original score. * Add examples for ExponentialDecayLengthPenalty * Fix styling issue in ExponentialDecayLengthPenalty doc * Apply suggestions from code review Co-authored-by: Arthur <[email protected]> * Style and quality fix * Fix example outputs --------- Co-authored-by: Arthur <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
In cases where the model logits are negative, ExponentialDecayLengthPenalty decreases the score of eos_token_id instead of increasing it.
To fix this issue we compute the penalty of the absolute value and add it to the original score, as described in #25416
The test was updated to check for negative logits.
In addition, this PR updates the class documentation and adds examples, as part of #24783
Fixes #25416
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@gante