Skip to content

[Bugfix][ROCm] Match the DeepSeek-V4.1 FP8 config in the shared-expert FSE compatibility check - #63099

Merged
shen-shanshan merged 6 commits into
vllm-project:mainfrom
BaoYunkai:bugfix/yunkai/dsv41-fse-compat-check
Oct 26, 2026
Merged

shen-shanshan merged 6 commits into
vllm-project:mainfrom
BaoYunkai:bugfix/yunkai/dsv41-fse-compat-check

Conversation

@BaoYunkai

@BaoYunkai BaoYunkai commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Overview

On CUDA and ROCm, deepseek_v4_fp8 always resolves to the DeepSeek-V4.1 DeepseekV4FP8Config, which subclasses Fp8Config, not the V4 class. is_shared_expert_quant_fse_compatible runs its DeepSeek-V4 checks only for the V4 class, so on these platforms it falls through to the generic Fp8Config branch and reports every DeepSeek-V4 shared expert as FSE-compatible. This PR matches both classes for MXFP4 routed experts, as the online-quantization branch of the same function already does.

Founded by Hyperloom.

Claims

  • Fixes the weight-loading failure of DeepSeek-V4-Flash on ROCm with VLLM_ROCM_USE_AITER_FUSION_SHARED_EXPERTS=1. Quark MXFP4 checkpoints hit the same failure on the FP8 MTP shared expert.
  • No change when the variable is unset: the check is not called.
  • No change for FP8-expert checkpoints such as DeepSeek-V4-Flash-Base: they keep the generic FP8 check, as on main.
  • Adds test_deepseek_v41_quant_config_rejects_fp8_mtp_shared_expert_fse and test_deepseek_v41_fp8_experts_keep_fp8_fse_check.

Validation

MI355X (gfx950), deepseek-ai/DeepSeek-V4-Flash, TP1, --kv-cache-dtype fp8 --block-size 256, vLLM built from main at 15a78aa (the touched files are unchanged on current main).

With VLLM_ROCM_USE_AITER_FUSION_SHARED_EXPERTS=1, main fails during weight loading in 2 of 2 runs:

KeyError: 'layers.0.ffn.experts.routed_experts.w13_weight_scale_inv'

With this PR, all 43 layers reject FSE ("only implemented/tested with Quark MXFP4"), run the shared expert unfused, and serve.

vllm bench serve, random 1000 in / 200 out, 400 prompts, concurrency 50, a fresh server per run. 4 runs per arm, median of the last 3, within-arm spread in parentheses:

Arm Total tok/s Median TTFT (ms) Median TPOT (ms)
main, variable unset 10061 (±3.5%) 1026 24.68
this PR, variable unset 10033 (±0.8%) 1024 24.76
this PR, variable = 1 10045 (±0.2%) 1027 24.77

GSM8K, 5-shot, greedy, 1319 items, lm_eval --model local-completions:

Arm strict-match Same verdict as main
main 0.9515 ± 0.0059 -
main, repeated (noise floor) 0.9568 ± 0.0056 98.10%
this PR, variable = 1 0.9538 ± 0.0058 97.95%

DeepSeek-V4-Flash-Base (FP8 experts), TP2, vLLM built from main at b3b0bc3: with the variable set, main runs the shared expert fused (257 experts, top-7 in the MoE), and GSM8K (same settings as above) stays within the noise of unfused runs: 0.9174 vs 0.9090 and 0.9234 from two runs without the variable. This PR gives the same FSE answer as main for this checkpoint.

Unit tests on MI355X against the built tree, pytest tests/model_executor/layers/test_fused_shared_expert.py: this PR 107 passed. Main fails test_deepseek_v41_quant_config_rejects_fp8_mtp_shared_expert_fse; test_deepseek_v41_fp8_experts_keep_fp8_fse_check passes on both.

Details

The existing unit test missed the bug because it constructs the V4 class directly. CI does not run DeepSeek-V4 with this variable set; the new tests are CPU-only.

AI assistance (Hyperloom with Claude) was used to find and prepare this change. I reviewed every changed line and reproduced the results above.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill. (Mandatory for agents, optional for humans).

  • AI assistance was used during the creation of this PR.

  • Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.

  • Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.

  • Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.

  • Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.

…t FSE compatibility check

Generated-by: Hyperloom
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: BaoYunkai <269084791+BaoYunkai@users.noreply.github.com>

@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.

@mergify mergify Bot added deepseek Related to DeepSeek models quantization DSv4.1 Related to DeepSeek-V4.1 models rocm Related to AMD ROCm bug Something isn't working labels Oct 22, 2026
@github-project-automation github-project-automation Bot moved this to Todo in AMD Oct 22, 2026
)

if isinstance(quant_config, DeepseekV4FP8Config):
if isinstance(quant_config, (DeepseekV4FP8Config, DeepseekV41FP8Config)):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A V4.1 config with expert_dtype="fp8" now takes this branch, and its first check returns "routed experts are not MXFP4". Today that class misses this isinstance and can still pass the Fp8Config check lower in the function. The reported failure and the new test are the fp4 Quark checkpoint. Should the fp8 case stay on that FP8 check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it should. On main, DeepSeek-V4-Flash-Base (FP8 experts) with the variable set runs fused (the MoE gets 257 experts, top-7), and GSM8K 5-shot stays within run-to-run noise of the unfused runs (0.917 vs 0.909 and 0.923; MI355X, TP2). The DeepSeek branch now only takes MXFP4 routed experts, so FP8 experts keep the FP8 check as on main. Added test_deepseek_v41_fp8_experts_keep_fp8_fse_check.

@Fangzhou-Ai

Copy link
Copy Markdown
Collaborator

Thanks @BaoYunkai , is dsv4 pro also having the same issue here?

@shen-shanshan shen-shanshan self-assigned this Oct 23, 2026
@shen-shanshan shen-shanshan added the verified Run pre-commit for new contributors without triggering other tests label Oct 23, 2026
Comment on lines 198 to 199

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please also update the log info to DeepSeek-V4/V4.1 ....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, updated all five messages to DeepSeek-V4/V4.1 ....

Comment on lines 207 to 211

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto.

Comment on lines 226 to 229
return (
False,
f"DeepSeek-V4 excludes shared experts at {shared_expert_prefix}",
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto.

Comment on lines 252 to 255
return (
False,
f"DeepSeek-V4 shared experts at {shared_expert_prefix} are not MXFP4",
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto.

@BaoYunkai

Copy link
Copy Markdown
Contributor Author

Yes. DeepSeek-V4-Pro has the same checkpoint layout as V4-Flash. On main with VLLM_ROCM_USE_AITER_FUSION_SHARED_EXPERTS=1 (MI355X, TP8, default MoE backend), it fails with the same KeyError: 'layers.0.ffn.experts.routed_experts.w13_weight_scale_inv'. TP8 with --moe-backend aiter takes the heterogeneous shared-expert path and does not reach this check.

The DeepSeek branch of is_shared_expert_quant_fse_compatible now also
handles the V4.1 config, so its rejection messages say DeepSeek-V4/V4.1.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: BaoYunkai <269084791+BaoYunkai@users.noreply.github.com>
@shen-shanshan

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #97236 for commit 4ea312e9c256.

Only MXFP4 routed experts need the DeepSeek-specific check. FP8-expert
checkpoints such as DeepSeek-V4-Flash-Base keep going through the
generic Fp8Config check, as on main, where fused shared experts load
and run correctly.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: BaoYunkai <269084791+BaoYunkai@users.noreply.github.com>
@shen-shanshan shen-shanshan added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 26, 2026
@github-actions

Copy link
Copy Markdown

✅ @BaoYunkai, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • Run commands test your branch with every commit currently on its upstream target branch. If it is behind, the bot merges the target branch into it first (pull before pushing again). Append --allow-stale to a run command to test an outdated branch as is, at your own risk.
  • /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
  • /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
  • /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

@shen-shanshan

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #97244 for commit 276f67552eb8.

@shen-shanshan
shen-shanshan merged commit c3941c0 into vllm-project:main Oct 26, 2026
115 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in AMD Oct 26, 2026
fxmarty-amd added a commit to fxmarty-amd/vllm that referenced this pull request Oct 26, 2026
Conflict: `tests/model_executor/layers/test_fused_shared_expert.py` also gained
DeepSeek-V4.1 coverage from vllm-project#63099. Keep this branch's rewritten per-layer
assertions (DSv4 mixed-precision skip reason, MTP layer-set expectations) and
adopt the reworded `DeepSeek-V4/V4.1 ...` skip reason; vllm-project#63099's two new
`test_deepseek_v41_*` tests merge in unchanged.
Micador pushed a commit to Micador/vllm that referenced this pull request Oct 27, 2026
…t FSE compatibility check (vllm-project#63099)

Signed-off-by: BaoYunkai <269084791+BaoYunkai@users.noreply.github.com>
Co-authored-by: BaoYunkai <269084791+BaoYunkai@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Shanshan Shen <467638484@qq.com>
Signed-off-by: Christopher Mackenzie <christopher.mackenzie.software@gmail.com>
Micador pushed a commit to Micador/vllm that referenced this pull request Oct 27, 2026
…t FSE compatibility check (vllm-project#63099)

Signed-off-by: BaoYunkai <269084791+BaoYunkai@users.noreply.github.com>
Co-authored-by: BaoYunkai <269084791+BaoYunkai@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Shanshan Shen <467638484@qq.com>
Signed-off-by: Christopher Mackenzie <christopher.mackenzie.software@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working deepseek Related to DeepSeek models DSv4.1 Related to DeepSeek-V4.1 models quantization ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm verified Run pre-commit for new contributors without triggering other tests

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants