Skip to content

[Spec Decode] Implement SupportsPP on all in-tree MTP draft models - #52117

Closed
nickus wants to merge 1 commit into
vllm-project:mainfrom
nickus:fix-draft-pp-verification
Closed

[Spec Decode] Implement SupportsPP on all in-tree MTP draft models#52117
nickus wants to merge 1 commit into
vllm-project:mainfrom
nickus:fix-draft-pp-verification

Conversation

@nickus

@nickus nickus commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #52069.

create_draft_parallel_config copies the target's pipeline_parallel_size into the draft's parallel config, so config validation demands SupportsPP of the draft model. Most MTP heads didn't declare it, so their speculative configs died in create_engine_config under PP > 1 — even though an MTP head always runs whole on a single PP rank and works fine there.

NemotronHMTP already shows the canonical fix: implement SupportsPP on the draft class itself, so registry.is_pp_supported_model() passes with no special-casing in the config layer. This PR applies that pattern to every in-tree MTP wrapper class (21 classes across 25 files, including the amd/xpu variants; subclasses inherit through the MRO; BailingMoeV3MTPModel, Glm4MoeLiteMTP, GlmOcrMTP and NemotronHMTP already conformed). The config layer is untouched: validation now holds every draft model to the same interface.

Test plan: tests/test_config.py::test_mtp_draft_model_not_required_to_support_pp (fails on main, passes here). Hardware: without the change --pipeline-parallel-size 4 --speculative-config '{"method":"mtp",...}' fails at config time on GLM-4.5-Air; with it, the server loads and serves (8× RTX 3090, three machines, nightlies dev678–dev693).


Assisted-By: Claude

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

Comment thread vllm/config/speculative.py Outdated
# SupportsPP from a model that is never pipelined, which rejects
# every MTP head under PP at config time (see #52069). Verify it as
# the single-stage model it is, restoring the config so rank and
# world-size bookkeeping downstream is unchanged.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A bit verbose, could you try to simplify it.

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.

Simplified — the mutate-and-restore dance is gone: verify now runs on a copy.copy of the parallel config with pipeline_parallel_size=1, 5 lines total. PR description trimmed as well.

@nickus
nickus force-pushed the fix-draft-pp-verification branch from 6e872b9 to ccabff6 Compare August 13, 2026 10:35
@nickus

nickus commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Added a regression test (test_mtp_draft_model_not_required_to_support_pp in tests/test_config.py): builds an MTP speculative config against luccafong/deepseek_mtp_main_random with pipeline_parallel_size=2 — this construction raised NotImplementedError before the fix — and asserts the draft's parallel config still carries the target's PP size, so only the verification treats it as single-stage. Mirrors the construction pattern of the adjacent test_draft_model_enables_async_scheduling_by_default.


Assisted-By: Claude

@nickus
nickus force-pushed the fix-draft-pp-verification branch 2 times, most recently from f1f2e1a to 11c06ca Compare August 13, 2026 13:26
@nickus

nickus commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Ran the regression test on hardware both ways (RTX 3090, wheel 0.27.2rc1.dev5+g98f86b9c0):

  • stock wheel → fails with exactly NotImplementedError: Pipeline parallelism is not supported for this model. Supported models implement the SupportsPP interface.
  • with this PR's change → 1 passed

Running it also caught a bug in the test itself: bare ParallelConfig(pipeline_parallel_size=2) fails pydantic validation on hosts with fewer than 2 GPUs, which CI runners often are. Fixed with nnodes=2 — that is what the latest force-push contains.


Assisted-By: Claude

@aoshen02

Copy link
Copy Markdown
Collaborator

Sounds good, will look today.

@aoshen02

Copy link
Copy Markdown
Collaborator

The code is a bit hacky. cc @TheEpicDolphin

@nickus
nickus force-pushed the fix-draft-pp-verification branch 3 times, most recently from 78a1d87 to 88aa5dc Compare August 14, 2026 09:19
@nickus
nickus requested a review from luccafong as a code owner August 14, 2026 09:19
@mergify mergify Bot added the deepseek Related to DeepSeek models label Aug 14, 2026
@nickus
nickus force-pushed the fix-draft-pp-verification branch from 88aa5dc to a3957c2 Compare August 14, 2026 09:36
@mergify mergify Bot added qwen Related to Qwen models kimi k3 labels Aug 14, 2026
@nickus nickus changed the title [Bugfix] Do not require SupportsPP of draft models under pipeline parallelism [Spec Decode] Implement SupportsPP on all in-tree MTP draft models Aug 14, 2026
@nickus

nickus commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Agreed — the config-layer copy was the wrong place to express this. Reworked: the PR now implements SupportsPP on every in-tree MTP draft class, following the NemotronHMTP precedent, and leaves the config layer untouched. Description updated to match.

@nickus
nickus force-pushed the fix-draft-pp-verification branch from a3957c2 to 101238f Compare August 14, 2026 10:20
@aoshen02

Copy link
Copy Markdown
Collaborator

Agreed — the config-layer copy was the wrong place to express this. Reworked: the PR now implements SupportsPP on every in-tree MTP draft class, following the NemotronHMTP precedent, and leaves the config layer untouched. Description updated to match.

still involve too many files, could you try to simplify this but make it elegant? Thanks.

A draft model is always built whole on the PP rank that samples tokens;
it is never split across pipeline stages. create_draft_parallel_config
nevertheless copied the target's pipeline_parallel_size, so config
validation demanded SupportsPP of every draft model and rejected MTP
speculative configs under PP > 1 even though they run fine there.

Construct the draft parallel config with pipeline_parallel_size=1, which
is the truth the V2 runner already implements. Validation then asks
nothing of models that never span ranks — in-tree and out-of-tree drafts
alike — with no per-model changes and no config-layer special-casing.

Fixes vllm-project#52069

Signed-off-by: Nick Iusiumbeli <nickuspro@gmail.com>
Assisted-By: Claude
@nickus
nickus force-pushed the fix-draft-pp-verification branch from 101238f to b3755d5 Compare August 14, 2026 13:05
@nickus nickus closed this Aug 14, 2026
@memgrafter

Copy link
Copy Markdown

Was this superceded? It contains 2 files. Thanks.

@aoshen02

Copy link
Copy Markdown
Collaborator

Was this superceded? It contains 2 files. Thanks.

Not sure what happen.

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 k3 kimi qwen Related to Qwen models

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: MTP speculative decoding cannot start under pipeline parallelism — SupportsPP demanded of the draft model

3 participants