[None][fix] Enable INT8 weight-only (W8A16) MoE for non-gated activations - #15550
Conversation
The INT8 weight-only per-channel MoE path assumed gated activations (Swiglu/Geglu) in three places, rejecting or mis-handling non-gated experts (squared-ReLU, e.g. Nemotron-H) that the underlying CUTLASS kernels already support: - moeOp.cpp: the woq validation hardcoded fc1.inter == 2 * fc2.inter; now conditioned on isGatedActivation(), mirroring the existing non-woq branch. - INT8WoqPerChannelFusedMoEMethod: buffer sizing, weight loading, and scale loading assumed the doubled (gate+up) layout; now handle the single up-projection when the gate weight is absent, mirroring the unquantized fused-MoE path's existing non-gated handling. Gated models are unaffected (they retain the original code path). Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
3c8ed84 to
b225aef
Compare
Covers the non-gated (squared-ReLU) path of INT8 weight-only per-channel fused MoE: the gate projection is absent, so the intermediate buffer is single-width (expand ratio 1). Complements the existing gated test and exercises the non-gated handling enabled by the preceding commit. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
eaefb2d to
3573e0e
Compare
📝 WalkthroughWalkthroughINT8 weight-only quantization per-channel MoE gains non-gated activation support. Python weight buffer and scale shapes are switched from a hardcoded ×2 intermediate size to ChangesINT8 WOQ Non-Gated MoE Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unittest/_torch/modules/test_fused_moe.py (1)
2981-3049: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage is still insufficient for the min-latency branch.
This test covers the regular
CutlassFusedMoE.forwardpath well, but this PR also changescpp/tensorrt_llm/thop/moeOp.cpp::runMoeMinLantency. Please add a companion case intests/unittest/_torch/modules/test_fused_moe.pythat enables min-latency on the sameActivationType.Relu2+QuantAlgo.W8A16setup, otherwise the new activation/layout validation there can regress unnoticed. As per path instructions,tests/**: “Act as a QA engineer reviewing test changes and coverage for TensorRT-LLM. Keep feedback actionable: suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR.”🤖 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/unittest/_torch/modules/test_fused_moe.py` around lines 2981 - 3049, Coverage is insufficient for the min-latency branch in runMoeMinLantency, so add a companion test alongside test_fused_moe_int8_woq_per_channel_non_gated in test_fused_moe.py that uses the same ActivationType.Relu2 and QuantAlgo.W8A16 setup but explicitly enables min-latency. Reuse the existing CutlassFusedMoE, QuantConfig, and RoutingMethod setup so the new case exercises the same non-gated INT8 weight-only path while validating the min-latency-specific activation/layout checks. Keep the assertions on output shape and finiteness so regressions in the new branch are caught.Source: Path instructions
🤖 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 827-858: `runMoeMinLantency()` now validates INT8-WOQ tensor
layout, but it still derives `hidden_size` and `inter_size` using the non-WOQ
index order before calling `getWorkspaceInfo(...)` and `runMoe(...)`. Update the
size selection logic in `runMoeMinLantency()` to branch on
`mUseINT8WoqPerChannel` and use the same fc1/fc2 dimension order as the new
validation block, so the downstream workspace and execution paths receive the
correct hidden/inter sizes for both WOQ and non-WOQ cases.
In `@tensorrt_llm/_torch/modules/fused_moe/quantization.py`:
- Around line 1358-1369: The gated MoE weight check in quantization.py is using
w3_weight.numel() before sharding, which breaks supported non-tensor checkpoint
entries handled by load_weight_shard(). Update the logic around the
w3_weight_shard/w1_weight_shard construction to first call load_weight_shard()
on w3_weight when present, then decide whether to concatenate based on the
sharded result’s emptiness; keep the non-gated fallback in the same fused-MoE
loading path.
---
Nitpick comments:
In `@tests/unittest/_torch/modules/test_fused_moe.py`:
- Around line 2981-3049: Coverage is insufficient for the min-latency branch in
runMoeMinLantency, so add a companion test alongside
test_fused_moe_int8_woq_per_channel_non_gated in test_fused_moe.py that uses the
same ActivationType.Relu2 and QuantAlgo.W8A16 setup but explicitly enables
min-latency. Reuse the existing CutlassFusedMoE, QuantConfig, and RoutingMethod
setup so the new case exercises the same non-gated INT8 weight-only path while
validating the min-latency-specific activation/layout checks. Keep the
assertions on output shape and finiteness so regressions in the new branch are
caught.
🪄 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: 1325b4ba-1aba-4627-83d5-d753fb661b91
📒 Files selected for processing (3)
cpp/tensorrt_llm/thop/moeOp.cpptensorrt_llm/_torch/modules/fused_moe/quantization.pytests/unittest/_torch/modules/test_fused_moe.py
runMoeMinLantency derived hidden_size/inter_size from the non-woq weight layout and fed them to getWorkspaceInfo/runMoe without the INT8 weight-only per-channel swap that runMoe and runGemmProfile already apply. Add the same swap so all three paths use consistent dimensions for INT8 woq per-channel. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
|
One note on test placement: I added test_fused_moe_int8_woq_per_channel_non_gated to tests/unittest/_torch/modules/test_fused_moe.py, mirroring the existing test_fused_moe_int8_woq_per_channel. I see that file is now marked deprecated in favor of the unified framework in tests/unittest/_torch/modules/moe/test_moe_backend.py. The unified framework is currently gated-only for W8A16. W8A16QuantizeUtil.create_weights always builds a gate (w3) projection and W8A16RefGatedMLPFusedMoE asserts Swiglu; so adding non-gated W8A16 coverage there needs a non-gated branch in the quantize util plus a non-gated reference module, analogous to the W4A8_MXFP4_FP8 unit-test support in #13401. Happy to extend the unified framework that way as a follow-up if you would prefer the coverage there rather than in test_fused_moe.py. On the min-latency test nitpick: min_latency_mode is hardcoded to False at all CutlassFusedMoE runner call sites and is not exposed through the module's forward, so a companion test reusing the existing CutlassFusedMoE setup cannot drive runMoeMinLantency, it always routes through runMoe. Exercising it would require the low-level run_moe_min_latency op with hand-constructed WOQ weights/scales/workspace, a lower layer than this test file operates at. The min-latency fix mirrors the INT8-woq size swap already present in runMoe and runGemmProfile, so the three paths are consistent by inspection; I have noted min-latency coverage as part of the same follow-up above. |
…framework Port the non-gated (squared-ReLU) INT8 weight-only per-channel MoE coverage from the deprecated test_fused_moe.py smoke test into the unified moe/test_moe_backend.py framework, which validates against a dequantized reference rather than only output shape and finiteness. - quantize_utils.py: W8A16QuantizeUtil.create_weights gains a non-gated branch (empty w3); W8A16RefGatedMLPFusedMoE forwards activation_type to the base reference and loads the single up-projection when non-gated, mirroring the existing NVFP4 element-wise support. - test_moe_backend.py: add W8A16 to the element-wise parameter set, guarded to the CUTLASS backend (the path this PR fixes). - Remove the superseded non-gated smoke test from test_fused_moe.py. Validated on A100: 20 non-gated + 20 gated W8A16 cases pass (float16 + bfloat16, CUTLASS). Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
|
Moved the coverage into the unified framework as suggested.
Validated on A100: 20 non-gated + 20 gated W8A16 cases pass (float16 + bfloat16, CUTLASS), non-gated added, gated unchanged. Pushed in 2d71065. |
leslie-fang25
left a comment
There was a problem hiding this comment.
LGTM, thanks for the PR
|
Thanks again @leslie-fang25 for the review and approval. It looks like the only thing left before merge is a maintainer approving the pending workflows. blossom-ci is showing "waiting for status to be reported." Would you be able to trigger that, or point me to the right person? The branch is slightly behind main but merges cleanly, and I would be happy to update it or address anything CI surfaces. |
|
While validating this at tensor-parallel sizes > 1, I found and fixed a related pre-existing bug in the same method (missing 64-alignment padding of the sharded intermediate). Made a follow-up in #16198, stacked on this branch, with a regression test. Note that main has moved substantially in moeOp.cpp since approval, however this branch still merges cleanly today. @leslie-fang25, anything needed from my side to get blossom-ci triggered here? |
|
/bot run |
|
@Dorijan10 sorry for the late response, I have triggered the CI. But I am afraid I am also don't have the full access to run all the cases now. Let's see what's the result for now. |
|
PR_Github #61457 [ run ] triggered by Bot. Commit: |
|
PR_Github #61457 [ run ] completed with state
|
|
PR_Github #61770 [ run ] completed with state
|
|
Thanks @leslie-fang25 for triggering the CI again. This time all other checks succeeded apart from the blossom-ci so I would really need a way to see which tests failed and why. I have few ideas, but seeing the CI report would make it clear for me. @xxi-nv making me a contributor should help here I think. |
|
@leslie-fang25 @xxi-nv I think I automatically become a Contributor once the PR gets merged. However, that means I still cannot get access right now to the CI report from the blossom-ci run. I think for that to happen I would need to be assigned as Collaborator. It would be very useful if I could see the CI run results and fix the issues. |
|
Here is the summary of the CI failures Based on the first suggestions, I have rebased this PR and will trigger the CI again with the latest codebase. |
|
/bot run |
|
PR_Github #62648 [ run ] triggered by Bot. Commit: |
|
PR_Github #62648 [ run ] completed with state
|
|
Hi @leslie-fang25 thanks for the triggering CI and rebase. I have checked ba0c487 locally and it preserves the changes exactly, so nothing changed here on my side. And all actions checks are green. What I can see from the first CI report you sent is this: RC-1: Test container/JIT cache path misconfigured (/root/.triton/cache missing) I think that when TensorRT runs during CI, it compiles some GPU kernels on the fly and caches them to disk. The test containers were pointed at /root/.triton/cache, from the looks of it a directory that did not exist or was not writable. So every test that needed to compile a kernel died with FileNotFoundError. Which would make sense as to why L0 Pipeline would fail then. I think this needs a fix on the CI environment side rather than a change in the PR. So this was in the CI report from #49971 CI run, can you send or check if the same error appeared in the #50788? I assume it did since all that changed between the two runs is the merge and that turned out to be fine! |
|
/bot run --disable-fail-fast |
|
some failures looks like Irrelevant, will trigger a rerun for it. |
|
PR_Github #62898 [ run ] triggered by Bot. Commit: |
|
PR_Github #62898 [ run ] completed with state
|
|
Thanks for triggering CI again @leslie-fang25. Could you share the report for #51021? Since it had fail fast disabled it should be the complete picture, and it would be much more useful than the older report. I think the problem might still be the container environment rather than the code, with the JIT cache paths under /root not being writeable. On my side the merge preserved the change exactly, and W8A16 is listed as SM >= 80 in CutlassFusedMoE.can_implement, which matches it running fine on SM120 in my own validation. So I do not see an arch gap on Blackwell. |
|
Here is the report, seems unrelated failures with this PR, will take a re-try. |
|
/bot run --disable-fail-fast |
|
PR_Github #63323 [ run ] triggered by Bot. Commit: |
|
PR_Github #63323 [ run ] completed with state
|
|
Thanks for the report @leslie-fang25. Agreed the failures look unrelated. On the B200 side, two of the fifteen TestKVCacheV2Llama tests scheduled in l0_b200.yml are already waived in main, test_chunked_prefill under nvbugs/6428002 and test_eviction_with_block_reuse under nvbugs/6462303. The one failing here, test_chunked_prefill_with_eviction, exercises both areas: it enables chunked prefill and uses the same _run_eviction_test helper as the second one. It also hit the 3600s timeout rather than failing an assertion, so it looks like a hang in an area with known issues rather than anything new. The GB200 failure is a UCX endpoint resolution error on a different rack from the previous run, so that looks like general multi-node instability. Would it make sense to maybe waive that KV-cache test alongside the others? I could also maybe merge latest main if that helps, since a fair number of waivers have landed since the last merge, according to my checks. Please let me know! |
|
/bot run --disable-fail-fast |
|
PR_Github #63419 [ run ] triggered by Bot. Commit: |
|
PR_Github #63419 [ run ] completed with state |
|
Excellent @leslie-fang25, all of the checks have passed! Seems like this PR is ready for merge, let me know if you guys are ready to proceed! |
|
@leslie-fang25 @xxi-nv @rosong11 and other reviewers. Thanks again for running the CI tests on this PR throughout. All checks are now green and this PR should be ready for clean merge. It would be great if this is completed so that we can continue the work on the W8A16 path in the PR #16198 I raised as well. It is stacked on this one so it is important #15550 gets merged first. Many thanks! |
…sed MoE Adds INT4 weight-only per-channel (W4A16) support to the Cutlass fused MoE path, which previously failed with "Unsupported weight only quantization" for any non-INT8 per-channel quantization. Changes: - Generalise the per-channel flag from use_int8_woq_per_channel to use_woq_per_channel across the Python bindings and moeOp.cpp so INT4 reaches the runner. - Extend the non-gated inter-size handling added in NVIDIA#15550 to INT4: under the dim-swapped per-channel layout the sub-byte packing sits on fc1's inter dim, so mInnerDimMultiplier must multiply the fc1 side. The previous form is equivalent at INT8 (multiplier 1) but rejects every valid INT4 shape. Applied in both runMoe and runMoeMinLantency. - Add W4A16WoqPerChannelFusedMoEMethod with packed weight creation, per-output-channel scales, TP-aware loaders and a 64-row alignment diagnostic. - View expert weights as torch.quint4x2 at both op call sites. Without this, isInt8Quant() matched and the runner was instantiated at the wrong element width, producing incorrect output rather than an error. - Fix the meta kernel, which reported half the hidden size for packed INT4. - Add W4A16 to the parametrized MoE backend tests with a dequantized reference at the existing 0.99/0.96 tolerances. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
… in module tests Mirror the non-gated harness handling that NVIDIA#15550 added for W8A16: W4A16QuantizeUtil emits empty w3 tensors when the activation is not gated, W4A16RefGatedMLPFusedMoE accepts Relu2/Silu and loads a single up-projection, and QuantAlgo.W4A16 joins the element-wise (Relu2) sweep so non-gated W4A16 runs against the dequantized reference on the CUTLASS path. Add QuantAlgo.W4A16 to test_moe_module's QUANT_ALGOS so test_configurable_moe_single_gpu exercises the method under the existing "CUTLASS and not None" pre-merge entries, and register the three standalone W4A16 tests in l0_b200 and l0_h100. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
…ings Replace file:line references in the W4A16 comments with symbol names. Several had rotted or were never right: tensorrt_llm/quantization/functional.py is 184 lines but was cited at :1020, interface.py at :1005-1007, and the moeOp.cpp references were several hundred lines off. Symbol names do not rot. Drop an incorrect claim that INT8WoqPerChannelFusedMoEMethod hardcodes intermediate_size_per_partition * 2; it has used expand_intermediate_size_per_partition since NVIDIA#15550. Make the alignment diagnostic accurate for both tensors: the 64-row constraint falls on the per-partition intermediate size for w2_weight, but on the hidden size for w3_w1_weight, which tensor parallelism does not change. Add docstrings to the new methods and properties. Signed-off-by: Dorijan10 <dorian.magasic@turintech.ai>
Description
The INT8 weight-only per-channel (W8A16) fused-MoE path assumes gated activations (SwiGLU/GeGLU), which prevents non-gated MoE models (squared-ReLU, e.g. Nemotron) from using it; even though the underlying CUTLASS mixed-GEMM kernels already support the non-gated layout. Non-gated experts are already handled for FP8 and NVFP4 (see the Nemotron weight mapper and the FP8 fused-MoE method); this PR brings the INT8 weight-only path to parity.
cpp/tensorrt_llm/thop/moeOp.cpp: the woq validation hardcodedfc1.inter == 2 * fc2.inter; now conditioned onisGatedActivation(), mirroring the existing non-woq branch.tensorrt_llm/_torch/modules/fused_moe/quantization.py(INT8WoqPerChannelFusedMoEMethod): buffer sizing, weight loading, and scale loading assumed the doubled (gate+up) layout; now handle the single up-projection when the gate weight is absent, mirroring the unquantized fused-MoE path's existing non-gated handling.Gated models are unaffected; they retain the original code path. W8A16 checkpoints are produced the standard way (ModelOpt
int8_wo, or offline per-output-channel quantization); no new runtime quantizer is introduced. On Ampere (e.g. A100), where FP8/NVFP4 are unavailable, INT8 weight-only is the natural weight-compression format for MoE inference.Test Coverage
Validated on NVIDIA-Nemotron-3-Nano-30B-A3B (non-gated, Relu² experts) on A100-SXM4-80GB, served via
trtllm-servefrom a pre-quantized W8A16 checkpoint (hf_quant_config.json→quant_algo: W8A16, non-expert linears excluded). Built and validated against currentmainwith only the two files in this PR.Throughput:
vllm bench serve(random, ISL/OSL 512/512, concurrency 32,--ignore-eos, mean of 3):Lossless within eval noise on gsm8k.
Validated end-to-end on Nemotron. Includes a unit test (test_fused_moe_int8_woq_per_channel_non_gated) in tests/unittest/_torch/modules/test_fused_moe.py covering the non-gated INT8 weight-only per-channel MoE path (single-width buffer, absent gate projection), for float16 and bfloat16.
Summary by CodeRabbit
Bug Fixes
Tests