[Bugfix][DSpark] Support speculators-convention checkpoints (gamma+1-wide draft block) - #30982
Open
buddywhitman wants to merge 5 commits into
Open
buddywhitman wants to merge 5 commits into
buddywhitman wants to merge 5 commits into
Conversation
…n't silently degrade DeepSpec-trained DSpark checkpoints train block slot k to predict anchor+k+1, with every slot trained -- this is the convention run_markov_block (dspark.py) is written against. speculators (github.com/vllm-project/speculators)-trained checkpoints instead train slot j to predict anchor+j with slot 0 loss-masked, so loading one of these produces a checkpoint where every run_markov_block slot is read one position early. The result isn't a crash or an obvious error -- it's a checkpoint that loads and runs fine but accepts almost nothing (accept_len ~1.1, vs. ~4.05 for the same weights on vLLM), silently throwing away nearly all of DSpark's speedup. Diagnosed and confirmed by jessiewei7 on sgl-project#30261 (comment, 2026-07-09): validated against RedHatAI/GLM-5.2-speculator.dspark and mgoin/GLM-5.2-speculator.dspark-block16, both detectable via speculators_model_type="dspark" in the checkpoint config, both showing the ~1.1 accept-length symptom, both explained by the slot-shift mismatch above. This adds detection (DSparkDraftConfig.speculators_convention, set in parse_dspark_draft_config) and, per the safer of the two fixes jessiewei7 offered ("conditional shift on load, or a clear reject-with-error"), raises a clear, cited ValueError in DSparkDraftMixin.__init__ instead of silently degrading. Implementing the actual slot-shift correction requires care around buffer sizes that are fixed to gamma across the block-verify/KV-cache/CUDA-graph machinery (DraftBlockResult, VerifyWindow, capture buffers) -- that's real surgery I can't validate without the real checkpoint + GPU hardware this PR's own comment thread used, so it's left as a documented follow-up rather than guessed at here. This change is deliberately the smaller, unambiguously-safe half: it stops the checkpoint from running in a state that looks like it works but silently doesn't. 3 new unit tests (parse_dspark_draft_config detection: DeepSpec checkpoint unflagged, non-dspark speculators model unflagged, dspark speculators model flagged), 5/5 existing dspark config tests still passing, no regressions.
gemini-code-assist flagged the exact-match "dspark" comparison as fragile against casing drift (e.g. "DSpark"/"Dspark") in a config value that's checkpoint-author-controlled, not a validated enum. Fair point -- every checkpoint verified so far uses lowercase "dspark", but there's no contract guaranteeing that stays true. Also guards against a non-string value (e.g. a malformed config setting the field to a number) instead of crashing on .lower(). 2 new tests: case-variant sweep (DSpark/DSPARK/Dspark all correctly flagged) and non-string value correctly not flagged. 5/5 passing (3 subtests for the case sweep).
…detect-and-reject Supersedes the earlier reject-with-error safety net (this branch's prior two commits) with the actual fix, informed by two independent reference implementations that landed after the safety net went in: - tanth47#2 (fork PR): restructures the draft block to gamma+1-wide, separating the anchor slot from the gamma real draft slots. Confirms the right *shape* of the fix, but does so unconditionally for every DSpark checkpoint -- an unverified risk to the already-working DeepSpec-trained checkpoints (jessiewei7's own diagnosis says these work correctly today under the gamma-wide convention; nothing in that PR's test evidence confirms they still do at the new width). - vllm-project/vllm#47093 (merged, validated against real checkpoints: Qwen3-8B, GLM-5.2-DSpark): the same gamma+1-wide restructuring, but gated by a `dspark_bonus_anchor` config flag -- DeepSpec checkpoints keep the exact behavior they had before the flag existed; speculators-format checkpoints get the wider block. This resolves the regression risk in tanth47's version and is the design this PR ports. Changes, mirroring vLLM's `sample_from_anchor`/`dspark_bonus_anchor` split (vllm/v1/worker/gpu/spec_decode/dspark/speculator.py + vllm/transformers_utils/configs/speculators/algos.py): - dspark_config.py: `_resolve_speculators_proposal_gamma` reads the checkpoint's own authoritative draft length from speculators_config.proposal_methods[i].speculative_tokens, instead of deriving it from block_size - 1 and hoping the convention holds universally (ground-truthed against RedHatAI/GLM-5.2-speculator.dspark: block_size=8, speculative_tokens=7 -- these are NOT the same number). DSparkRuntimeConfig now carries speculators_convention through to the worker. - models/dspark.py: removed the earlier reject-ValueError guard -- DSparkDraftMixin no longer needs to know about this at all, since run_markov_block always receives exactly gamma real draft-hidden slots regardless of which convention produced them. - dspark_draft.py: DraftBlockProposer and DsparkDraftSampler both gain a `bonus_anchor` flag (from speculators_convention) and a `draft_width` property (gamma, or gamma+1 when bonus_anchor). Only the draft forward-pass's own block construction changes width; gamma itself, and everything downstream that reads it (verify window sizing, KV commit, accept-length accounting), is completely unaffected -- this is the surgical, low-blast-radius version of the fix, not a codebase-wide gamma redefinition. Added `.contiguous()` after the anchor-hidden-state slice (a real bug I initially missed: PyTorch's `.view()` calls downstream would raise on the resulting non-contiguous tensor). - dspark_worker_v2.py: threads `speculators_convention` from DSparkRuntimeConfig into `self._bonus_anchor`, passed to both the eager (DraftBlockProposer) and CUDA-graph-folded (DsparkDraftSampler) construction sites. Confirmed DSpark has no separate CUDA-graph-capture runner class (unlike the P-EAGLE bug fixed earlier this session) -- _run_forward is the single source of truth for both eager and graph- replay shapes, so there's no second capture-time site to fix. Rebased twice onto hnyls2002's fast-moving tip during this implementation (the branch moved 17 commits between review-response and this commit, including a refactor that extracted the exact gamma-resolution logic this PR touches into a new resolve_runtime_config() function) -- resolved by extending its DSparkRuntimeConfig with the one new field needed rather than duplicating the resolution logic outside it. 8 new unit tests (gamma resolution from speculators_config vs block_size fallback vs explicit default_proposal_method selection; draft_width for both conventions on both DraftBlockProposer and DsparkDraftSampler), 16/16 passing including the 5 pre-existing detection tests. Not hardware-validated end-to-end (no GPU access to the actual checkpoints this fixes) -- the shape/config-resolution logic is unit-tested, but the real accept-length improvement (jessiewei7's reported 1.1 -> ~4.05) needs confirmation against a real speculators-format DSpark checkpoint before this should be treated as fully proven.
buddywhitman
requested review from
Qiaolin-Yu,
Ying1123,
hnyls2002 and
merrymercy
as code owners
July 13, 2026 06:01
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Open
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
speculators(github.com/vllm-project/speculators)-trained DSpark checkpoints use a different block-slot convention than the DeepSpec-trained onesrun_markov_block(dspark.py) was originally written against:anchor+k+1. The anchor itself (slot 0) is also a real, trained prediction -- the draft block is exactlygammaslots wide, anchor-first.anchor+j. The draft block isgamma + 1slots wide, with slot 0 excluded from both sampling and verification.Loading a speculators-trained checkpoint through the DeepSpec-width path reads every real slot one position early, degrading accept length to ~1 regardless of the underlying model's real speculative quality, with no error or crash - it just
silently produces near-zero speedup.
Diagnosed and confirmed by @jessiewei7: validated against
RedHatAI/GLM-5.2-speculator.dsparkandmgoin/GLM-5.2-speculator.dspark-block16, both showing the ~1.1 accept-length symptom (vs. ~4.05 for the same weights on vLLM).Update: this PR now implements the real fix, not a safety net
The first version of this PR only detected the convention and rejected it with a clear error (the safer of the two fixes offered): "conditional shift on load, or a clear reject-with-error"). It's since been upgraded to the actual fix, informed by two independent reference implementations that appeared after their safety net went in:
gamma+1-wide, separating the anchor from thegammareal draft slots. Confirms the right shape of the fix -- but applies it unconditionally to every DSpark checkpoint, which is an unverified risk to the DeepSpec-trained checkpoints @jessiewei7's own diagnosis says currently work correctly (nothing in that PR's test evidence confirms they still do at the new width).Qwen3-8B,GLM-5.2-DSpark): the samegamma+1-wide restructuring, but gated by adspark_bonus_anchorconfig flag. DeepSpec checkpoints keep exactly the behavior they had before the flag existed; only speculators-format checkpoints get the wider block. This resolves the regression risk in tanth47's version, and is the design this PR ports (mirroringvllm/v1/worker/gpu/spec_decode/dspark/speculator.py'ssample_from_anchorflag andvllm/transformers_utils/configs/speculators/algos.py).Modifications
dspark_config.py:_resolve_speculators_proposal_gammareads the checkpoint's own authoritative draft length fromspeculators_config.proposal_methods[i].speculative_tokens, instead of deriving it fromblock_size - 1and hoping the convention holds universally (ground-truthed againstRedHatAI/GLM-5.2-speculator.dspark:block_size=8,speculative_tokens=7-- these are not the same number).gammaresolution priority: explicitdspark_block_sizeoverride >speculators_config's stated value > plainblock_sizefallback (unchanged DeepSpec path).models/dspark.py: removed the earlier reject-ValueErrorguard.DSparkDraftMixin/run_markov_blockno longer needs to know about this at all - they always receive exactlygammareal draft-hidden slots regardless of which convention produced them.dspark_draft.py:DraftBlockProposerandDsparkDraftSamplerboth gain abonus_anchorflag (fromspeculators_convention) and adraft_widthproperty (gamma, orgamma + 1whenbonus_anchor). Only the draft forward pass's own block construction changes width --gammaitself and everything downstream that reads it (verify window sizing, KV commit, accept-length accounting) are completely unaffected. This is deliberately the surgical, low-blast-radius version, not a codebase-widegammaredefinition.dspark_worker_v2.py: threadsspeculators_conventionfromDSparkRuntimeConfigintoself._bonus_anchor, passed to both the eager (DraftBlockProposer) and CUDA-graph-folded (DsparkDraftSampler) construction sites. Confirmed DSpark has no separate CUDA-graph-capture runner class (unlike a P-EAGLE bug I found and fixed earlier this week in a different PR) -_run_forwardis the single source of truth for both eager and graph-replay shapes, so there's no second capture-time site that could silently diverge.Accuracy Tests
Not hardware-validated end-to-end - I don't have GPU access to the actual checkpoints this fixes. What's verified:
speculators_configvsblock_sizefallback vs explicitdefault_proposal_methodselection;draft_widthfor both conventions on bothDraftBlockProposerandDsparkDraftSampler), 16/16 passing total.[:, 1:, :]) produces a non-contiguous tensor; downstream.view()calls would have raised on it. Added.contiguous()after both slice sites.The real accept-length improvement (@jessiewei7's reported 1.1 → ~4.05) needs confirmation against a real speculators-format DSpark checkpoint before this should be treated as fully proven - happy to help validate, or for a maintainer with hardware access to confirm before merge.
Speed Tests and Profiling
Not applicable to the config/shape-resolution changes here. No hot-path code is touched beyond the one-time 'draft_width` branch already present in the block construction; real speedup validation is the accuracy-test gap above.
Checklist
AI Assistance Disclosure
Implementation assisted by Claude Sonnet 4.6 (Claude Code). I reviewed the diagnosis, the diff, the checkpoint-config verification, and the two reference implementations this ports from, and can explain any part of it.
CI States
Latest PR Test (Base): ❌ Run #29230869752
Latest PR Test (Extra): ❌ Run #29230869606