Repository navigation
[None][fix] Pad INT8-woq MoE intermediate to 64 for tensor-parallel sharding - #16198
Conversation
4db3246 to
eb79f4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tensorrt_llm/_torch/modules/fused_moe/quantization.py (1)
1444-1446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new scale-padding helper.
_pad_scaleis newly defined without parameter or return annotations. Usetorch.Tensor -> torch.Tensor.Proposed fix
- def _pad_scale(t): + def _pad_scale(t: torch.Tensor) -> torch.Tensor:🤖 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 `@tensorrt_llm/_torch/modules/fused_moe/quantization.py` around lines 1444 - 1446, Update the nested _pad_scale helper to annotate its parameter and return value as torch.Tensor, preserving its existing padding behavior.Source: Coding guidelines
cpp/tensorrt_llm/thop/moeOp.cpp (1)
483-483: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace tabs with spaces.
These changed lines contain tab indentation, contrary to the required four-space style.
Also applies to: 834-834
🤖 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 `@cpp/tensorrt_llm/thop/moeOp.cpp` at line 483, Replace the tab indentation in the affected comment lines near the non-WOQ activation branches with the required four-space indentation, including the additional occurrence, without changing the comment text or surrounding logic.Source: Coding guidelines
🤖 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 `@cpp/tensorrt_llm/thop/moeOp.cpp`:
- Around line 795-801: Move the defaulting of unpadded_hidden_size_val in
cpp/tensorrt_llm/thop/moeOp.cpp around lines 795-801 until after the INT8-WOQ
dimension swap, using the resolved hidden_size. Add a min-latency regression in
tests/unittest/_torch/modules/moe/test_moe_backend.py around lines 489-546 where
hidden size differs from padded intermediate size and unpadded_hidden_size is
omitted; retain coverage of the existing case.
- Around line 483-495: Move the fc1/fc2 intermediate-dimension validation in the
relevant MoE setup flow to after swiglu_alpha, swiglu_beta, and swiglu_limit
finalize base_activation_type, matching runMoeMinLantency. Ensure
isGatedActivation(base_activation_type) evaluates the finalized activation so
SwigluBias uses the gated shape rule.
---
Nitpick comments:
In `@cpp/tensorrt_llm/thop/moeOp.cpp`:
- Line 483: Replace the tab indentation in the affected comment lines near the
non-WOQ activation branches with the required four-space indentation, including
the additional occurrence, without changing the comment text or surrounding
logic.
In `@tensorrt_llm/_torch/modules/fused_moe/quantization.py`:
- Around line 1444-1446: Update the nested _pad_scale helper to annotate its
parameter and return value as torch.Tensor, preserving its existing padding
behavior.
🪄 Autofix (Beta)
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: 46f7b912-8649-4ef1-bfe3-58ad266624da
📒 Files selected for processing (4)
cpp/tensorrt_llm/thop/moeOp.cpptensorrt_llm/_torch/modules/fused_moe/quantization.pytests/unittest/_torch/modules/moe/quantize_utils.pytests/unittest/_torch/modules/moe/test_moe_backend.py
INT8WoqPerChannelFusedMoEMethod did not pad the per-partition
intermediate dimension after tensor-parallel sharding, so any model
whose moe_intermediate_size / tp_size is not a multiple of 64 failed to
load at TP>1. The INT8 mixed-GEMM preprocessor
(preprocess_weights_for_mixed_gemm) requires 64-aligned rows
(rows_per_tile = 128 * 8 // BITS_PER_ELT_A, BITS_PER_ELT_A = 16 for
W8A16). The FP8/NVFP4/MXFP4/WFP4A16 fused-MoE methods round the sharded
intermediate up and F.pad; the INT8-woq method did not.
Mirror that pattern: round the per-partition intermediate up to a
multiple of 64 in create_weights and F.pad each shard up to it in both
weight loaders and the fc31 scales. Zero-padding is numerically inert
(Relu2(0) = 0 for squared-ReLU, silu(0) * 0 = 0 for gated), so lossless.
Reproduces on Nemotron-3-Nano-30B-A3B (moe_intermediate_size = 1856 =
64 * 29) at TP={2,4,8}. Validated on consumer Blackwell (sm_120) at
TP=8: native W8A16 checkpoint loads and serves; GSM8K 5-shot
strict-match 0.8385 vs BF16 0.8393 (within stderr); also TP=2/4.
Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
The mixed-GEMM preprocessor requires the packed row count to be a multiple of 64 for W8A16 (rows_per_tile = 128 * 8 // BITS_PER_ELT_A) and of B_ROWS_PER_MMA = 16. Every intermediate_size in MOE_MODEL_CONFIGS is already a multiple of 64, so the path that pads a non-aligned per-partition intermediate had no coverage. Add a W8A16 CUTLASS-only parameter set with intermediate_size = 200 (a multiple of neither 16 nor 64, rounding up to 256), covering both the non-gated (squared-ReLU) and gated (SwiGLU) layouts; the gated case also exercises padding of the w3 gate projection before concatenation. The 128-alignment gate in get_quick_skip_reason() is bypassed for this case only, since that gate is precisely what the padding makes unnecessary; all other skip reasons are honoured. Fails without the padding fix (AssertionError in preprocess_weights_for_mixed_gemm at num_rows % B_ROWS_PER_MMA == 0) and passes with it, validated against the dequantized reference on A100 for float16 and bfloat16. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
The fc1/fc2 intermediate-size validation in runMoe ran before the swiglu_alpha/beta/limit handling, which can promote base_activation_type to SwigluBias (a gated activation). A caller supplying those parameters with an initially non-gated activation was therefore validated under the non-gated shape rule while the kernel ran gated. Move the validation below the activation-parameter handling so it sees the finalized activation type, matching the ordering already used in runMoeMinLatency. This also places it after the INT8-woq hidden/inter size swap, as in runMoeMinLatency. Also drops a stray tab left in runMoeMinLatency by an earlier commit in this stack. Addresses review feedback on the gated/non-gated validation introduced earlier in this stack. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
unpadded_hidden_size_val was initialized from fc2_expert_weights.sizes()[1] before the INT8 weight-only per-channel path reinterprets fc2 as [num_experts, inter_size, hidden_size]. When the optional argument is absent, the output buffer was therefore sized from the intermediate dimension rather than the hidden dimension. Compute the default after the swap in both runMoe and runMoeMinLatency. Not reachable through the current call sites, since CutlassFusedMoE always passes unpadded_hidden_size, but the defaulting path was wrong for a direct caller that omits it. Also widen the non-aligned MoE test config to hidden_size=512 with intermediate_size=200, so the hidden size and the padded intermediate (256) are distinct and a confusion between them is not masked. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
5c4edb4 to
70f5d9b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/unittest/_torch/modules/moe/test_moe_backend.py`:
- Around line 921-924: Update the W8A16 coverage guard before returning params
to track the activation types represented in the generated cases and raise
ValueError unless both Relu2 and Swiglu are present; replace the current assert
params check while preserving the existing empty-case protection.
- Around line 866-897: Add multi-GPU configurations to
W8A16_NON_ALIGNED_MOE_MODEL_CONFIGS and parameter generation so test_moe_backend
exercises post-sharding intermediate sizes: TP2 with 100→128, TP4 with 50→64,
and TP8 with 25→64, while preserving the existing single-GPU 200→256 case.
Ensure the generated cases provide the corresponding world_size, tp_size, and
moe_tp_size values through the existing test configuration flow.
🪄 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: 13a8ef67-097f-4482-8ee8-240c3c722dfa
📒 Files selected for processing (3)
cpp/tensorrt_llm/thop/moeOp.cpptensorrt_llm/_torch/modules/fused_moe/quantization.pytests/unittest/_torch/modules/moe/test_moe_backend.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tensorrt_llm/_torch/modules/fused_moe/quantization.py
- cpp/tensorrt_llm/thop/moeOp.cpp
…6 guard The previous assert only proved the parameter list was non-empty, so a reworded upstream skip message could drop one activation family while the other kept it populated. Track the generated activation types and raise ValueError unless both Relu2 and Swiglu are present. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
|
@leslie-fang25 thanks for merging #15550. This PR stacks on that one. All the GitHub checks are green and both CodeRabbit review threads are addressed. Could you please trigger CI when convenient? |
|
/bot run --disable-fail-fast |
|
PR_Github #67212 [ run ] triggered by Bot. Commit: |
|
PR_Github #67212 [ run ] completed with state
|
|
Thanks for the approval and the CI run @leslie-fang25 . I cannot open the CI report or the failure analysis from outside. Could you paste the failed test list from #54739? I have checked the obvious candidates on my side and found nothing, so the main thing I need to know is whether the failures sit in the MoE tests or somewhere unrelated. The failure analysis page would be ideal. |
|
Here is the failure analysis page |
|
/bot run --disable-fail-fast |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
PR_Github #68054 [ run ] triggered by Bot. Commit: |
|
PR_Github #68054 [ run ] completed with state
|
|
PR_Github #77931 [ run ] triggered by Bot. Commit: |
|
PR_Github #77931 [ run ] completed with state
|
|
@leslie-fang25 Thanks for triggering the CI, can you please send across the CI report from #64235, so we can triage the failures. |
|
There is a real failure, here is the CI failure analysis |
…ners Runners without a GPU reject W8A16 CUTLASS in can_implement, so the generator yields no cases and the guard raised during collection. Raise only on SM80+ runners, where an empty list means the alignment skip message changed upstream. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
|
Thanks for the report @leslie-fang25. RC-1 was mine: CPU runners reject W8A16 in RC-2 ( |
|
/bot run --disable-fail-fast |
|
PR_Github #78835 [ run ] triggered by Bot. Commit: |
|
PR_Github #78835 [ run ] completed with state
|
…-padding Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
|
@leslie-fang25 I merged current main (3c93414). Our branch still had the old |
|
/bot run --disable-fail-fast |
|
PR_Github #79053 [ run ] triggered by Bot. Commit: |
|
PR_Github #79053 [ run ] completed with state
|
|
Thanks for running it again @leslie-fang25, can you please share the CI report? Especially if there are new failures. |
|
Here is the CI analysis, the failures should be unrelated. Could you resolve the conflict and I will trigger the CI again? |
…-padding Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai> # Conflicts: # cpp/tensorrt_llm/thop/moe/cutlass/moeOp.cpp
|
Thanks for the report @leslie-fang25. I resolved the conflict with main in |
|
/bot run --disable-fail-fast |
|
PR_Github #79707 [ run ] triggered by Bot. Commit: |
|
PR_Github #79707 [ run ] completed with state
|
|
Thanks for triggering @leslie-fang25. #65624 failed again, can you send across the CI report? |
|
/bot run --disable-fail-fast |
|
The failure seems unrelated, I have triggered to rerun these tests again |
|
PR_Github #79987 [ run ] triggered by Bot. Commit: |
|
PR_Github #79987 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Multi-GPU #2913: B200 multi-GPU stages are paused to relieve the CI queue backlog (109 running / 459 pending on 2026-10-22). Owner: @sharabeshj-nvda. The CI team decides when to lift it. |
|
Thanks @leslie-fang25 for re-running. CI passed on ccf446e! Could one of you merge it when convenient? |
Move the W4A16 wiring onto the cutlass/ leaf package as cutlass/w4a16.py, rename FusedMoeOpFlags.use_int8_woq_per_channel to use_woq_per_channel, and keep the INT8 padding from NVIDIA#16198 in WoqPerChannelFusedMoEMethodBase, gated to INT8. Signed-off-by: Dorian Magasic <dorian.magasic@turintech.ai>
#15550 has merged, so this now stands alone on main: the padding fix, non-aligned-shape test coverage, and a C++ change addressing a CodeRabbit finding from that review. The other finding, defaulting
unpadded_hidden_sizeafter the INT8-woq dim swap, is now on main via #19753.Problem
INT8WoqPerChannelFusedMoEMethoddoes not pad its per-partition intermediate dimension after tensor-parallel sharding, so any model whosemoe_intermediate_size / tp_sizeis not a multiple of 64 fails to load at TP > 1. The INT8 mixed-GEMM preprocessor (preprocess_weights_for_mixed_gemm) requires 64-aligned rows (rows_per_tile = 128 * 8 // BITS_PER_ELT_A,BITS_PER_ELT_A = 16for W8A16). The FP8, NVFP4, MXFP4 and WFP4A16 fused-MoE methods all round the sharded intermediate up andF.pad; INT8-woq was the only one that did not.This limitation is already noted in-tree:
should_skip_cutlassintests/unittest/_torch/moe/moe_test_utils.pyrecords that W8A16 "fails in preprocess_weights_for_mixed_gemm (num_rows % rows_per_tile != 0)" and skips the affected TP configurations rather than exercising them.Reproduction
nvidia/NVIDIA-Nemotron-3-Nano-30B-A3Bhasmoe_intermediate_size = 1856 = 64 × 29. Since 29 is prime,1856 / tpis a multiple of 64 only fortp ∈ {1, 29}; every power-of-2 TP leaves a non-aligned shard (1856/8 = 232, not ÷64). The native W8A16 checkpoint at--tensor_parallel_size {2,4,8}fails during weight load:TP = 1 loads (1856 is already 64-aligned) but the checkpoint does not fit a single 32 GB card, so TP > 1 is required in practice.
This is not specific to one checkpoint. Across the current Nemotron 3 MoE lineup, Nano, Lightning, Elastic, TwoTower and Cascade-2 all have
moe_intermediate_size = 1856and are misaligned at every TP ≥ 2, and Super (2688 = 64 × 42) is misaligned at TP ≥ 4. Ultra (5120 = 64 × 80) is aligned at every TP ≤ 8 and pays no padding cost, which also shows the change is a no-op wherever alignment already holds.Fix
Round the per-partition intermediate up to a multiple of 64 in
create_weights, andF.padeach shard up to it in both weight loaders and the fc31 scales. Zero-padding is numerically inert (Relu2(0) = 0for squared-ReLU,silu(0)·0 = 0for gated), meaning it is lossless.Test Coverage
Adds a W8A16 CUTLASS parameter set with
intermediate_size = 200(a multiple of neither 16 nor 64, rounding up to 256), covering both the non-gated (squared-ReLU) and gated (SwiGLU) layouts; the gated case additionally exercises padding of thew3gate projection before concatenation. Everyintermediate_sizeinMOE_MODEL_CONFIGSis already a multiple of 64, so this path previously had no coverage. Without the fix all eight cases fail with AssertionError in preprocess_weights_for_mixed_gemm (num_rows % B_ROWS_PER_MMA == 0 in tensorrt_llm/quantization/functional.py); with the fix all eight pass against the dequantized reference on A100 for float16 and bfloat16.Note that the new parameter set bypasses the 128-alignment gate in
get_quick_skip_reason()for this case only, since that gate is precisely what the padding makes unnecessary for W8A16; all other skip reasons are honoured. Happy to instead relax that gate for W8A16 generally if maintainers prefer.On SM80+ runners the generator raises ValueError if it produces no cases, so an upstream rewording of the skip message cannot silently drop them. CPU-only runners collect none.
Validation
Consumer Blackwell (sm_120), TP = 8: native W8A16 checkpoint loads and serves; GSM8K 5-shot strict-match 0.8385 ± 0.0101 vs BF16 0.8393 ± 0.0101 (Δ 0.0008, within stderr). Also loads and serves coherently at TP = 2 and TP = 4. All 40 existing W8A16 cases in
test_moe_backend.pypass on A100 (gated and non-gated, float16 and bfloat16, CUTLASS). The branch merges cleanly against currentmain.Dev Engineer Review
QA Engineer Review
Relu2MoE with intermediate size 200.SwiGLUMoE with intermediate size 200.tests/integration/test_lists/were modified.Verdict: needs follow-up.