[Generation] Fix default overwrite for non-None defaults #42958
Conversation
| run-slow: gpt2,whisper,qwen2_5_vl,glm4v,biogpt |
| This comment contains models: ["models/biogpt", "models/glm4v", "models/gpt2", "models/qwen2_5_vl", "models/whisper"] |
ArthurZucker left a comment
ArthurZucker left a comment There was a problem hiding this comment.
We need a small fast test 🫡
| 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. |
src/transformers/generation/utils.py Outdated
| | ||
| # Due to some values being boolean and not `None`, we need additional logic to overwrite | ||
| # them explicitly (`defaults_only=False`) | ||
| generation_config.update(**{k: v for k, v in self.generation_config.to_dict().items() if isinstance(v, bool)}) |
There was a problem hiding this comment.
so we don't need to restrict that update? to something like
default_gc = GenerationConfig() bool_defaults = {k: v for k, v in self.generation_config.to_dict().items() if isinstance(v, bool) and hasattr(generation_config, k) and hasattr(default_gc, k) and getattr(generation_config, k) == getattr(default_gc, k) } generation_config.update(**bool_defaults)just to be sure I understand 😁
There was a problem hiding this comment.
Yup, it's correct I didnt notice that we can pass a generation config directly there as well (e.g. as in assisted decoding)
| run-slow: whisper,gemma3n,gpt2,moshi,bert |
CI Results |
| 💔 This comment contains |
| run-slow: whisper,gemma3n,gpt2,moshi,bert |
| This comment contains models: ["models/bert", "models/gemma3n", "models/gpt2", "models/moshi", "models/whisper"] |
CI Results✅ No failing test specific to this PR 🎉 ! |
| View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=42958&sha=202fd1 |
| generation_config.update( | ||
| **{ | ||
| k: v | ||
| for k, v in self.generation_config.to_dict().items() | ||
| if isinstance(v, bool) | ||
| and hasattr(default_generation_config, k) | ||
| and getattr(generation_config, k, None) == getattr(default_generation_config, k) | ||
| } | ||
| ) |
There was a problem hiding this comment.
sorry for late review. I think we're back to the original issue where users pass do_sample=False in their custom config and the model has do_sample=True saved on the hub. And since False is the default value, we won't update it here and keep True
In that case we'd need to set the booleans to None by default as well, I dont' see a cleaner way than defining None as non-set
5 participants
Followup to #42702 where defaults are now either
Noneor a boolean value.The logic was dependent on the values being
Nonewhich ignored overriding other values, e.g.do_sample. The fails due to this can be seen in a few integration tests, e.g. gpt2.