[TRTLLM-15177][chore] Consolidate trtllm-gen SiTu activation slot handling - #17414
[TRTLLM-15177][chore] Consolidate trtllm-gen SiTu activation slot handling#17414brnguyen2 wants to merge 2 commits into
Conversation
|
/bot run |
WalkthroughSiTu activation parameters now use shared ChangesSiTu parameter storage
Benchmark test waiver
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
PR_Github #64616 [ run ] triggered by Bot. Commit: |
|
PR_Github #64616 [ run ] completed with state
|
|
/bot run |
|
PR_Github #64651 [ run ] triggered by Bot. Commit: |
|
PR_Github #64651 [ run ] completed with state
|
|
/bot run |
e4e12fa to
be08d33
Compare
|
PR_Github #64674 [ run ] triggered by Bot. Commit: |
|
PR_Github #64674 [ run ] completed with state
|
|
/bot run |
|
PR_Github #64683 [ run ] triggered by Bot. Commit: |
|
PR_Github #64683 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64700 [ run ] triggered by Bot. Commit: |
|
PR_Github #64700 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "DGX_B200-PyTorch-8, GH200-PackageSanityCheck-PY312-UB2404" |
|
PR_Github #64733 [ run ] triggered by Bot. Commit: |
|
PR_Github #64733 [ run ] completed with state
|
|
/bot run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/test_lists/waives.txt`:
- Line 437: Add the repository-standard NVIDIA copyright header for 2026 at the
top of the modified waives.txt file, before its existing test waiver content,
without changing the waiver entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a231f53e-e230-4eba-99c7-8da9b85818fd
📒 Files selected for processing (1)
tests/integration/test_lists/waives.txt
| unittest/llmapi/test_llm_pytorch.py::test_gqa_nemo_lora[cuda_graph_config0] SKIP (https://nvbugs/6162504) | ||
| unittest/llmapi/test_llm_pytorch.py::test_llm_context_only_timed_out_kv_cache_exhausted[None-UCX-1000] SKIP (https://nvbugs/6490004) | ||
| unittest/llmapi/test_memory_profiling.py::test_profile_kvcache SKIP (https://nvbugs/5580781) | ||
| unittest/tools/test_layer_wise_benchmarks.py::test_performance_alignment[1] SKIP (bug pending, tracked in PR #17414) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required NVIDIA copyright header.
This modified file has no NVIDIA copyright header. Add the repository-standard header at the top of tests/integration/test_lists/waives.txt with year 2026.
As per coding guidelines, all modified files must contain the NVIDIA copyright header with the year of the latest meaningful modification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/test_lists/waives.txt` at line 437, Add the
repository-standard NVIDIA copyright header for 2026 at the top of the modified
waives.txt file, before its existing test waiver content, without changing the
waiver entry.
Source: Coding guidelines
…dling Item 1.2 of the PR NVIDIA#17269 deferred-cleanup bucket. (a) Record the decision to keep SiTu in the trtllm-gen-local ActType_TrtllmGen enum rather than adding it to the shared ActivationType, which mirrors the cutlass enum in common.h and would then need a cutlass member no cutlass kernel implements. Documented at the enum and left the enums unchanged. (b) Route SiTu and SwiGLU per-expert alpha/beta through a single _gemm1_activation_params accessor at the op call instead of duplicating the is_situ_activation ternary. Behavior-identical: same tensors, same op slot. The deeper storage merge (eliminating the separate situ_alpha/situ_beta buffers by reusing swiglu_alpha/swiglu_beta) is left as a TODO because 'swiglu_alpha is not None' gates quant-method selection and validation on the numeric path; it needs GPU parity revalidation before it can land. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
…swiglu_beta storage Complete item 1.2(b): drop the separate situ_alpha/situ_beta parameters and reuse the swiglu_alpha/swiglu_beta storage, so the gemm1 alpha/beta op slot has a single backing parameter and the _gemm1_activation_params accessor branch disappears. Safe with respect to the 'swiglu_alpha is not None' gates: - create_moe.py validates the constructor kwargs, which remain None for SiTu; the storage is populated later in create_weights. - _get_quant_method consults swiglu_alpha only on the nvfp4 branch; SiTu requires W4A8_MXFP4_MXFP8, and selection also runs before the storage is populated. - _check_configs's swiglu gate admits w4a8_mxfp4_mxfp8, and its SiTu branch now validates the merged storage. - The NVFP4 post-load swiglu_beta rescale (NVFP4TRTLLMGenFusedMoEMethod.process_weights_after_loading) is not in SiTu's MXFP4 quant-method ancestry. - _validate_backend_local_activation runs from __init__, before create_weights, so it still rejects constructor-provided SwiGLU parameters for SiTu. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
|
/bot run |
7c9a039 to
505865d
Compare
|
PR_Github #64863 [ run ] triggered by Bot. Commit: |
|
PR_Github #64863 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
5 participants
Summary
Consolidate the trtllm-gen SiTu activation slot handling, a review follow-up from #17269 tracked in TRTLLM-15177 (item 1.2).
Included
situ_alpha/situ_betaparameters and reuse theswiglu_alpha/swiglu_betastorage. SiTu and SwiGLU are mutually exclusive and feed the samegemm1_alpha/gemm1_betaop slots, so the slot now has a single backing parameter and the interim_gemm1_activation_paramsaccessor is gone.swiglu_alpha is not Nonegate for behavior neutrality:create_moe.pyvalidates the constructor kwargs (stillNonefor SiTu; the storage is populated later increate_weights);_get_quant_methodconsultsswiglu_alphaonly on the nvfp4 branch (SiTu requires W4A8_MXFP4_MXFP8) and runs before the storage is populated;_check_configs's swiglu gate admitsw4a8_mxfp4_mxfp8and its SiTu branch validates the merged storage; the NVFP4 post-loadswiglu_betarescale is not in SiTu's MXFP4 quant-method ancestry;_validate_backend_local_activationruns from__init__and still rejects constructor-provided SwiGLU parameters for SiTu._torch/utils.py, the deliberate decision to keepSiTuin the trtllm-gen-localActType_TrtllmGenrather than adding it to the sharedActivationType. The shared enum mirrors the cutlass enum incommon.hand drives cutlass kernels; SiTu exists only in the trtllm-gen batched-GEMM kernels, so adding it to the shared enum would force a cutlass member no kernel implements.Validation
Run on a Blackwell (SM100-class) node:
tests/unittest/_torch/modules/moe/test_kimi_k3_situ_moe.py— full suite pass, including fused-vs-reference SiTu parity and MoE-TP shard-loader / TP8 sharded-forward parity.tests/unittest/_torch/modules/moe/test_moe_backend.py -k trtllm— pass; covers the trtllm-gen SwiGLU leg including gptoss-style per-expertswiglu_alpha/swiglu_beta/swiglu_limiton W4A8_MXFP4_MXFP8, proving the merge did not regress the SwiGLU path.tests/unittest/_torch/modules/moe/test_moe_module.py -k "trtllm and not multi_gpu"— pass (single-GPU ConfigurableMoE coverage). The DEP multi-GPU comm parametrizations are not runnable in this single-node harness (they need pre-spawned MPI ranks) and are covered by CI.tests/unittest/_torch/modules/kimi_kda/andtests/unittest/_torch/modeling/test_kimi_kda_{fused_,}verify_parity.py— pass.No enum values changed, so the Python/cutlass and Python/
KernelRunner.hmappings are untouched.Related: TRTLLM-15177
Dev Engineer Review
swiglu_alphaandswiglu_betastorage._gemm1_activation_paramsaccess were removed.ActType_TrtllmGen.waives.txtadds the layer-wise benchmark waiver for the bug tracked in PR#17414.QA Engineer Review
test-db/orqa/files were modified.unittest/tools/test_layer_wise_benchmarks.py::test_performance_alignment[1]towaives.txt.pr-babysitter waive set-bug NVIDIA/TensorRT-LLM#17414 <bug-url>— the waives entry carries a placeholder until then