[https://nvbugs/6483369][fix] Keep the engineer-confirmed one-line stale-waiver removal, and add a… - #17438
[https://nvbugs/6483369][fix] Keep the engineer-confirmed one-line stale-waiver removal, and add a…#17438trtllm-agent wants to merge 2 commits into
Conversation
…cy] on B300 The MMLU drop (87.2 -> 61.866) came from the deferred MoEAllReduce finalize kernel accumulating the top-k weighted expert sum in bf16, rounding after every top_k term. Across DeepSeek-V3.2's routed MoE layers that rounding bias drifts the hidden states far enough to lose accuracy without any crash. That numerics bug was fixed upstream by 2801e94 ("[TRTLLM-14575][fix] MoE: fp32 accumulation in deferred MoEAllReduce finalize", NVIDIA#16778), which is already an ancestor of this branch, so no product-code change is needed here. The failing QA run used branch tip 5d8a786, which predates that commit. The assigned engineer reached the same conclusion on the bug ("already fixed by 2801e94... It is no longer reproducible in TOT now. V2C."). The [latency] config does reach the fixed kernel: modeling_deepseekv3.py selects the deferred finalize path (do_finalize=False) when POST_MOE_FUSION is on with moe_backend="TRTLLM", nvfp4 experts and tokens <= 128 -- all true for tp8/ep8, attention_dp=False, max_batch_size=1. Confirmed by a full run of this exact test on B300 (tp8/ep8, DeepSeek-V3.2-Exp-FP4-v2) at this branch's product tree: MMLU weighted average accuracy: 87.99 (4104) reference 87.200 -> PASS gsm8k average accuracy: 96.25 reference 95.600 -> PASS 1 passed, 2 warnings in 6281.09s (1:44:41) The 1:44:41 wall-clock is why this case cannot be validated inside a short CI timeout; it needs the full B300 QA budget, which unwaiving restores. Only the waiver line for this bug id is removed; the neighbouring B300 DeepSeekV32 entries belong to 6423845 and 6423866 and are left in place. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
The deferred MoEAllReduce finalize kernel used to accumulate the top-k weighted expert sum in bf16, rounding after every one of the top_k terms. That biased DeepSeek-V3.2 routed-MoE hidden states and dropped MMLU from 87.2 to 61.9 on B300 without any crash. It was fixed upstream by 2801e94, which accumulates into a local fp32 facc[] and rounds to DType once; that commit is an ancestor of HEAD but not of the QA tree 5d8a786 that reported this failure, which is why the test now passes. Nothing in CI pins that contract. The existing pattern tests exercise this exact kernel but assert at rtol/atol=0.2, which a bf16 accumulator satisfies just as easily as an fp32 one, so a regression would land silently and only surface as a multi-hour accuracy test on one platform. Add a test that compares the kernel against both candidate reference implementations -- fp32 accumulation rounded once, and DType accumulation rounded per term -- and asserts it is measurably closer to the fp32 one. The two references bracket the kernel, so the check calibrates itself instead of hardcoding a hardware-dependent error bound. Measured separation in relative RMS error is 2.4x at top_k=8 and 4.2x at top_k=32, against a required 1.5x margin; substituting a bf16 accumulator makes the assertion fire for every top_k/seed combination tried. The file is already collected wholesale by l0_dgx_h100.yml, so no test-list change is needed. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
|
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)
💤 Files with no reviewable changes (1)
WalkthroughThe pull request adds deferred-MoE accumulation-precision coverage for ChangesDeferred MoE precision validation
DeepSeek latency coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
The waiver removal checks out: 6483369 was bisected to the in-tree fp32-accumulation fix and confirmed non-reproducible at TOT, so this isn't a speculative unwaive. I also verified the new test's "reference B" against the pre-fix kernel source — the old code cast the fp32 product to bf16 and +='d in bf16, which is exactly what permuted[:, k] * scale[:, k] followed by a bf16 add reproduces. So the test really would have caught the regression, and the bracketing trick is a good answer to "an absolute tolerance here would be shape- and hardware-dependent".
On CI registration: no test-db change is needed. tests/integration/test_lists/test-db/l0_dgx_h100.yml:20 lists unittest/_torch/multi_gpu -m "not post_merge" as a directory, so the new test is collected automatically. Worth noting in the description that the coverage lands on the 2-GPU H100 stage rather than B300 — fine here since the kernel path is architecture-independent, but it's the kind of thing a future reader will ask.
Description hygiene: the PR title ("Keep the engineer-confirmed one-line stale-waiver removal, and add a…") describes the bot's decision process rather than the change. The branch's own commit subject — "[nvbugs/6483369][test] Pin fp32 accumulation in deferred MoE finalize" — is a much better title; please use it. It would also help to state up front that the kernel fix is already in main (2801e94) and this PR is unwaive + regression coverage only, since "Root cause / Fix" phrasing reads as if the kernel change is in here.
Test-only change, so no docs or release-note obligation.
| err_vs_fp32 = rel_rms_vs(acc_fp32) | ||
| err_vs_dtype = rel_rms_vs(acc_dtype) | ||
|
|
||
| # The two references differ by ~2.4x (top_k=8) up to ~8x (top_k=128) in |
There was a problem hiding this comment.
The comment quotes a ~2.4x reference separation at top_k=8 and ~8x at top_k=128, but the parameterization only runs 8 and 32. That leaves the weakest case (8) with ~1.6x headroom over the 1.5x threshold, and cites a data point for a shape the test never exercises.
Two concrete options: record the measured ratios for the values actually parameterized (8 and 32) so a future reader can judge the margin, or add a larger top_k to the parameter list if you want the 8x case covered. Either way, please drop the top_k=128 number from the comment unless it's in the list — it reads as if the test has more margin than it does.
| output_hidden_states, _ = moe_allreduce( | ||
| fc2_output, all_reduce_params=moe_all_reduce_params) | ||
|
|
||
| permuted = fc2_output[expanded_idx_to_permuted_idx] |
There was a problem hiding this comment.
The kernel skips slots where expanded_idx_to_permuted_idx == -1 (moeAllReduceFusionKernels.cu:582), but make_moe_finalize_inputs draws indices from [0, seq_len * top_k), so no test in this file ever reaches that branch — the pre-existing pattern tests have the same gap.
Note also that this fancy-index reference cannot simply be reused if sentinels are added: -1 wraps to the last row of fc2_output and silently contributes a bogus expert term rather than being skipped. A follow-up variant that plants a few -1s and zeroes the corresponding scale entries in the reference would close both holes.
| assert r is True | ||
|
|
||
|
|
||
| def make_moe_finalize_inputs(seq_len, hidden_size, top_k, dtype): |
There was a problem hiding this comment.
test_moe_finalize_allreduce_no_residual (line 796) still builds the same tensors inline with the same shapes. It can't reuse this helper verbatim (it doesn't draw residual, so the RNG sequence differs), but leaving one of the three call sites hand-rolled means the next shape change has to be made in two places. Consider a with_residual=False flag on the helper, or a comment noting why that one is intentionally separate.
3 participants
Summary
top_kterms, biasing DeepSeek-V3.2 routed-MoE hidden states (MMLU 87.2 → 61.866); already fixed in tree, but no test pinned the fp32-accumulation contract.pytest tests/integration/defs/accuracy/test_llm_api_pytorch.py::TestDeepSeekV32::test_nvfp4_multi_gpus_chunked_prefill[latency] -vTest plan
Links
Dev Engineer Review
6483369.top_kvalues 8 and 32.QA Engineer Review
test_moe_finalize_allreduce_accumulation_precision.make_moe_finalize_inputs.run_moe_finalize_allreduce_accumulation_precision_op.tests/integration/test_lists/waives.txt.test-db/orqa/.