[ModelRunner V2] Speculative Decoding NGram GPU Implementations - #40704
PatchouliTIS wants to merge 82 commits into
Conversation
Signed-off-by: PatchouliTaisa <patchychen@tencent.com>
Signed-off-by: PatchouliTaisa <patchychen@tencent.com>
Signed-off-by: PatchouliTaisa <patchychen@tencent.com>
Signed-off-by: PatchouliTaisa <patchychen@tencent.com>
Signed-off-by: PatchouliTaisa <patchychen@tencent.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a GPU-accelerated N-gram speculator (NgramGPUSpeculator) for speculative decoding in vLLM V2. Key changes include infrastructure to handle variable-length draft tokens, memory management updates to keep token IDs on the GPU for faster scanning, and performance optimizations for the Gumbel sampler by making FP64 precision optional. A review comment suggests improving the N-gram matching logic in the kernel to select the most recent occurrence of a pattern instead of the first, which better captures local context for prompt lookup.
| idx = matches.int().argmax(dim=1) | ||
| has_match = matches[batch_idx, idx] | ||
| first_match_pos[:, i] = torch.where(has_match, idx.long(), -1) |
There was a problem hiding this comment.
The current implementation uses argmax(dim=1) on the boolean matches tensor, which finds the first occurrence of the n-gram pattern in the sequence. In speculative decoding (specifically prompt lookup), it is standard practice and significantly more effective to use the most recent (last) occurrence of the pattern, as it better captures the local context.
You can find the last match by applying argmax to a tensor of indices where matches occur, which will return the largest index for each row.
| idx = matches.int().argmax(dim=1) | |
| has_match = matches[batch_idx, idx] | |
| first_match_pos[:, i] = torch.where(has_match, idx.long(), -1) | |
| # Find the last match by using argmax on indices to get the most recent occurrence | |
| matched_indices = torch.where(matches, window_pos.unsqueeze(0), -1) | |
| idx = matched_indices.argmax(dim=1) | |
| has_match = matches[batch_idx, idx] | |
| first_match_pos[:, i] = torch.where(has_match, idx, -1) |
Signed-off-by: PatchouliTaisa <patchychen@tencent.com>
|
@PatchouliTIS it seems like the FP64 -> FP32 gumbel sample changes are not necessary for enabling the ngram functionality. Would it be possible to separate those changes out into a separate PR? |
okay, I'm on my vacation now and I will handle this next week. |
Signed-off-by: PatchouliTaisa <patchychen@tencent.com>
|
Removed gumbel sampling modifications from this PR, ready for review. @TheEpicDolphin |
f1c0a89 to
ee72c7a
Compare
…esolve_cudagraph_mode_and_sizes Both adaptive verification and variable-length drafters decide per-request query lengths on device, so decode batches are varlen and cudagraph capture needs a separate decode routine. Express that as a varlen_decode flag on resolve_cudagraph_mode_and_sizes, alongside the other backend-support downgrades, instead of mutating compilation_config.cudagraph_mode from the model runner beforehand. Only CUDAGraphMode.FULL actually needs to change (it has full cudagraphs but no separate decode routine); PIECEWISE/NONE capture no full decode graphs and FULL_DECODE_ONLY/FULL_AND_PIECEWISE already have one. This drops the adaptive-verification override of an explicitly requested PIECEWISE or FULL_DECODE_ONLY mode, which was a no-op for the default FULL_AND_PIECEWISE. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Nick Hill <nickhill123@gmail.com>
Replace the getattr/hasattr injection of RequestState into the speculator with an explicit init_speculator parameter, handed to the speculators that draft from the persistent token store (currently only NgramGPUSpeculator). RequestState is now built before the speculator, which only needs config values that were already available at that point. NgramGPUSpeculator.req_states is consequently non-optional, so propose() drops its injection assert, and its tests exercise a real RequestState instead of a duck-typed stand-in. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Nick Hill <nickhill123@gmail.com>
…draft_trimmer Pass vllm_config and RequestState instead of eight individually-derived values: LoRA/PP/CP/cudagraph-mode support all come from the config, and max_num_reqs, device and the logit chunk limit from RequestState. Declare trims_drafts_on_gpu and num_valid_drafts on BaseSpeculator so the factory reads them directly rather than through getattr, which also lets it accept a None speculator and drop that check from the call site. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Nick Hill <nickhill123@gmail.com>
trims_drafts_on_gpu carried no information beyond "num_valid_drafts is set", so replace both with a single optional num_valid_drafts_for_trim tensor on BaseSpeculator: None means verify every scheduled draft, a tensor opts the drafter into device-side trimming. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Nick Hill <nickhill123@gmail.com>
|
Hi @PatchouliTIS, 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, |
|
I pushed my changes along with a bit more rework, but I'm still not sure it's in the best state w.r.t. how the draft trimmer abstraction is structured.
@PatchouliTIS here is the benchmark script that was used. I hadn't actually read it, I guess the prompts aren't ideal and we could use some better test sets for this: bench_ngram_trim.py |
|
Hi @PatchouliTIS, 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, |
|
Thanks @PatchouliTIS for the PR and maintainers for reviewing it! Really excited to see Ngram x MRv2 getting merged! @njhill - just wondering if the PR is likely to be merged soon, or if there’s any blocker preventing the merge? |
|
Hi @PatchouliTIS — we tried a fixed-budget port of your GPU ngram implementation on Qwen3.8-Flash-Next across two DGX Sparks, and got useful copy/edit results. Thanks for the scan/finalize kernels; this is based on your work at Against our optimized native-MTP4 baseline (already using the FP8 proposal head from #56577), K12 measured:
That's 57–108% higher output throughput on these copy/edit fixtures. Fresh paired runs used concurrency 1, one warmup plus three measured requests per case, eager execution and prefix caching off. All 32 responses matched the expected outputs exactly, with matching per-case hashes/lengths across arms and natural stops (552/552/1094/1095 output tokens). An earlier configuration-only screen agreed. The GPU proposer tests passed 20 cases, with 2 explicit skips for variable-length trimming that we did not port. The local integration needed proposer construction after request-state allocation, request-history plumbing, Our runtime base was This is an alternative proposer for copy/edit work, not simultaneous MTP+lookup or a general-generation speedup. We have not validated the full PR, variable-budget scheduler integration, CUDA graphs, concurrent requests or general sampling. Would a minimal Qwen/V2 compatibility patch and the fixed-budget reproduction cases be useful here? We can prepare those against your preferred base; we do not plan to open a competing ngram implementation PR. Development and testing were AI-assisted, with the experiments executed by the coding agent. |
Signed-off-by: Xuanan Chen <xuananchenc@nvidia.com>
|
I see in the Test plan that the AL reported for sonnet is 1.15 for MRv1 and 2.58 for MRv2. |
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Xuanan Chen <xuananchenc@nvidia.com>
|
@PatchouliTIS @njhill - wondering if the code correctness is established with current state of the PR and only code quality improvement is being worked on OR there is something critical missing from the PR and needs to fixed/implemented? Asking because I wanted to use ngram-gpu in MRv2 so checking if its safe to build from the current commit of the PR. Lmk otherwise if its unsafe to use the PR right now. |
Purpose
Added a new NGram GPU speculator.
The main feature is a new implementation at:
vllm/v1/worker/gpu/spec_decode/ngram/speculator.py, similiar to [Core] NGram GPU Implementation compatible with Async Scheduler #29184.Updated request state storage for NGram GPU.
vllm/v1/worker/gpu/states.pyvllm/v1/worker/gpu/model_runner.pyThis changes how RequestState is initialized so that all_token_ids can stay densely resident on GPU instead of defaulting to UVA when ngram_gpu is active. As discussed in [Core] NGram GPU Implementation compatible with Async Scheduler #29184, the new n-gram speculator repeatedly scans active request token history, doing that from GPU-resident dense storage is much more appropriate than pulling through UVA-backed memory, this is a performance-oriented architectural change supporting the new feature.
The
model_runner.pyalso injects req_states into speculators that need direct access to the persistent token store.Added variable-length draft token plumbing
Several files were updated to support draft proposals where different requests may have different numbers of valid draft tokens:
vllm/v1/outputs.pyvllm/v1/worker/gpu/spec_decode/utils.pyvllm/v1/core/sched/scheduler.pyvllm/v1/engine/core.pyvllm/v1/worker/gpu/model_runner.pyDraftTokenIds now includes:
num_valid_draft_tokens: list[int] | None.Scheduler logic now truncates speculative tokens based on
num_valid_draft_tokens.EngineCore adds _maybe_update_async_draft_token_ids()to consume draft metadata from async execution and update scheduler state at the right time.Test Plan
vllm bench cmd:
Test Result
Async NGram GPU V1 results:
Async NGram GPU V2 results:
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.