-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
[MusicGen] Add sampling rate to config #26136
[MusicGen] Add sampling rate to config #26136
Conversation
|
||
forward_params = {"max_new_tokens": 10} | ||
# test single sample | ||
outputs = music_generator("this", forward_params=forward_params) |
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.
Can only use very limited prompts for the tiny model since it has a vocab size of 99 😅
|
||
forward_params = { | ||
"do_sample": False, | ||
"max_new_tokens": 250, | ||
} | ||
|
||
outputs = speech_generator("This is a test", forward_params=forward_params) | ||
# musicgen sampling_rate is not straightforward to get |
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.
Fixed by this PR! The remainder of the changes are just small refactors to the pipeline tests
The documentation is not available anymore as the PR was closed or merged. |
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 just have a small question regarding testing the tiny musicgen, but otherwise thanks for taking care of this @sanchit-gandhi and for the clearer renaming !
@require_torch | ||
def test_tiny_musicgen_pt(self): | ||
music_generator = pipeline( | ||
task="text-to-audio", | ||
model="hf-internal-testing/tiny-random-MusicgenForConditionalGeneration", | ||
framework="pt", | ||
) |
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 might be wrong, but models that are in hf-internal-testing
should be tested with run_pipeline_test
called by PipelineTesterMixin
, right ?
Maybe @ydshieh can answer this question ?
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.
Good spot - MusicGen currently isn't tested by the pipeline, but I'll fix this in a follow-up PR: #26146
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.
Looks good, good catch!
Co-authored-by: Arthur <[email protected]>
* [MusicGen] Add sampling rate to config * remove tiny * make property * Update tests/pipelines/test_pipelines_text_to_audio.py Co-authored-by: Arthur <[email protected]> * style --------- Co-authored-by: Arthur <[email protected]>
* [MusicGen] Add sampling rate to config * remove tiny * make property * Update tests/pipelines/test_pipelines_text_to_audio.py Co-authored-by: Arthur <[email protected]> * style --------- Co-authored-by: Arthur <[email protected]>
* [MusicGen] Add sampling rate to config * remove tiny * make property * Update tests/pipelines/test_pipelines_text_to_audio.py Co-authored-by: Arthur <[email protected]> * style --------- Co-authored-by: Arthur <[email protected]>
What does this PR do?
Adds sampling rate attribute to the parent model config. We get the sampling rate from the EnCodec sub-model config. Adding this attribute brings consistency with the other TTA models in the lib, where the output sampling rate is accessible directly through the top-level config (huggingface/audio-transformers-course#140 (comment))