[GG] fix(spec-decode): centralize probabilistic draft sampling - #211
Conversation
Signed-off-by: Martin Vit <martin@voipmonitor.org>
📝 WalkthroughWalkthroughThe base speculator now owns probabilistic draft sampling. Autoregressive and DSpark speculators delegate to this shared helper, preserving draft position handling, seeds, temperature, step metadata, processed logits, and active-row state. Tests cover sampler arguments and delegation. ChangesProbabilistic draft sampling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
vllm/v1/worker/gpu/spec_decode/dspark/speculator.py (1)
253-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a DSpark regression test for the position translation.
Line 258’s
-2adjustment is a DSpark-specific Philox contract; the added tests cover only the base helper and autoregressive delegation. Assert this call forwardssample_pos[:, i] - 2so a future cleanup cannot shift DSpark’s randomness stream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vllm/v1/worker/gpu/spec_decode/dspark/speculator.py` around lines 253 - 264, Add a DSpark regression test covering the call from the relevant speculator method to _sample_probabilistic_draft, and assert its positions argument is exactly sample_pos[:, i] - 2. Keep the assertion focused on preserving this DSpark-specific position translation and randomness contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@vllm/v1/worker/gpu/spec_decode/dspark/speculator.py`:
- Around line 253-264: Add a DSpark regression test covering the call from the
relevant speculator method to _sample_probabilistic_draft, and assert its
positions argument is exactly sample_pos[:, i] - 2. Keep the assertion focused
on preserving this DSpark-specific position translation and randomness contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07142b45-8717-4b54-93dd-25af02a2c48a
📒 Files selected for processing (4)
tests/v1/worker/test_gpu_autoregressive_speculator.pyvllm/v1/worker/gpu/spec_decode/autoregressive/speculator.pyvllm/v1/worker/gpu/spec_decode/dspark/speculator.pyvllm/v1/worker/gpu/spec_decode/speculator.py
Rental-machine validation — PASS (focused and composed-source gates)Tested merged commit: Host: Vast.ai 46335896, Ubuntu 24.04.4 / kernel 7.0.0-28, 2x EPYC 9455, 1.5 TiB RAM. Commands and results
Greedy/temperature-zero coverage passed and no capture-time compile, Xid, hang, or request error was observed. The later full-model live-probabilistic gate was not run because independent SparkInfer and LMCache source-composition gates failed first. Report and durable logs: field-review result No unreported source modifications were present; the tested worktrees were clean. Exact r14-base composition — PASSThe immutable r14 vLLM lock was replayed from base Resulting candidate This extends the earlier current-development-branch result to the actual r14 image source boundary. Exact evidence: r14 ledger at |
Live full-model probabilistic follow-up — PASSTested merged change: The previously missing live gate is now complete. The full GLM-5.2 3.25-bpw EXL3 server ran the V2 runner with MTP3, Observed across 32 live requests / 4,096 generated tokens:
The exact-r14 focused distribution suite already passed 11/11 cold and 11/11 warm, including greedy and temperature-zero controls. A separate plain-checkpoint download is unnecessary: this EXL3 MTP path reaches the same changed sampler. Log scope: cold priming compiled 28 previously unseen SparkInfer request shapes, which the test plan permits. The subsequent warm audit contains zero compile, ERROR, CUDA/IPC, or process-failure findings. Four XGrammar FSM errors occurred earlier in structured-output checks because this exact-source gate deliberately disabled the appliance compatibility patch to preserve SHA attribution; that separate built-image requirement is not attributable to #211. Evidence: campaign ledger, raw benchmark JSON, and clean warm-window audit. |
Summary
Centralize probabilistic draft Gumbel sampling in
DraftModelSpeculator._sample_probabilistic_draftand route the generic, autoregressive, and DSpark paths through it.The shared method always applies
draft_gumbel_pos(), keeping proposal noise disjoint from verifier acceptance/recovery offsets. The autoregressive caller still forwardsactive_num_reqs, preserving its FULL-graph padded-row safeguard.Fixes #205.
Supersedes #209.
Why this replaces #209
#209 correctly fixes the live GG autoregressive override, but only changes that call site. GG already contains the broader salted-stream backport corresponding to upstream vLLM vllm-project#47386; a later GG autoregressive override copied the old upstream
positions + 1call and bypassed the corrected base implementation.Keeping raw
gumbel_sample()calls in individual speculators makes that failure mode easy to repeat. This PR makes stream ownership a base-speculator responsibility instead:active_num_reqs;Only the base method calls
gumbel_sample()inspec_decode; individual speculators pass model positions, not Philox offsets.Correctness scope
Affected before this fix:
Unaffected:
The first-order defect is rejection recovery reusing the exact vocabulary-sized Gumbel field that selected the rejected draft token. Immediate acceptance probability is unchanged; the fix restores the recovery sampler's independence contract.
Runtime invariants
positions + 1 + 2**30.Verification
Release-container tests:
GPU tests on CUDA:
Static checks:
The existing statistical regression inherited from upstream vllm-project#47386 measures the coupled stream outside its target-distribution noise floor and the salted stream within it; this PR preserves that implementation and closes the GG override gap without claiming an unmeasured model-level quality delta.
Summary by CodeRabbit
Bug Fixes
Tests