-
Notifications
You must be signed in to change notification settings - Fork 31.7k
[Generation, Gemma 3] When passing a custom generation_config, overwrite default values with the model's base generation_config
#36684
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
Merged
gante
merged 16 commits into
huggingface:main
from
gante:inherit_values_from_base_generation_config
Mar 15, 2025
Merged
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
7221e23
use base gen config
gante 3bf123f
trigger tests
gante 3befb4a
fix whisper: custom processors have priority
gante 8e004b7
Merge branch 'main' into inherit_values_from_base_generation_config
gante 900b273
Merge branch 'main' into inherit_values_from_base_generation_config
gante 5ac5869
Merge branch 'main' into inherit_values_from_base_generation_config
gante e105a63
add missing pytest.mark.generate decorator
gante 1866a47
make fixup
gante c075e66
correct mark after paremeterization
gante e7a2fa3
we can only safely overwrite None defaults
gante d56f073
apply kwargs afterwards
gante 9034227
add gemma 3 test
gante e6b67ef
nit
gante 0b67b12
throw warning on overwrite; add 'dynamic' cache_implementation
gante b66b0dc
Merge branch 'main' into inherit_values_from_base_generation_config
gante a636af0
better msg
gante File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this function are secondary to the main change:
.generate()(i.e. assumes the user knows what they are doing when the passlogits_processorsto.generate()instead of crashing)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tiny comment wouldn't hurt for future us, isn't very easy to get why we do this without reading PR description.
Also I am not sure if this is required, aren't we restricting custom logits processors to only those that cannot be configured by generation config? Something that is only defined by users for their use-case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user always had the option of unsetting a flag and passing the corresponding processor. This change makes it less restricting: if they pass both a flag and the corresponding processor, we keep the processor and ignore the flag (previously we would throw an exception)
I'm not super fan of the new behavior, I think the exception less ambiguous (and thus preferable). However, it's needed to coexist with the main change in this PR, which is (IMO) more important.
I'll add a comment to clarify the evolution of this function, in case we need to trace back a related change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, same feeling here. I would even restrict it to only custom logits processors in v5 to free us from the burden of maintaining correct priority/ordering etc. Looks like pandora's box what is being fixed 😿
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely don't want this,
generatewould become very uncomfortable to use 😅 A user always has the option of disablinggeneration_configflags and passing the processors in the order they want. But most users don't want that level of control, and yet may want an external processorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason
transformersis so popular is because we enable complex use-cases from a few lines of codeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that this gives users more freedom to order processors the way they want by disabling generation config. Though I feel like it is not very clear to me as a user what happens under the hood, when I pass my own Temperature processor or use config. IMO we need a better docs page for advanced usage, if we allow that much freedom and expect users to know what they are doing
Users almost never 100% know what they are doing, thus open issues on GH 😆