Skip to content

[Core] Add an explicit sequence_parallel_moe override - #48657

Closed
drakosha wants to merge 1 commit into
vllm-project:mainfrom
drakosha:feat-sequence-parallel-moe-flag
Closed

drakosha wants to merge 1 commit into
vllm-project:mainfrom
drakosha:feat-sequence-parallel-moe-flag

Conversation

@drakosha

@drakosha drakosha commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Purpose

ParallelConfig.use_sequence_parallel_moe is a derived property with no override: it is decided entirely by the all2all backend and the parallel sizes. This PR adds an explicit sequence_parallel_moe setting (--sequence-parallel-moe / --no-sequence-parallel-moe) that defaults to None, i.e. no behavior change — the current heuristic keeps deciding unless an operator sets the flag.

Motivation is #48656. Since #48036 removed the data_parallel_size > 1 condition, sequence-parallel MoE auto-enables on every EP + TP deployment without data parallelism. On our TP=4 / EP / dp=1 GLM-5.2 setup (4×H200) that measured as:

GPU KV cache N=1 N=8 N=32
SP-MoE off 1,032,448 tok 75 tok/s 407 tok/s 931 tok/s
SP-MoE on (current main) 784,300 tok 61 tok/s 353 tok/s 875 tok/s

The extra all-gather/reduce-scatter per MoE layer, plus the activation memory it needs, cost more than the duplicate expert work it avoids — and today the only escape is switching all2all_backend away from the default, which changes the expert-exchange mechanism entirely. Patching parallel.py downstream (what we do now) is worse.

This PR only adds the knob; whether the auto-heuristic itself should be revisited for dp=1 is the open question in #48656.

Test plan

pytest tests/test_config.py -k sequence_parallel_moe

New test asserts the default still follows the heuristic and that an explicit value overrides it in both directions.

Test result

  • pytest tests/test_config.py -k sequence_parallel_moe: passes.
  • ruff check / ruff format: clean.
  • Verified on hardware (4×H200, GLM-5.2-NVFP4, TP4/EP/dp1): CLI parses to None / True / False, and --no-sequence-parallel-moe reproduces the "SP-MoE off" row above end-to-end.

AI assistance

AI assistance (Claude) was used for this change. The human submitter reviewed every changed line and ran the tests above.

`ParallelConfig.use_sequence_parallel_moe` is derived from the all2all
backend and the parallel sizes, with no way to override it. Since vllm-project#48036
dropped the `data_parallel_size > 1` condition, it also turns on for every
EP + TP deployment without data parallelism, where the extra all-gather /
reduce-scatter per MoE layer and the activation memory it needs can cost
more than the duplicate expert work it avoids (see vllm-project#48656: -19% single-
request throughput and -24% KV cache on GLM-5.2 at TP4/EP/dp1).

Add `sequence_parallel_moe` (`--sequence-parallel-moe` /
`--no-sequence-parallel-moe`), defaulting to None so the heuristic keeps
deciding; an explicit value overrides it. No behavior change by default.

Co-authored-by: Claude
Signed-off-by: Mikhail Kostryukov <mike@triptrack.net>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@yewentao256 yewentao256 self-assigned this Jul 15, 2026

@yewentao256 yewentao256 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work! Let me take a further look

@yewentao256 yewentao256 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you test #48763?

We don't want to make this exposed to users as it is sometimes complicated

@drakosha

Copy link
Copy Markdown
Contributor Author

Thanks @yewentao256. Independent confirmation just landed in #48656: a bisect
pins the regression to #48036 (1ff942965, the removed data_parallel_size > 1
gate), and on Nemotron-3-Ultra-550B-NVFP4 at TP4/EP/DP1 memory went
79.71 → 85.33 GiB (+7%). So this isn't GLM-5.2-specific — it's MoE models on
pure TP generally.

I don't think #48763 covers it:

  • It only changes deepseek_v32/nvidia/mtp.py. The repro you posted has no
    --speculative-config, so MTP isn't loaded and that file can't be producing
    the 1.4–4.8% — could you share the exact command behind those numbers?
  • Even with MTP, glm_moe_dsa is rewritten to the generic DeepSeekMTPModel
    (SpeculativeConfig.hf_config_override → registry deepseek_mtp), so
    DeepseekV32MTP.forward doesn't run on the default GLM-5.2 path — that class
    isn't registered as an arch; reaching it needs the NVIDIA model impl forced
    explicitly. Since the reordered collective doesn't execute here, it can't move
    throughput or KV capacity for this config either way.

The fix that matches both reports is the one passtoor-agi also suggested:
restore and data_parallel_size > 1 in the use_sequence_parallel_moe
heuristic, so SP-MoE auto-enables only with DP. That removes the CLI surface
entirely if the flag is your concern. (I'm not claiming SP-MoE is useless at
DP=1 — TP still duplicates work SP could shard — only that on GLM-5.2 and
Nemotron its current memory/comm cost outweighs the benefit, so DP1 auto-enable
is the wrong default.)

Happy to close #48657 once the DP gate is restored or another working opt-out
lands; until then the regression stands on two models with a clean bisect. I'll
post our GLM-5.2 N=1-decode + KV numbers tonight.

AI assistance was used for the code-path analysis above; our GLM-5.2 numbers
will be from a real run.

@yewentao256 yewentao256 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you are right, #48660 is for MTP case only.
Let's close this issue and apply #48849

@drakosha

Copy link
Copy Markdown
Contributor Author

Numbers as promised, on GLM-5.2-NVFP4, TP4 / DCP4 / EP / dp=1, util 0.945,
MTP=3, fp8_ds_mla. Same image throughout — the only change is the
and data_parallel_size > 1 term in use_sequence_parallel_moe (before vs
after #48036).

metric SP-MoE off (dp>1 gate) SP-MoE on (#48036) Δ
N=1 decode 98.0 tok/s 78.3 tok/s −20%
N=32 aggregate ~883 tok/s ~791 tok/s −10%
GPU KV cache 950,272 tok 734,208 tok −22.7%
boots at --max-model-len 786432

The last row is the sharp one: with SP-MoE on, the engine fails init at our
production max-model-len — 10.02 GiB KV needed, 7.7 GiB available; estimated maximum model length is 604416. So the −22.7% KV isn't just "slower", it drops
us below the config we actually serve (I had to lower max-model-len to 512K just
to boot SP-on for its tok/s).

This matches the independent Nemotron report in #48656 (same bisect to #48036,
+5.6 GiB at TP4/EP/DP1) — the regression holds on two models.

I also confirmed at runtime that #48763 doesn't touch this path: the prod log
shows Resolved architecture: DeepSeekMTPModel for GLM-5.2, i.e. the generic
deepseek_mtp, not the DeepseekV32MTP that #48763 patches.

So the ask stands: restore and data_parallel_size > 1 in the heuristic, no
user-facing flag needed. Happy to close #48657 in favor of that.

AI assistance was used to run and analyze this; the numbers are from a real
4×H200 run.

@yewentao256

Copy link
Copy Markdown
Member

Thanks for the work, close this PR as #48849 landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants