Repository navigation
[Bugfix][MRV2][Spec Decode] Honor dynamic K in autoregressive speculators, 1.2~1.3x kernel perf improvement - #57053
Conversation
Signed-off-by: yewentao256 <zhyanwentao@126.com>
There was a problem hiding this comment.
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.
|
/ci run |
|
✅ Triggered Buildkite CI #89149 for commit |
|
/ci run |
|
✅ Triggered Buildkite CI #89399 for commit |
|
Hi @yewentao256, 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, |
Signed-off-by: yewentao256 <zhyanwentao@126.com>
|
/ci run |
|
❌ This PR is 1 commit behind upstream |
|
/ci run --allow-stale |
|
✅ Triggered Buildkite CI #89416 for commit
|
|
/ci run |
|
✅ Triggered Buildkite CI #89905 for commit |
|
/ci run |
|
✅ Triggered Buildkite CI #90287 for commit |
|
✅ Triggered Buildkite CI #91398 for commit |
|
This pull request has merge conflicts that must be resolved before it can be |
LucasWilkinson
left a comment
There was a problem hiding this comment.
LGTM to me, i do think as a follow-up once MRV1 is deleted we should fold this into the adaptive verification code
would be good for @benchislett and @TheEpicDolphin to review also
Signed-off-by: yewentao256 <zhyanwentao@126.com>
|
/ci run |
|
❌ This PR is 1 commit behind upstream |
|
/ci run --allow-stale |
|
✅ Triggered Buildkite CI #92245 for commit
|
|
/ci run |
|
✅ Triggered Buildkite CI #92624 for commit |
CI selector (shadow): 127 test steps (165 jobs) instead of 78 (97 jobs)Shadow mode: this changes nothing about what CI runs. It shows what the evidence-based selector would pick for this PR, next to today's rules. How it works. Feedback welcome: reply here if it would skip a step this change needs, or runs something unrelated.
Selector would run (127)
Would skip (today's rules run them) (13)
Would add (today's rules do not run them) (62)
AMD mirrors: would skip (9)
AMD mirrors: would add (52)
15 changed files · base |
|
/ci run |
|
✅ Triggered Buildkite CI #92695 for commit |
… in config/vllm.py and the V2 model runner - vllm/config/vllm.py: num_lookahead_tokens keeps Uno's branch first (K lookahead slots: one seed and K-1 noisy rows), then upstream's fill-in rule (vllm-project#59105): DFlash and fill-in DSpark reserve K + 1, anchor-sampling DSpark, EAGLE and draft models reserve K. - vllm/v1/worker/gpu/model_runner.py: ExecuteModelState carries upstream's num_spec_tokens_to_schedule (vllm-project#57053) beside Uno's skip_speculator_proposal and zero_next_draft_req_ids. The proposal stays in _run_speculator_proposal, after the AsyncOutput D2H handoff and behind the proposal-skip check; it keeps the watermark preparation, the draft workspace lane and adaptive-verification confidences, and now passes num_speculative_tokens to propose() and blanks draft rows past the scheduled count to -1, as upstream does. _publish_draft_tokens publishes only the first num_spec_tokens_to_schedule columns, for every speculator and for diffusion drafts. Dynamic K and Uno: the scheduler lowers num_spec_tokens_to_schedule below K only when num_speculative_tokens_per_batch_size is set, and Uno's config rejects that option ("Uno requires a fixed num_speculative_tokens"). Uno therefore always runs at its configured K and its proposals and acceptance are unchanged. The generic blanking and column slice still apply to Uno, so if the scheduler ever asked for fewer tokens, the extra drafts would be dropped before verification and the output would stay lossless. Signed-off-by: MJ <121681560+mjbrn@users.noreply.github.com>
The V2 runner now passes the scheduled spec-token count to every speculator. Uno requires a fixed K and drafts all K rows in one launch, so it accepts the argument like DFlash; the runner blanks any rows past the scheduled count. The sample_tokens ordering test carries the new ExecuteModelState field and expects the new keyword. Signed-off-by: MJ <121681560+mjbrn@users.noreply.github.com>
Purpose
MRV2 did not propagate
SchedulerOutput.num_spec_tokens_to_scheduleto autoregressive speculators, causing unnecessary draft forwards and stale draft tokens.This PR fixes the issue
Test
Acc covered in current unit test
Perf can be seen in this AI generated script
And we can get