Skip to content

[ROCm] Remove MRV1 model exceptions - #56360

Merged
njhill merged 1 commit into
mainfrom
rocm-mrv2-default
Oct 10, 2026
Merged

njhill merged 1 commit into
mainfrom
rocm-mrv2-default

Conversation

@njhill

@njhill njhill commented Sep 11, 2026

Copy link
Copy Markdown
Member

Remove the ROCm-specific exceptions for falling back to use MRV1 by default for certain models, now that the performance gaps have been addressed.

@mergify mergify Bot added the rocm Related to AMD ROCm label Sep 11, 2026
@github-project-automation github-project-automation Bot moved this to Todo in AMD Sep 11, 2026
@mergify

mergify Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @njhill.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Sep 12, 2026
mawong-amd added a commit to ROCm/vllm that referenced this pull request Sep 17, 2026
apply_config_platform_defaults forced cudagraph_mode=NONE for DeepSeek-V4 on
gfx950 under MRV2, citing the unquantified accuracy report in vllm-project#52644. That
report was filed against ROCm 7.2.53211 and never answered when a maintainer
asked for the score; its author's own workaround says it is "just a workaround
before ROCm 7.14 is released".

The regression does not reproduce on either runtime. DeepSeek-V4-Pro, TP8,
fp8 KV cache, MRV2, gsm8k 8-shot:

  ROCm 7.2.53211, n=1319:  0.9591 graphed vs 0.9515 eager
  ROCm 10 / HIP 7.15, n=100:  0.95 graphed vs 0.97 eager

The graphed arm is equal or better in both, i.e. the opposite sign to the
report. Meanwhile the default costs 8.5x/5.9x/3.1x output throughput at
concurrency 1/16/64 on 7.2, and 9.3x/4.9x/1.6x at 1/32/256 on ROCm 10; eager
TPOT is flat at 124-135 ms from concurrency 1 to 64, so decode is bound by
Python dispatch rather than by the GPU.

The gate only fires under MRV2, so removing the ROCm MRV1 exceptions (vllm-project#56360)
would otherwise make it reachable by default.

Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
Signed-off-by: Nick Hill <nickhill123@gmail.com>
@njhill
njhill force-pushed the rocm-mrv2-default branch from 144b0fc to 4bf97b2 Compare October 9, 2026 22:35
@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 9, 2026
@njhill
njhill marked this pull request as ready for review October 9, 2026 22:36

@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 repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@njhill

njhill commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

/amd-ci run

@mergify mergify Bot removed the needs-rebase label Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite AMD CI #14412 for commit 4bf97b2e4418.

@mawong-amd mawong-amd self-assigned this Oct 10, 2026

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

Approving. Checked on head 4bf97b2:

  • AMD CI on MI355: DeepSeek-V3.2 TP/DP, DeepSeek-V4-Flash-NVFP4 and Qwen3-30B-NVFP4 all pass their GSM8K thresholds, running on MRV2.
  • Perf, TP8, 1k/1k, default config, median TPOT in ms (MRV1 -> MRV2). Single runs, and GSM8K is within noise:
model ROCm conc 1 conc 128 max delta (conc 1-128)
GLM-5.2 FP8 10 11.46 -> 11.42 32.12 -> 32.09 0.5%
GLM-5.2 FP8 7.2.3 11.52 -> 11.52 32.22 -> 32.05 0.5%
GLM-5.2 MXFP4 10 9.29 -> 9.22 25.64 -> 25.60 0.8%
GLM-5.2 MXFP4 7.2.3 9.36 -> 9.32 25.55 -> 25.52 1.4%
DeepSeek-V4-Pro 10 11.47 -> 11.45 26.68 -> 26.43 1.0%
DeepSeek-V4-Pro 7.2.3 14.76 -> 14.20 27.82 -> 27.28 3.8%

The GLM-5.2 slowdown behind #53155 no longer reproduces. It looks fixed by #51915, and the DeepSeek-V4 one by #53712. This also makes #54925 unnecessary.

@njhill

njhill commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

/ci run --allow-stale

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #93958 for commit 4bf97b2e4418.

⚠️ This PR is 14 commits behind upstream main. Running CI at your own risk because --allow-stale was requested; outdated CI configuration may cause failures. Before merging, merge or rebase onto the latest main, then rerun /ci run on the latest PR commit.

@njhill
njhill enabled auto-merge (squash) October 10, 2026 03:20
@njhill
njhill merged commit 7d0b4e5 into main Oct 10, 2026
183 checks passed
@njhill
njhill deleted the rocm-mrv2-default branch October 10, 2026 05:09
jschmied pushed a commit to jschmied/vllm that referenced this pull request Oct 10, 2026
vllm/config/vllm.py: main removed ROCM_DEFAULT_MRV1_ARCHITECTURES (vllm-project#56360); keep only
_QWEN4EXP_RECOVERSSM_ARCHS. tests/kernels/mamba/test_recoverssm_ple.py: ple_conv moved to
vllm.models.qwen4_exp.common.ops.ple upstream (same signature).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Jürgen Schmied <juergenschmied70@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants