Implement VibeVoice - #40546
Conversation
ebezzam
left a comment
There was a problem hiding this comment.
@pengzhiliang thanks for the PR! This is an exciting model to add 🔥
My first comments are mainly on rearranging content to be consistent with the other models in Transformers, and creating a modular file to better optimize copying components from other models in Transformers.
There are also some other files to modify:
- in
src/transformers/models/auto - in
docs - and eventually some tests (for which a lot of code can be copied from other models)
As an example of typical files to create/modify, you can check out the Qwen2.5-Omni PR, which is also multimodal.
|
Hopefully this PR can get merged, since the deletion of the VibeVoice repo not sure the original authors can continue contributing to this PR |
|
Glad to see that someone has picked this PR up, thanks 🤗 |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, csm, vibevoice, vibevoice_acoustic_tokenizer |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, csm, vibevoice, vibevoice_acoustic_tokenizer |
1 similar comment
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, csm, vibevoice, vibevoice_acoustic_tokenizer |
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, csm, vibevoice, vibevoice_acoustic_tokenizer |
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, csm, vibevoice, vibevoice_acoustic_tokenizer |
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
| # Default to the feature extractor's own sampling rate | ||
| kwargs.setdefault("sampling_rate", self.feature_extractor.sampling_rate) |
There was a problem hiding this comment.
ugly hack to avoid the warning:
[transformers] It is strongly recommended to pass the `sampling_rate` argument to `VibeVoiceAcousticTokenizerFeatureExtractor()`. Failing to do so can result in silent errors that might be hard to debug.
because _defaults needs to be empty to satisfy this new style check
There was a problem hiding this comment.
You can add a trf ignore instead of this workaround
There was a problem hiding this comment.
thanks, and I wonder if we really need to keep this type of guard for future feature extractors? otherwise new models will always need this ignore
There was a problem hiding this comment.
We need it, it's an opinionated choice that's been made and that is still of great importance IMO. It means that the style check will have to get adapted to take this into account thought
| # NOTE: mask padding through the attention mask, as the padding and EOS token share the same ID | ||
| if "attention_mask" in data: | ||
| labels[data["attention_mask"] == 0] = -100 |
| {%- if not add_generation_prompt %} | ||
| {%- if has_target_audio %}{{ audio_token }}{{ audio_eos_token }}{% endif %}{{ eos_token }} | ||
| {%- endif %}""" |
There was a problem hiding this comment.
one idea for distinguishing between training/inference regime
add_generation_prompt=True(inference) -> no eos_tokenadd_generation_prompt=False(training) -> append eos_token
here is a script to see the chat template rendering before tokenization.
there is one TRL chat template that does this, but in reality TRL doesn't have a strict requirement on the chat template as they often create their own to make sure they are compatible with training. See their collection of chat templates here and guidelines here.
although I'm still not sure if this is the best approach. As Higgs still sets add_generation_prompt=True for training here
There was a problem hiding this comment.
add_generation_prompt=True on higgs shouldn't be there, nice catch !!
nevertheless we got lucky there because eos comes from the codebooks, so this has no detrimental effect
There was a problem hiding this comment.
ahhhhh it's really tricky here, I'd be tempted to say yes, let's use add_generation_prompt for that but it means overloading a parameter for a behavior it was not intended for...
Nevertheless TRL does use it to that end, so I am ok with that!
| # Processor kwargs are passed separately from Jinja2 template kwargs. | ||
| processor_kwargs = kwargs.pop("processor_kwargs", None) or {} | ||
| chat_template_kwargs = { | ||
| "tokenize": True, | ||
| "return_dict": True, | ||
| "add_generation_prompt": True, | ||
| "processor_kwargs": processor_kwargs, | ||
| **kwargs, | ||
| } | ||
| output = preprocessor.apply_chat_template( | ||
| text.messages, | ||
| tokenize=True, | ||
| return_dict=True, | ||
| **kwargs, | ||
| **chat_template_kwargs, |
There was a problem hiding this comment.
(this file in general needs a revamp, but I think worth adding things as we see them?)
We should be passing "add_generation_prompt": True? As do all of Higgs audio examples and if we do go with using add_generation_prompt to distinguish between training/inference for Vibevoice
as well as passing processor_kwargs separately
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, csm, vibevoice, vibevoice_acoustic_tokenizer |
| [conversation1, conversation2], | ||
| add_generation_prompt=True, | ||
| tokenize=True, |
| if not is_soundfile_available(): | ||
| raise ImportError("Please install `soundfile` to save audio files.") |
There was a problem hiding this comment.
updating to main you'll be able to use requires_backends(self, ["soundfile"])
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, csm, vibevoice, vibevoice_acoustic_tokenizer |
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, csm, vibevoice, vibevoice_acoustic_tokenizer |
CI recapDashboard: View test results in Grafana |
What does this PR do?
Merge the model from https://github.com/microsoft/VibeVoice/tree/main
HF:
https://huggingface.co/microsoft/VibeVoice-1.5B