Repository navigation
[AMD] fix(moe): correct fused shared-expert scaling on aiter/DeepEP path (mori all-to-all) - #28237
Conversation
On the HIP aiter + DeepEP-class (e.g. MoRI) path, routed_scaling_factor is folded into each routed topk weight by aiter_biased_grouped_topk, and DeepseekV2MoE.forward_deepep skips the post-MoE `*= routed_scaling_factor` multiply for _use_aiter. But _remap_topk_for_deepep unconditionally set the fused shared-expert weight to 1/routed_scaling_factor, compensating for a post-multiply that never happens on this path. The always-on shared expert was therefore under-weighted by routed_scaling_factor, corrupting every MoE layer and producing degenerate, non-converging generation. Set the shared weight to 1.0 on the aiter path; keep 1/routed_scaling_factor for the default post-MoE-scaled path. The fix is deliberately scoped to the aiter path (the one validated on AMD MI355X). forward_deepep also skips the post-multiply for the non-aiter families that pre-fold routed_scaling_factor in topk (should_fuse_routed_scaling_factor_in_topk: ModelOpt NVFP4, cutlass/trtllm fp8), so those would likewise need a 1.0 shared weight, but they are left at their existing behavior for their maintainers to address. Add unit tests pinning the shared-weight contract for the aiter and post-MoE-scaled paths plus the shared-id home-rank placement. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Rita Brugarolas Brufau <rita.brugarolasbrufau@amd.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
…pert-scaling-aiter-deepep
|
/tag-and-rerun-ci |
|
@amd-bot ci-status |
CI Status for PR #28237Merge verdict: ❌ Do not merge yet. This PR's own new unit test ( Caution The fix in Changed files: Executed CI failure attribution: AMD: 2 failures (1 possibly related) · Others: 4 failures (1 related) · plus 1 AMD job still running ( AMD Executed Failures
Other Executed Failures
Details / what to do before merge
Generated by amd-bot using Claude Code CLI |
|
@rbrugaro-amd please check the issues from |
…helpers The new test mocked topk.get_moe_expert_parallel_world_size / get_moe_expert_parallel_rank, but the production code was refactored to read get_parallel().moe_ep_size / .moe_ep_rank (runtime_context). patch.object raised AttributeError on the missing names, so the test errored before any assertion and CI never verified the fix. Patch topk.get_parallel to return a SimpleNamespace exposing moe_ep_size / moe_ep_rank instead. Verified: all 3 tests pass against the PR code in a torch 2.9.1 / ROCm 7.2 environment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@HaiShaw I fixed the test raised as blocker above. if num_fused_shared_experts > 0 and is_deepep_class_backend(): The failing test TestDeepseekR1MXFP4MTP launches with only --tp 8 + EAGLE spec args (test_deepseek_r1_mxfp4_8gpu.py#L106-L120) — it never sets --moe-a2a-backend, which defaults to "none" (server_args.py#L998-L1006). → is_deepep_class_backend() is False in that test → _remap_topk_for_deepep is never invoked → the PR cannot change its output. The avg_spec_accept_length=1.447 < 2.04 failure (assertion here) is pre-existing / flaky / infra, not a regression from this PR. |
|
@amd-bot ci-status |
CI Status for PR #28237Merge verdict: No PR-CI failure is caused by this PR's changes. The new unit test ( Warning The PR's real behavioral change ( Changed files: Executed CI failure attribution: AMD: 1 executed failure (0 related, 1 possibly) · Others: ~7 root-cause executed failures (0 related) + many fast-fail cascades AMD Executed Failures
( Other Executed Failures
All Details / what to do before merge
Generated by amd-bot using Claude Code CLI |
|
|
||
| from sglang.test.ci.ci_register import register_cpu_ci | ||
|
|
||
| register_cpu_ci(est_time=7, suite="base-a-test-cpu") |
There was a problem hiding this comment.
@rbrugaro-amd is it expected that we register the test file to cpu test ? thanks.
…path (mori all-to-all) (sgl-project#28237) Signed-off-by: Rita Brugarolas Brufau <rita.brugarolasbrufau@amd.com>
…ghts When routed_scaling_factor is pre-folded into the topk weights (should_fuse_routed_scaling_factor_in_topk families: ModelOpt NVFP4 non-marlin, fp8 cutlass / trtllm-routed), forward_deepep skips the post-MoE rsf multiply, but both shared-expert-fusion paths still hard-coded the fused shared expert column weight to 1/rsf as if the multiply still happened, so the always-on shared expert contributes 1/rsf (0.4x for DeepSeek rsf=2.5) instead of 1.0x in every MoE layer. remap_topk_for_per_rank_shared_slots now sets the shared weight to 1.0 whenever the rsf is pre-folded: the aiter arm extended to also cover the apply_routed_scaling_factor_on_output families (the same signal forward_deepep checks, relayed through TopKConfig). WaterfillBalancer mirrors the condition via a new rsf_prefolded_in_topk flag plumbed from prepare_moe_topk. Non-fused families (deep_gemm fp8 etc.) and aiter are bit-identical to before. Follow-up to sgl-project#28237, which fixed the aiter arm only and left the non-aiter pre-folding families at 1/rsf (see the former in-code NOTE).

fix(moe): correct fused shared-expert scaling on aiter/DeepEP path
Motivation
On the HIP aiter + DeepEP-class MoE path (e.g. MoRI all-to-all), the fused shared
expert is under-weighted by
routed_scaling_factor, corrupting every MoE layerand producing degenerate, non-converging generation on long outputs.
Two code paths scale the MoE output differently:
DeepseekV2MoE.forward_deepepmultiplies thewhole MoE output by
routed_scaling_factorafter the experts run.aiter_biased_grouped_topkfoldsrouted_scaling_factorintoeach routed topk weight, and
forward_deepepskips the post-MoE multiply for_use_aiter(itsnot (... or _use_aiter)guard)._remap_topk_for_deepepunconditionally set the fused shared-expert weight to1/routed_scaling_factorto compensate for the post-MoE multiply. On the aiterpath that multiply never happens, so the always-on shared expert ended up scaled
by
1/routed_scaling_factor(≈0.4 at rsf=2.5) instead of 1.0.Impact is length-dependent, which is why this bug is easy to miss (full
numbers + commands under Accuracy Tests below):
always-on shared expert leaves the argmax token robust on short,
format-anchored prompts — so smoke tests pass and the bug hides.
repetition loop (
...→Wait→Wait→Wait...) and never terminates.(recovering the no-fusion reference of 82.0%), with
tokens_per_sampledropping6664 → 3713 as the runaway generation stops burning the token cap.
Modifications
Set the fused shared-expert weight to
1.0on the aiter path; keep1/routed_scaling_factorfor the default post-MoE-scaled path.Scope. This fix is deliberately scoped to the aiter path — the one validated
on AMD MI355X.
forward_deepepalso skips the post-multiply for the non-aiterfamilies that pre-fold
routed_scaling_factorin topk(
should_fuse_routed_scaling_factor_in_topk: ModelOpt NVFP4, cutlass/trtllm-routedfp8), so those would likewise need a
1.0shared weight — but they areintentionally left at their existing behavior here, to be addressed by their
maintainers on hardware where they can be validated.
New CPU unit tests (
test/registered/unit/layers/moe/test_fused_shared_expert_scaling.py,3 tests) pin the contract: aiter →
1.0, post-MoE-scaled (default) →1/rsf, andshared-id home-rank placement. Verified the patch applies cleanly to a pristine
rocm/sgl-dev:v0.5.13-rocm720-mi35x-20260613image and the 3 tests pass there.Accuracy Tests
Server (same env/flags for unpatched and patched; only the topk fix differs).
The buggy path is confirmed active at runtime (
router_topk=9,num_local_experts=33→ 1 shared expert fused per rank):Short-form (GSM8K) — insensitive to this bug
Before fix:
After fix:
GSM8K does not discriminate this bug: a constant 0.4× on the always-on shared
expert leaves the argmax token robust on short, format-anchored prompts (the
0.960/0.950 delta is within run-to-run noise).
Long-form reasoning — sensitive (this is the real failure)
Identical prompt,
temperature 0.0,max_tokens 8000:Before fix — the tail collapses into a runaway repetition loop and never
terminates (hits the token cap):
After fix — stays coherent for the full generation and reaches a final answer:
This is the same degenerate / non-convergent failure seen end-to-end in MLPerf.
End-to-end MLPerf accuracy (DeepSeek-R1, max OSL 20k)
The MLPerf accuracy harness (
exact_matchmetric) quantifies the same effect.Fusion ON unpatched vs patched, plus a no-fusion reference for context:
The fix lifts fusion-on accuracy 62.5 → 80.5, recovering essentially the
no-fusion reference (82.0).
tokens_per_sampledrops 6664 → 3713 (back to the~3610 no-fusion baseline) — direct evidence of the runaway-generation mechanism:
the under-weighted shared expert makes unpatched runs burn ~80% more tokens
hitting the generation cap.
Runtime accuracy validation was manual since the aiter/HIP path isn't covered by
CPU CI. (These subset runs are for relative before/after comparison, not a
compliant MLPerf submission.)
Benchmarking and Profiling
N/A — this is a correctness fix. The shared-expert weight is a scalar multiply
in topk remap; no throughput impact.
CI States
Latest PR Test (Base): ❌ Run #28007730005
Latest PR Test (Extra): ❌ Run #28007729851