[https://nvbugs/6412108][fix] Restore original order — all_reduce the routed partial first, then add the… - #15922
Conversation
|
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)
📝 WalkthroughWalkthroughThe reduction order in Qwen3_5MoeSparseMoeBlock.forward is changed so the routed expert output is all-reduced before adding the shared expert output, rather than summing first then reducing. A test waiver entry for a Qwen3.5 MoE NVFP4 accuracy test is removed. ChangesMoE reduction fix and waiver update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
da126a4 to
285807f
Compare
|
/bot run |
|
PR_Github #58496 [ run ] triggered by Bot. Commit: |
|
PR_Github #58496 [ run ] completed with state
|
285807f to
b656654
Compare
1db06b6 to
93d7a64
Compare
…replicated shared expert The shared expert (Qwen3_5MoeMLP) intentionally omits the layer_type hint on its torch_linear_simple ops and the qwen3.5_moe_400b.yaml shard_layers whitelist excludes it, so its output is already the full value on every rank. The previous single-merge-point ordering (add then all_reduce) scaled the replicated shared output by world_size and dropped MMLU from ~85% to ~0.07%. Restore the original order: all_reduce the routed partial first, then add the replicated shared output. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
93d7a64 to
17322b1
Compare
brnguyen2
left a comment
There was a problem hiding this comment.
The reorder is right for the qwen3.5_moe_35b.yaml path (manual tp_plan with the shared_expert_* entries commented out → shared expert replicated), but it is wrong for the default apply_sharding_hints path, where the model's own hints do shard the shared expert. See the inline comment. Also, the description says a nvbugs/6412108 waiver line was removed — no test-list change is in this diff.
| # One reduction on the sum lifts both to full; reducing before the add | ||
| # would mix a full routed contribution with an unreduced shared one. | ||
| expert_output = expert_output + shared_expert_output | ||
| # The shared expert is replicated (Qwen3_5MoeMLP intentionally omits |
There was a problem hiding this comment.
The premise here is wrong for the default pipeline. Qwen3_5MoeMLP does not omit layer_type — lines 643-662 tag all three projections with layer_type="mlp" and tp_mode="colwise"/"colwise"/"rowwise", and its own docstring comment at line 638 still says the opposite of this one ("Tagged layer_type=\"mlp\" so the shared expert is TP-sharded ... lifted to full by the single merge-point all_reduce").
apply_sharding_hints is the default sharding transform (config/default.yaml:134) and applies no shard_layers filter unless a config sets one, so on that path the shared expert's rowwise down_proj emits a per-rank partial. With the new order that partial is added after the only all_reduce and is never reduced — silently 1/world_size of the shared contribution on TP>1, no error. The 8x symptom you fixed comes from qwen3.5_moe_35b.yaml, which disables the hint path (sharding_source: ['manual'], shared_expert entries commented out) and therefore replicates it.
Pick one and make the model self-consistent: either drop the tp_mode/layer_type hints from Qwen3_5MoeMLP so the shared expert is genuinely replicated under both pipelines (then this order is correct and line 638's comment must go), or keep all_reduce(routed + shared) and make the 35b config shard the shared expert. As written, correctness depends on which YAML is loaded.
|
NVBug 6412108 is closed as Bug - Fixed. The linked bug appears resolved elsewhere or for a reason that does not prove this PR is redundant. This PR should be judged on its own merits; repair-bot is not auto-closing it. |
Summary
expert_output + shared_expert_outputandall_reduce, scaling the replicated (unsharded) shared_expert output by world_size (8×) and corrupting MMLU/GSM8K outputs.all_reducethe routed partial first, then add the replicated shared output — and update the comment; also removed the nvbugs/6412108 waiver line.Test plan
Links
Summary by CodeRabbit
Bug Fixes
Tests