Fix MusicGen generation config#43194
Conversation
| generation_config = copy.deepcopy(generation_config) | ||
| model_kwargs = generation_config.update(**kwargs) # All unused kwargs must be model kwargs | ||
| generation_config.validate() | ||
| generation_config = self.generation_config if generation_config is None else generation_config |
There was a problem hiding this comment.
this line is used in all overriden generate() to use custom gen-params. Need to clean up after your PR on custom generation parameters
There was a problem hiding this comment.
Do we want to prio #43181 first then, I changed the logic to be within update directly (via a new kwarg), lmk what you think otherwise fine to fix this first
There was a problem hiding this comment.
yeah, I don't mind waiting for your PR to be merged first
|
run-slow: musicgen_melody |
|
This comment contains models: ["models/musicgen_melody"] |
|
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. |
CI Results✅ No failing test specific to this PR 🎉 ! |
vasqu
left a comment
vasqu
left a comment
There was a problem hiding this comment.
Fine to fix this first before we forget, test_generate_text_audio_prompt still fails not sure if it failed before as well
| logger.warning_once( | ||
| f"Passing `generation_config` together with generation-related " | ||
| f"arguments=({generation_kwargs}) is deprecated and will be removed in future versions. " | ||
| "Please pass either a `generation_config` object OR all generation " | ||
| "parameters explicitly, but not both.", | ||
| ) |
There was a problem hiding this comment.
I would love to have this raise an error but it's probably too breaking?
There was a problem hiding this comment.
yeah, we don't want to break it yet. This is to nudge users for better practices and give them time to get used to it. Maybe we can raise errors after 10-15 releases
| generation_config = copy.deepcopy(generation_config) | ||
| model_kwargs = generation_config.update(**kwargs) # All unused kwargs must be model kwargs | ||
| generation_config.validate() | ||
| generation_config = self.generation_config if generation_config is None else generation_config |
There was a problem hiding this comment.
Do we want to prio #43181 first then, I changed the logic to be within update directly (via a new kwarg), lmk what you think otherwise fine to fix this first
|
CI is going crazy with the flakiness 😢 |
|
CI is sooo flaky |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: musicgen_melody, whisper |
Commit 38e5987 from PR huggingface#43194 introduced a minor bug that causes the issuing of a deprecation warning regardless of the user using the deprecated interface. For example: ``` model.generate(**input_ids, do_sample=False) ``` would issue a deprecation warning not to pass `GenerationConfig` as well as generation kwargs even though we're not passing a `GenerationConfig`. Along the fix this also introduces a unit test to check both the intended and the bug behavior.
Commit 38e5987 from PR huggingface#43194 introduced a minor bug that causes the issuing of a deprecation warning regardless of the user using the deprecated interface. For example: ``` model.generate(**input_ids, do_sample=False) ``` would issue a deprecation warning not to pass `GenerationConfig` as well as generation kwargs even though we're not passing a `GenerationConfig`. Along the fix this also introduces a unit test to check both the intended and the bug behavior.
Commit 38e5987 from PR huggingface#43194 introduced a minor bug that causes the issuing of a deprecation warning regardless of the user using the deprecated interface. For example: ``` model.generate(**input_ids, do_sample=False) ``` would issue a deprecation warning not to pass `GenerationConfig` as well as generation kwargs even though we're not passing a `GenerationConfig`. Along the fix this also introduces a unit test to check both the intended and the bug behavior.
Commit 38e5987 from PR #43194 introduced a minor bug that causes the issuing of a deprecation warning regardless of the user using the deprecated interface. For example: ``` model.generate(**input_ids, do_sample=False) ``` would issue a deprecation warning not to pass `GenerationConfig` as well as generation kwargs even though we're not passing a `GenerationConfig`. Along the fix this also introduces a unit test to check both the intended and the bug behavior. Co-authored-by: nemo <git@ningu.net>
Commit 38e5987 from PR huggingface#43194 introduced a minor bug that causes the issuing of a deprecation warning regardless of the user using the deprecated interface. For example: ``` model.generate(**input_ids, do_sample=False) ``` would issue a deprecation warning not to pass `GenerationConfig` as well as generation kwargs even though we're not passing a `GenerationConfig`. Along the fix this also introduces a unit test to check both the intended and the bug behavior. Co-authored-by: nemo <git@ningu.net>
* fix tests * custom gen config now works
Commit 38e5987 from PR huggingface#43194 introduced a minor bug that causes the issuing of a deprecation warning regardless of the user using the deprecated interface. For example: ``` model.generate(**input_ids, do_sample=False) ``` would issue a deprecation warning not to pass `GenerationConfig` as well as generation kwargs even though we're not passing a `GenerationConfig`. Along the fix this also introduces a unit test to check both the intended and the bug behavior. Co-authored-by: nemo <git@ningu.net>
3 participants
What does this PR do?
Some audio models have their own
generate()which doesn't callprepare_generation_config. We need to callprepare_generation_configto update values to prev defaultsAlso, I added a small deprecation warning preparing us and users to passing either generation config of
kwargs, only one. We will anyway have to support it for a long time, it is a common pattern among users