[None][fix] LTX-2 audio PE pad: use token-axis seq_dim=1 for token-major rope - #14818
Conversation
|
/bot run --disable-fail-fast |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR fixes audio text rotary positional embedding padding in LTXModel's ChangesAudio PE padding fix and test improvements
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #51346 [ run ] triggered by Bot. Commit: |
54502ef to
8609028
Compare
…jor rope `prepare_text_cache` extended the audio PE with a `seq_dim` derived via `ndim == 4 -> seq_dim=2`, which predates the SPLIT-rope switch to the token-major `[B, T, H, D]` layout. With token-major rope the token axis is dim 1; the heuristic instead pads the head axis and `_make_pe_local` still shards the unpadded token axis, leaving `cos.shape[1]` short of the padded latent. The fused split-norm-rope kernel then rejects the cos/num_tokens mismatch at runtime (e.g. `cos=31, num_tokens=32` for WAN-style warmup; `cos=7, num_tokens=8` at the unit-test scale). Pin the seq_dim to 1 to match `_make_pe_local`'s `cos[:, s:e]` shard; this is uniform across SPLIT (4D) and INTERLEAVED (3D) layouts. Default the audio-padding unit test to `rope_type=SPLIT` so the regression that motivated the original `_pad_pe` plumbing is actually exercised on the production layout. The previous INTERLEAVED default produced 3D PE, never hit the buggy 4D branch, and silently passed. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
8609028 to
27589d8
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #51349 [ run ] triggered by Bot. Commit: |
|
PR_Github #51346 [ run ] completed with state |
|
PR_Github #51349 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51379 [ run ] triggered by Bot. Commit: |
|
PR_Github #51379 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51499 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #51499 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51600 [ run ] triggered by Bot. Commit: |
|
PR_Github #51603 [ run ] triggered by Bot. Commit: |
|
PR_Github #51600 [ run ] completed with state |
|
PR_Github #51613 [ run ] triggered by Bot. Commit: |
|
PR_Github #51603 [ run ] completed with state |
|
PR_Github #51613 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51729 [ run ] triggered by Bot. Commit: |
|
PR_Github #51729 [ run ] completed with state |
Summary
prepare_text_cachewas padding the head axis (dim 2) instead of the token axis (dim 1) for 4D positional embeddings, leavingcos.shape[1]short of the padded latent. The fused split-norm-rope kernel then trips itscos_emb.size(0) == num_tokenscheck at runtime withcos=Nvsnum_tokens=N+1(e.g.cos=31, num_tokens=32at WAN-style warmup;cos=7, num_tokens=8at the unit-test scale).rope_type=SPLITso it actually exercises the production token-major rope layout. Previously the test fixture usedINTERLEAVED(3D PE), which made thendim == 4 -> seq_dim=2heuristic fall through to the correctseq_dim=1by accident and missed the regression entirely.Root cause
a173175ac2 [None][perf] ltx2: switch SPLIT-rope cos/sin to token-major [B, T, H, D]changed the SPLIT-rope layout from[B, H, T, D]to token-major[B, T, H, D]and removed allswapaxes(1, 2)plumbing; at the same time it correctly setpe_seq_dim = 1in the (then-existing) code that touched PE shape.b1dfd3094c [TRTLLM-12653][feat] LTX-2 Ulysses cross-attention for v2a with audio padding (#14044)later introduced the_pad_pehelper plus its call site inprepare_text_cacheto extend audio PE under Ulysses. The new call site pickedseq_dimvia:That heuristic is the pre-token-major convention (
[B, H, T, D]). For the post-token-major SPLIT layout the actual seq axis is dim 1, so the pad grows the head axis instead and downstream_make_pe_local'scos[:, s:e]still shards the un-padded token axis — exactly the off-by-_audio_padwe observe at runtime.The existing
test_av_ulysses_audio_paddid not catch this because:LTXModeldefaultsrope_type=LTXRopeType.INTERLEAVED, and the test fixture left that default.ndim == 4branch is never taken and the call lands onseq_dim=1by accident.Fix
tensorrt_llm/_torch/visual_gen/models/ltx2/transformer_ltx2.py:This matches
_make_pe_local's slicing on dim 1 and is uniform across SPLIT/INTERLEAVED.tests/unittest/_torch/visual_gen/multi_gpu/test_ltx2_ulysses.py:rope_type_namethrough_logic_ltx2_av_ulysses_vs_single_gpuand default to"SPLIT"so the audio-padding parity test exercises the production layout that motivated the original_pad_peplumbing.test_av_ulysses_audio_pad[VANILLA]and[FA4]both fail with the expectedcos_emb.size(0) (7) must equal num_tokens (8)mismatch (verified locally on dev image).Test plan
pytest tests/unittest/_torch/visual_gen/multi_gpu/test_ltx2_ulysses.py -v(4 cases pass — no-pad / pad × VANILLA / FA4) on a 2-GPU dev image.mainwith the new SPLIT-default test (fails with the production-sidecos_emb=N, num_tokens=N+1message).PR Checklist
[JIRA/NVBUG/None][type] descriptionSummary by CodeRabbit
Bug Fixes
Tests