[None][fix] make FA4 proper pip dependency - #13788
Conversation
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #46890 [ run ] triggered by Bot. Commit: |
|
PR_Github #46890 [ run ] completed with state
|
de0a859 to
0c0f333
Compare
|
/bot run --disable-fail-fast --add-multi-gpu-test |
2 similar comments
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #47045 [ run ] triggered by Bot. Commit: |
|
PR_Github #47045 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #47230 [ run ] triggered by Bot. Commit: |
|
PR_Github #47230 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #47431 [ run ] triggered by Bot. Commit: |
|
PR_Github #47431 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #47622 [ run ] triggered by Bot. Commit: |
|
PR_Github #47622 [ run ] completed with state
|
c9b3ed9 to
aaa667a
Compare
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #48027 [ run ] triggered by Bot. Commit: |
|
PR_Github #48027 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #48212 [ run ] triggered by Bot. Commit: |
|
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 (43)
💤 Files with no reviewable changes (34)
📝 WalkthroughWalkthroughThis PR migrates from a vendored Flash Attention 4 implementation to an external dependency. Requirements now pin ChangesFlash Attention External Dependency Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
PR_Github #48212 [ run ] completed with state |
|
PR_Github #48943 [ run ] triggered by Bot. Commit: |
|
PR_Github #48943 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #48956 [ run ] triggered by Bot. Commit: |
|
PR_Github #48956 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test --disable-artifact-copy |
|
PR_Github #48961 Bot args parsing error: usage: /bot [-h] |
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #48974 [ run ] triggered by Bot. Commit: |
|
PR_Github #48974 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #49236 [ run ] triggered by Bot. Commit: |
|
PR_Github #49236 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
1 similar comment
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #49476 [ run ] triggered by Bot. Commit: |
|
PR_Github #49476 [ run ] completed with state
|
Signed-off-by: Olivia Stoner <245287810+o-stoner@users.noreply.github.com>
0c1b24a to
9049c01
Compare
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #49988 [ run ] triggered by Bot. Commit: |
|
PR_Github #49988 [ run ] completed with state
|
|
/bot run --disable-fail-fast --add-multi-gpu-test |
|
PR_Github #50349 [ run ] triggered by Bot. Commit: |
|
PR_Github #50349 [ run ] completed with state |
Signed-off-by: Olivia Stoner <245287810+o-stoner@users.noreply.github.com>
…ysses Per reviewer feedback, remove the ``parallel.audio_pad_for_ulysses`` ParallelConfig knob and derive audio padding behavior internally based on the runtime context. With Ulysses active, audio is always padded to make ``T_a`` divisible by ``ulysses_size`` and a ``[B, T_a_padded]`` validity mask is attached so attention zeros pad slots; with Ulysses inactive nothing changes. The opt-out mode the knob exposed was a silent performance downgrade (disengaged the v2a Ulysses wrapper, fell back to plain attention on non-divisible T_a) and not something users should be deciding. Source changes (all behavior-equivalent to the previous True default): * Drop ``ParallelConfig.audio_pad_for_ulysses``. * ``LTX2Attention._init_audio_modules``: TRTLLM→VANILLA downgrade for ``audio_attn1`` now keys on ``vgm.ulysses_size > 1`` (the condition that actually drives the mask requirement) instead of the dropped flag. * ``LTXModel.__init__``: drop the cached ``self._audio_pad_for_ulysses``. * ``LTXModel.configure_audio_ulysses``: always pad when the sharder is active. Removes the dead "no-pad mode" branch and the CP-without- Ulysses ``ValueError`` (now unreachable because padding always makes audio shardable). * Docstrings + the ``audio_padding_mask`` field comment updated. Tests adjusted: drop the parameter from the three call sites in test_ltx2_ulysses.py / test_ulysses_attention.py / test_fa4_key_padding_mask.py. Verified locally with the multi-GPU e2e LTXModel parity test (VANILLA backend, ws=2): both ``test_av_ulysses_no_audio_pad`` and ``test_av_ulysses_audio_pad`` PASS. The FA4 backend cases failed in this container due to an unrelated env regression already fixed on main (PR NVIDIA#13788, ``nvvm.fmax`` API change in ``nvidia_cutlass_dsl``); CI on a fresh container picks up the fix automatically. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
…ysses Per reviewer feedback, remove the ``parallel.audio_pad_for_ulysses`` ParallelConfig knob and derive audio padding behavior internally based on the runtime context. With Ulysses active, audio is always padded to make ``T_a`` divisible by ``ulysses_size`` and a ``[B, T_a_padded]`` validity mask is attached so attention zeros pad slots; with Ulysses inactive nothing changes. The opt-out mode the knob exposed was a silent performance downgrade (disengaged the v2a Ulysses wrapper, fell back to plain attention on non-divisible T_a) and not something users should be deciding. Source changes (all behavior-equivalent to the previous True default): * Drop ``ParallelConfig.audio_pad_for_ulysses``. * ``LTX2Attention._init_audio_modules``: TRTLLM→VANILLA downgrade for ``audio_attn1`` now keys on ``vgm.ulysses_size > 1`` (the condition that actually drives the mask requirement) instead of the dropped flag. * ``LTXModel.__init__``: drop the cached ``self._audio_pad_for_ulysses``. * ``LTXModel.configure_audio_ulysses``: always pad when the sharder is active. Removes the dead "no-pad mode" branch and the CP-without- Ulysses ``ValueError`` (now unreachable because padding always makes audio shardable). * Docstrings + the ``audio_padding_mask`` field comment updated. Tests adjusted: drop the parameter from the three call sites in test_ltx2_ulysses.py / test_ulysses_attention.py / test_fa4_key_padding_mask.py. Verified locally with the multi-GPU e2e LTXModel parity test (VANILLA backend, ws=2): both ``test_av_ulysses_no_audio_pad`` and ``test_av_ulysses_audio_pad`` PASS. The FA4 backend cases failed in this container due to an unrelated env regression already fixed on main (PR NVIDIA#13788, ``nvvm.fmax`` API change in ``nvidia_cutlass_dsl``); CI on a fresh container picks up the fix automatically. Signed-off-by: Yiyun Lu <55233584+luyiyun1021@users.noreply.github.com>
5 participants
Summary by CodeRabbit
Release Notes
Chores
Refactor
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.