Generate: unset GenerationConfig parameters do not raise warning#29119
Generate: unset GenerationConfig parameters do not raise warning#29119gante merged 4 commits intohuggingface:mainfrom
Conversation
|
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. |
fxmarty
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot for the fix
amyeroberts
left a comment
There was a problem hiding this comment.
Thanks for adding! Just a question about the removal of some validation, as the checks on none look like they should be enough to resolve
|
|
||
| generation_config = copy.deepcopy(generation_config) | ||
| model_kwargs = generation_config.update(**kwargs) # All unused kwargs must be model kwargs | ||
| generation_config.validate() |
There was a problem hiding this comment.
Why get rid of these validations here (and in the other utils files)?
There was a problem hiding this comment.
Because I've added the .validate() call to .update(), which is the line that precedes all these removals :) It makes more sense to validate at update time, rather than relying on the user to manually validate.
| # parameters with `do_sample=False`). May be escalated to an error in the future. | ||
| with warnings.catch_warnings(record=True) as captured_warnings: | ||
| GenerationConfig(temperature=0.5) | ||
| GenerationConfig(do_sample=False, temperature=0.5) |
There was a problem hiding this comment.
This commit I've made a few implicit parameters explicit, to make reading the test easier :)
What does this PR do?:
Thank you @fxmarty for raising this issue.
This PR allows users to unset (= set to
None) unused parameters to ensuregeneration_config.validate()doesn't throw a warning. Previously, this was not possible when a parameter had a non-Nonedefault.For instance, the following snippet would throw a warning before this PR: