Repository navigation
Fix Qwen2.5-Omni Token2Wav DiT rotary embedding layout (interleaved cos/sin) - #47403
Conversation
The Token2Wav DiT applies an interleaved rotate (`rotate_half_codec`, pairing channels (2i, 2i+1)), but since huggingface#39847 the DiT rotary embedding built cos/sin with the half-split `torch.cat((freqs, freqs))` layout inherited from `LlamaRotaryEmbedding`. A half-split cos/sin combined with an interleaved rotate is not a valid rotation: RoPE's translation invariance is destroyed on head 0 of every DiT attention layer, degrading the predicted mel-spectrogram (and thus the synthesized audio). Override `Qwen2_5OmniDiTRotaryEmbedding.forward` to emit the interleaved `repeat_interleave(freqs, 2)` layout, restoring the pre-huggingface#39847 behavior that matches `rotate_half_codec` and the released weights (originally integration-tested in huggingface#36752). Add CPU regression tests asserting the interleaved layout and RoPE translation invariance.
ebezzam
left a comment
There was a problem hiding this comment.
@HenryVarro666 thanks for spotting this regression and opening a PR!
I was able to observe it myself with this script and these output audio to compare generated audio (BEFORE and AFTER fix).
I agree with you that at this point we don't want to change the released weights. Although I think we can do a different fix to better align with library conventions, as that was the intention of #39847. To that end, I've left some comment.
cc @zucchini-nlp for another opinion!
| @torch.no_grad() | ||
| @dynamic_rope_update # power user: used with advanced RoPE types (e.g. dynamic rope) | ||
| def forward(self, x, position_ids): | ||
| inv_freq_expanded = self.inv_freq[None, :, None].float().expand(position_ids.shape[0], -1, 1).to(x.device) | ||
| position_ids_expanded = position_ids[:, None, :].float() | ||
|
|
||
| device_type = x.device.type if isinstance(x.device.type, str) and x.device.type != "mps" else "cpu" | ||
| with maybe_autocast(device_type=device_type, enabled=False): # Force float32 | ||
| freqs = (inv_freq_expanded.float() @ position_ids_expanded.float()).transpose(1, 2) | ||
| # The DiT applies an interleaved rotary (``rotate_half_codec``, pairing channels | ||
| # ``(2i, 2i + 1)``), so cos/sin must interleave the frequencies to match. The | ||
| # half-split ``torch.cat((freqs, freqs))`` layout mismatches the rotate and breaks | ||
| # RoPE's translation invariance. Regression from #39847; see #47328. | ||
| emb = torch.repeat_interleave(freqs, 2, dim=-1) | ||
| cos = emb.cos() * self.attention_scaling | ||
| sin = emb.sin() * self.attention_scaling | ||
|
|
||
| return cos.to(dtype=x.dtype), sin.to(dtype=x.dtype) |
There was a problem hiding this comment.
let's keep as is and rather change apply_rotary_pos_emb and how we use it (see here)
| @torch.no_grad() | |
| @dynamic_rope_update # power user: used with advanced RoPE types (e.g. dynamic rope) | |
| def forward(self, x, position_ids): | |
| inv_freq_expanded = self.inv_freq[None, :, None].float().expand(position_ids.shape[0], -1, 1).to(x.device) | |
| position_ids_expanded = position_ids[:, None, :].float() | |
| device_type = x.device.type if isinstance(x.device.type, str) and x.device.type != "mps" else "cpu" | |
| with maybe_autocast(device_type=device_type, enabled=False): # Force float32 | |
| freqs = (inv_freq_expanded.float() @ position_ids_expanded.float()).transpose(1, 2) | |
| # The DiT applies an interleaved rotary (``rotate_half_codec``, pairing channels | |
| # ``(2i, 2i + 1)``), so cos/sin must interleave the frequencies to match. The | |
| # half-split ``torch.cat((freqs, freqs))`` layout mismatches the rotate and breaks | |
| # RoPE's translation invariance. Regression from #39847; see #47328. | |
| emb = torch.repeat_interleave(freqs, 2, dim=-1) | |
| cos = emb.cos() * self.attention_scaling | |
| sin = emb.sin() * self.attention_scaling | |
| return cos.to(dtype=x.dtype), sin.to(dtype=x.dtype) |
There was a problem hiding this comment.
(not on you) but let's actually use apply_rotary_pos_emb from Llama to take advantage of the kernel implementation for speed
from ..llama.modeling_llama import LlamaRotaryEmbedding, apply_rotary_pos_emb, rotate_half
There was a problem hiding this comment.
we can remove this custom implementation and use that of Llama, which has a kernel implementation
and instead define a method
def deinterleave_head_dim(x: torch.Tensor) -> torch.Tensor:
"""Reorder the head dim from interleaved RoPE layout `(x0, x1, x2, x3, ...)` to the standard half-split layout
`(x0, x2, ..., x1, x3, ...)` so the kernelized `apply_rotary_pos_emb` can be used.
"""
return torch.cat((x[..., 0::2], x[..., 1::2]), dim=-1)and use it here like so:
# apply rotary position embedding
# Due to training process, only first head is applied with RoPE, will be fixed at next release
# The checkpoint uses interleaved RoPE, so deinterleave q/k before the half-split rotation.
cos, sin = position_embeddings
query[:, :1], key[:, :1] = apply_rotary_pos_emb(
deinterleave_head_dim(query[:, :1]), deinterleave_head_dim(key[:, :1]), cos, sin
)|
|
||
|
|
||
| @require_torch | ||
| class Qwen2_5OmniDiTRotaryEmbeddingTest(unittest.TestCase): |
There was a problem hiding this comment.
instead of this test, let's instead add an integration test to this collection which checks that the audio outputs don't regress.
For example, see this test which uses a separate script to compute expected output and applies the model to make sure outputs stay similar here. It is for a codec model, but a similar idea can be applied for audio generation models (be sure to set a seed and temperature=0 for deterministic outputs).
|
Thanks a lot for the thorough review @ebezzam — and for independently confirming the regression with your script and the before/after audio! Your direction sounds good to me: deinterleaving q/k ahead of the kernelized For the integration test, I followed the xcodec2 fixture pattern with one adaptation: it replays talker codec tokens captured from a deterministic seeded |
| def deinterleave_head_dim(x: torch.Tensor) -> torch.Tensor: | ||
| """Reorder the head dim from interleaved RoPE layout `(x0, x1, x2, x3, ...)` to the standard half-split layout | ||
| `(x0, x2, ..., x1, x3, ...)` so the kernelized `apply_rotary_pos_emb` can be used. | ||
| """ | ||
| return torch.cat((x[..., 0::2], x[..., 1::2]), dim=-1) | ||
|
|
There was a problem hiding this comment.
hmm, maybe we can permute once in conversion mapping instead of de-interleaving at each forward run?
There was a problem hiding this comment.
yep, there is a class PermuteForRope which currently is broken but easy to fix the permutation axis, and add in mapping
There was a problem hiding this comment.
@HenryVarro666 can you switch to this and remove deinterleave_head_dim?
There was a problem hiding this comment.
Thanks both, happy to move toward the library convention here!
One wrinkle worth flagging before wiring it up: in this DiT, RoPE is applied to head 0 only (query[:, :1] / key[:, :1]), while to_q/to_k are a single fused Linear(hidden_size, num_heads * head_dim). PermuteForRope permutes all heads, so applying it to the whole to_q/to_k weight would deinterleave head 0 (what we want) but also permute heads 1–15, which are used un-rotated — that would change their attention scores. So the conversion op has to permute only the first head_dim rows (and, since to_q/to_k carry bias, the first head_dim bias entries too), leaving the other heads untouched — i.e. a small custom op rather than PermuteForRope as-is, plus a matching reverse_op so save_pretrained round-trips back to the interleaved checkpoint.
I'm glad to implement that (permute the head-0 slice at load via a qwen2_5_omni_dit conversion mapping, and drop deinterleave_head_dim). One thing to weigh: since the deinterleave here only ever touches head 0's (batch, 1, seq, head_dim) slice, the per-forward cost is negligible, so in this specific case the speed benefit of moving it to load time is marginal. Either is fine by me:
- (a) keep the current runtime
deinterleave_head_dim(localized, already verified against the checkpoint), or - (b) move it to a load-time head-0 conversion op as suggested.
Just let me know which you'd prefer and I'll push it. Either way, on the test I'll drop the fixture file and switch to a lightweight integration check (~50 codes in / 50 output values compared via Expectation) with a linked gist for reproducing the expected values, following the audioflamingo3 example. 👍
There was a problem hiding this comment.
huh, didn't know that, it makes DiT similar to some recent deepseek with NOPE dims. In that case i think it is not worth converting at run-time and we can keep the de-interleaved
There was a problem hiding this comment.
not a fan of this, can we check a slice of logits/codes/mean/std and keep the expected values as testing_utils.Expectation?
There was a problem hiding this comment.
audio conventions of adding such fixture files 🙈 but I'm also fine with what @zucchini-nlp is suggesting!
-
adding a link to a script for computing the expected values (as mentioned here) so we can reproduce them if needed in the future
-
only inputing 50 codes and comparing 50 output values so the test is lighter
There was a problem hiding this comment.
ahh i see, didn't know that. Yeah, whatever works for audio better, just suggesting how we did in text generation models :)
| self.assertFalse(torch.isnan(output[1]).any().item()) | ||
|
|
||
| @slow | ||
| def test_small_model_integration_test_token2wav_regression(self): |
There was a problem hiding this comment.
hopefully this fits in out runners 😅
| Regression test for #47328: the Token2Wav DiT applies an interleaved rotary embedding | ||
| (pairing channels `(2i, 2i + 1)`), so its `cos`/`sin` must be applied accordingly; a | ||
| half-split application silently degrades the generated audio. This test drives the | ||
| Token2Wav module of the released checkpoint with pre-recorded talker codec tokens and | ||
| checks that the generated waveform does not regress. The waveform is compared on a | ||
| strided sample signature spanning the whole utterance (a contiguous head slice would | ||
| only cover leading silence) plus its overall RMS. | ||
|
|
||
| The fixture was produced with the reproducer script in PR #47403 (deterministic | ||
| `generate()` run: greedy thinker/talker, `torch.manual_seed(0)`, then the captured | ||
| talker codes replayed through `model.token2wav` exactly as below). |
There was a problem hiding this comment.
can you instead put a link to a gist like this? we we can reproduce the expected output in the future
There was a problem hiding this comment.
audio conventions of adding such fixture files 🙈 but I'm also fine with what @zucchini-nlp is suggesting!
-
adding a link to a script for computing the expected values (as mentioned here) so we can reproduce them if needed in the future
-
only inputing 50 codes and comparing 50 output values so the test is lighter
|
Thanks @zucchini-nlp @ebezzam, that makes sense. I've kept the runtime I reworked the audio regression test along the lines you both suggested (pushed to the same branch):
Values were captured on an H100 (cc 9.0). On |
zucchini-nlp
left a comment
There was a problem hiding this comment.
Thanks a lot, lgtm! Lets wait for Eric and then we can merge
|
run-slow: qwen2_5_omni |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: qwen2_5_omni |
|
This comment contains models: ["models/qwen2_5_omni"] |
|
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 recapDashboard: View test results in Grafana |
ebezzam
left a comment
There was a problem hiding this comment.
thanks @HenryVarro666 for the updates! I pushed some small changes and ran our tests. Everything checks out 👍
thanks for the contribution!
| signature = waveform[:: waveform.shape[-1] // 50][:50] | ||
| expected_signatures = Expectations( | ||
| { | ||
| ("cuda", 8): torch.tensor([ |
There was a problem hiding this comment.
added outputs for our CI/machines
| @slow | ||
| def test_small_model_integration_test_token2wav_regression(self): | ||
| """ | ||
| reproducer (for the expected values below): https://gist.github.com/ebezzam/12286028df44e91434f7c770efc4e5b5 |
There was a problem hiding this comment.
shortened description to just the reproducer (copied yours and made small tweaks in the comment header)
|
Thanks @ebezzam @zucchini-nlp for the careful review, and for handling the merge-main and CI outputs on your side. Glad to get this one fixed in. |
…os/sin) (huggingface#47403) * Fix Qwen2.5-Omni Token2Wav DiT rotary embedding layout The Token2Wav DiT applies an interleaved rotate (`rotate_half_codec`, pairing channels (2i, 2i+1)), but since huggingface#39847 the DiT rotary embedding built cos/sin with the half-split `torch.cat((freqs, freqs))` layout inherited from `LlamaRotaryEmbedding`. A half-split cos/sin combined with an interleaved rotate is not a valid rotation: RoPE's translation invariance is destroyed on head 0 of every DiT attention layer, degrading the predicted mel-spectrogram (and thus the synthesized audio). Override `Qwen2_5OmniDiTRotaryEmbedding.forward` to emit the interleaved `repeat_interleave(freqs, 2)` layout, restoring the pre-huggingface#39847 behavior that matches `rotate_half_codec` and the released weights (originally integration-tested in huggingface#36752). Add CPU regression tests asserting the interleaved layout and RoPE translation invariance. * Use kernelized apply_rotary_pos_emb with deinterleaved q/k in Token2Wav DiT * Replace DiT rotary unit tests with Token2Wav audio regression test * Replace audio-regression fixture with inline Expectations-based test * Add expected outputs for our CI. --------- Co-authored-by: Eric B <ebezzam@gmail.com> Co-authored-by: Eric Bezzam <4757445+ebezzam@users.noreply.github.com>
…os/sin) (huggingface#47403) * Fix Qwen2.5-Omni Token2Wav DiT rotary embedding layout The Token2Wav DiT applies an interleaved rotate (`rotate_half_codec`, pairing channels (2i, 2i+1)), but since huggingface#39847 the DiT rotary embedding built cos/sin with the half-split `torch.cat((freqs, freqs))` layout inherited from `LlamaRotaryEmbedding`. A half-split cos/sin combined with an interleaved rotate is not a valid rotation: RoPE's translation invariance is destroyed on head 0 of every DiT attention layer, degrading the predicted mel-spectrogram (and thus the synthesized audio). Override `Qwen2_5OmniDiTRotaryEmbedding.forward` to emit the interleaved `repeat_interleave(freqs, 2)` layout, restoring the pre-huggingface#39847 behavior that matches `rotate_half_codec` and the released weights (originally integration-tested in huggingface#36752). Add CPU regression tests asserting the interleaved layout and RoPE translation invariance. * Use kernelized apply_rotary_pos_emb with deinterleaved q/k in Token2Wav DiT * Replace DiT rotary unit tests with Token2Wav audio regression test * Replace audio-regression fixture with inline Expectations-based test * Add expected outputs for our CI. --------- Co-authored-by: Eric B <ebezzam@gmail.com> Co-authored-by: Eric Bezzam <4757445+ebezzam@users.noreply.github.com>
The generated `modeling_*.py` files for qwen3_tts and its two audio tokenizers were last produced on 2026-06-30 and had drifted from the parent models they inherit from (qwen3, qwen2_5_omni, qwen3_omni_moe, mimi). Re-running `utils/modular_model_converter.py` picks up those changes: `nn.Buffer` instead of `register_buffer` (huggingface#47722), the `inv_freq` linting annotation removal (huggingface#47753), the kernel decorator rename to `use_kernel_forward_from_hub` (huggingface#46883), the Token2Wav DiT interleaved-RoPE fix (huggingface#47403), removal of the legacy FlashAttention2 / SdpaAttention classes, and assorted spelling fixes (huggingface#47684). Three fixes were needed in the modular sources so that regeneration does not regress behaviour: - Move `Qwen3TTSConfig.get_text_config` into `modular_qwen3_tts.py`. It had only ever been added to the generated configuration file, so the converter dropped it. - Pin `Qwen3TTSTalkerModel._no_split_modules` to `["Qwen3TTSTalkerDecoderLayer"]`. The converter resolves the inherited value with the most common prefix (`Qwen3TTS`) and would otherwise point at the wrong decoder layer class. - Add a `Qwen3TTSTokenizerMultiCodebookSnakeBeta` alias and use it in the code2wav decoder. Upstream renamed `SnakeBeta` to `Qwen2_5OmniSnakeBeta`, which left an undefined `SnakeBeta` reference in the generated file. Conversion is idempotent, `ruff check` / `ruff format --check` pass, and all touched files compile. Claude-Session: https://claude.ai/code/session_01WLU94QdJCmuVf181X98tno
What does this PR do?
Fixes #47328.
The Token2Wav DiT applies its rotary embedding with the interleaved convention — the released checkpoint pairs adjacent channels
(2i, 2i + 1)— but since #39847 (🚨 [v5] Refactor RoPE for layer types)Qwen2_5OmniDiTRotaryEmbeddinginheritedLlamaRotaryEmbedding.forward, which buildscos/sinwith the half-split layouttorch.cat((freqs, freqs)), while the module kept its interleaved rotate (rotate_half_codec). A half-splitcos/sincombined with an interleaved rotate is not a valid rotation: RoPE's defining property —⟨R_m·q, R_n·k⟩depending only on the relative offsetm − n— is destroyed. It is applied to head 0 of every DiT attention layer, silently degrading the predicted mel-spectrogram and thus the synthesized audio forreturn_audio=True.This is a regression, not the checkpoint design. Before #39847 (e.g.
v4.57.0),Qwen2_5OmniDiTRotaryEmbedding.forwardbuilt the embedding astorch.stack((freqs, freqs), dim=-1).reshape(...)— the interleaved layout matchingrotate_half_codec. The original add PR #36752 was integration-tested againstQwen/Qwen2.5-Omni-7Bwith that convention, so the released weights expect it.The fix (as suggested in review — thanks @ebezzam!)
Rather than restoring the interleaved
cos/sinlayout (the first version of this PR), the fix now keeps the standard half-split rotary embedding and aligns the application with library conventions, per the review:apply_rotary_pos_emb/rotate_half_codecand use the kernelizedapply_rotary_pos_embfrom Llama;deinterleave_head_dimhelper and reorderq/k(head 0 only) from the checkpoint's interleaved layout to the half-split layout before the rotation.This is numerically equivalent to the checkpoint's original interleaved rotation:
deinterleave_head_dimmaps each pair(2i, 2i + 1)to positions(i, d/2 + i), which the half-splitcos/sinrotates by the sameθ_i. The rotatedq/kdiffer from the interleaved formulation only by a channel permutation applied consistently to both, which the attention scoresq·kᵀare invariant to — verified: attention scores match the interleaved-layout fix to ~1e-5 (fp32), while the brokenmainpath differs by ~4e1. RoPE translation invariance is restored (drift ~1e-5 across offsets vs ~10.7 onmain).The text model's mrope (
Qwen2_5OmniRotaryEmbedding) legitimately uses the half-split convention end-to-end and is untouched;qwen3_omni_moehas its own rotary and is unaffected (independently confirmed by @pepepeboom, who also reproduced the root cause — thanks!).Tests
Replaces the previous unit tests with an integration test (
test_small_model_integration_test_token2wav_regression), as requested in review: it drives the Token2Wav module of the released checkpoint with pre-recorded talker codec tokens (fixture produced by the reproducer below) and checks the generated waveform against expected outputs —torch.testing.assert_closeon a strided sample signature spanning the whole utterance (2,040 points; a contiguous head slice would only cover the leading silence) plus the overall RMS. Token2Wav runs in fp32 with a single seededtorch.randn, so the comparison is stable on a given device. Measured margins (H100):mainThe test deliberately replays captured codes through
model.token2wavinstead of running the fullgenerate()pipeline: the bf16 thinker/talker greedy decode can flip tokens across GPU architectures (see the per-deviceExpectationstables already in this test file), which would make an end-to-end audio comparison flaky, while the fp32 Token2Wav component — where the regression lives — is stable.Reproducer script that produced the fixture (run on the fixed branch)
Before submitting
Who can review?
@eustlb @ebezzam
Thanks to @pepepeboom for independently reproducing the root cause and confirming the regression on
main.