[Spec Decode][Perf] Fuse the MTP trailing all-reduce; local-argmax draft tokens - #49793
Conversation
|
This pull request has merge conflicts that must be resolved before it can be |
f38a02f to
2669b05
Compare
2669b05 to
ac90d2e
Compare
|
Part of the #48597 re-split. See that PR for the tracker: merge order, measured numbers, and what was dropped. |
f5de84c to
625d357
Compare
|
Hi @zhou9402, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
|
This pull request has merge conflicts that must be resolved before it can be |
625d357 to
f7c92f1
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #82463 for commit |
|
Hi @zhou9402, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
|
/ci run |
|
✅ Triggered Buildkite CI #82503 for commit |
Resolve the DeepSeek V3.2 indexer conflict by retaining the typed active-indexer helper while preserving main's skip-topk behavior. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com>
|
To clarify, we are going to merge #47352 first |
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com>
|
Is this PR still needed? |
The index_share_for_mtp_iteration gates and the V2 speculator lifecycle hooks this PR proposed have since landed on main, so the only thing left in attention.py was rewriting the existing `self.indexer is not None and not self.skip_topk` checks into an `_active_indexer` property — no behavior change. Revert it and keep the PR to the two MTP draft-path optimizations plus the lifecycle-hook regression test. Co-authored-by: Claude Opus 5 Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com>
Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com>
Yes, but much simpler now — the index-sharing part has already merged into main, so all that's left here is the RMSNorm fusion and the local argmax. |
get_top_tokens took spec_step_idx as int|None with an assert while the sibling compute_logits takes int=0 and silently uses step 0. The assert guarded a case compute_logits does not guard either, so drop it and keep the two signatures identical. Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com>
…aft tokens (vllm-project#49793) Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: Alessandra005 <aurib032@fiu.edu>
…aft tokens (vllm-project#49793) Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…aft tokens (vllm-project#49793) Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: Wyett <wyettzeng@gmail.com>
…aft tokens (vllm-project#49793) Signed-off-by: Peiyuan Zhou <peiyuanzhou1994@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: Zhu, Zufang <zufang.zhu@intel.com>
Two optimizations on the DeepSeek-V3.2 / GLM-5.2 MTP draft path.
non-sequence-parallel path, as the main model already does at layer
boundaries. The sequence-parallel path is unchanged.
get_top_tokens),skipping the full-vocab all-gather in
compute_logits. The proposer alreadyprobes for this via
use_local_argmax_reduction; this adds the model side.Not a duplicate: an earlier revision also carried the
index_share_for_mtp_iterationgates and the V2 speculator lifecycle hooks,both of which have since landed on main; that part is dropped. Nothing open
overlaps with what remains.
Benchmarks
2×GB300, TP8 across 2 nodes,
nvidia/GLM-5.2-NVFP4, MTP=5,VLLM_USE_V2_MODEL_RUNNER=1, fp8 KV cache, no prefix caching. The baseline armis the same tree with the fusion reverted to an explicit
tensor_model_parallel_all_reduce+RMSNorm; local argmax is toggled with--speculative-config.use_local_argmax_reduction.Batch 1 (8192 in / 1024 out, c=1) — no measurable change. Three runs per
arm, output tok/s and MTP acceptance length:
The arms overlap completely. At c=1 the saved all-gather is one vocab row and
the fused all-reduce is one small tensor, so this is the expected result;
median ITL sat at 9.08–9.12 ms across all nine runs.
Concurrency 64 (1024 in / 1024 out, 256 prompts) — this is where it pays.
One A/B pair on the same node pair:
The gain cannot be attributed to speculation luck: acceptance length is 3.5%
lower in the faster arm. Normalizing throughput by acceptance length
(steps/s) gives +17.8%. At c=64 the draft's per-step full-vocab all-gather is
64 × 151k × 2 B ≈ 19 MB, five times per accepted token — that is what
get_top_tokensremoves.Caveats, stated plainly: the c=64 result is a single A/B pair, and a repeat was
lost to cluster contention. Cross-node variability on this cluster is large
(the +fusion arm measured 4226 tok/s on a different node pair with the same
acceptance length as the baseline), so only within-pair comparisons above are
meaningful — do not compare numbers across the two tables.
Accuracy
gsm8k, 5-shot, full 1319 questions, via
lm_eval --model local-completions:Unchanged within stderr, which is the point: the fused kernel accumulates in
fp32 and local argmax breaks vocab-boundary ties differently, so both are
output-affecting in principle.
AI assistance (Claude) was used; every changed line has been reviewed.