Drop noisy generate warnings when do_sample=False (or num_beams=1)#45559
Closed
ArthurZucker wants to merge 1 commit intohuggingface:mainfrom
Closed
Drop noisy generate warnings when do_sample=False (or num_beams=1)#45559ArthurZucker wants to merge 1 commit intohuggingface:mainfrom
ArthurZucker wants to merge 1 commit 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. |
Member
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
in general i agree, users don't want to see warnings if they just turn off sampling iwth one flag. Though I am not sure about the reverse case, when they pass sampling params and the default config has do_sample=False. Kinda misleading if we silently fallback to greedy generation, no?
Comment on lines
-639
to
+660
| if self.do_sample is not True: | ||
| # | ||
| # The warning is suppressed for flags that weren't explicitly set by the caller (see `_is_user_set`): values | ||
| # inherited from a model's `generation_config.json` are harmless when the user opts into greedy decoding. | ||
| # We also require `do_sample` itself to be user-set -- otherwise the non-sampling mode was inherited and the | ||
| # user never expressed intent to skip sampling, so flagging their sampling kwargs would be misleading. | ||
| if self.do_sample is not True and _is_user_set("do_sample"): | ||
| greedy_wrong_parameter_msg = ( | ||
| "`do_sample` is set not to set `True`. However, `{flag_name}` is set to `{flag_value}` -- this flag is only " | ||
| "used in sample-based generation modes. You should set `do_sample=True` or unset `{flag_name}`." | ||
| "`do_sample` is set to `{do_sample}`. However, `{flag_name}` is set to `{flag_value}` -- this flag is " | ||
| "only used in sample-based generation modes. You should set `do_sample=True` or unset `{flag_name}`." |
Member
There was a problem hiding this comment.
great opportunity to move to hub-dataclass validation 😄
Comment on lines
+182
to
+188
| # Inverse provenance case: `do_sample` inherited from a model's config (so not user-set this call), user only | ||
| # sets a sampling flag. The conflict shouldn't produce noise because the user never asked for greedy. | ||
| logger.warning_once.cache_clear() | ||
| greedy_hub_config = GenerationConfig(do_sample=False) # mimics a model's default config forcing greedy | ||
| with CaptureLogger(logger) as captured_logs: | ||
| greedy_hub_config.update(top_p=0.8) | ||
| self.assertEqual(len(captured_logs.out), 0) |
Member
There was a problem hiding this comment.
i am not sure about this one. Beginner users might expect this to just work and sample with top-p, while we silently fallback to greedy
Comment on lines
711
to
723
| # 2.4. check `num_return_sequences` | ||
| if self.num_return_sequences is not None and self.num_return_sequences > 1: | ||
| if self.num_beams is None or self.num_beams == 1: | ||
| if not self.do_sample: | ||
| raise ValueError( | ||
| "Greedy methods (do_sample != True) without beam search do not support " | ||
| f"`num_return_sequences` different than 1 (got {self.num_return_sequences})." | ||
| ) | ||
| elif ( | ||
| self.num_beams is not None | ||
| and self.num_return_sequences is not None | ||
| and self.num_return_sequences > self.num_beams | ||
| ): |
Member
There was a problem hiding this comment.
so the is-user check is only when greedy decoding, no changes on other params?
Cyrilvallez
force-pushed
the
drop-sampling-flag-warnings
branch
from
0579f79 to
a691ca5
Compare
Member
|
Completely messed up this branch by mistake 😅 Opened #45619 with the correct diffs |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Summary
GenerationConfig.validate()was warning about sampling-only flags (temperature,top_p,top_k,min_p,top_h,typical_p,epsilon_cutoff,eta_cutoff) wheneverdo_samplewas notTrue, and about beam-only flags (early_stopping,length_penalty) whenevernum_beams == 1-- even when those values were inherited from the model'sgeneration_config.json. In practice, nearly every popular Hub model ships with a non-defaulttemperature/top_p, so users got a warning for everygenerate(do_sample=False)call.This PR threads a
user_set_attributesset throughGenerationConfig.__init__andupdate(), sovalidate()can distinguish attributes the caller explicitly provided from values inherited from the model's default config. The sampling-only and beam-only warnings now only fire for user-set attributes.Behavior
generate(do_sample=False)on a model whose hub config hastemperature=0.6, top_p=0.9: silent (values inherited, not user intent).generate(do_sample=False, top_p=0.8): warns abouttop_p(user explicitly set both -- almost certainly a mistake).GenerationConfig(do_sample=False, temperature=0.5): warns (both explicit).generation_config.validate(strict=True)(e.g. fromsave_pretrained) with nouser_set_attributespreserves the original "refuse to save bad configs" behavior.cc @Cyrilvallez