[Spec] Reject DSpark speculators-convention checkpoints instead of silently degrading accept length - #30852
Closed
buddywhitman wants to merge 3 commits into
Closed
[Spec] Reject DSpark speculators-convention checkpoints instead of silently degrading accept length#30852buddywhitman wants to merge 3 commits into
buddywhitman wants to merge 3 commits into
Conversation
buddywhitman
requested review from
Qiaolin-Yu,
Ying1123,
hnyls2002 and
merrymercy
as code owners
July 11, 2026 09:05
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a check to detect and refuse DSpark checkpoints trained with the 'speculators' convention, preventing silent performance degradation due to unsupported block-slot conventions. It adds a 'speculators_convention' flag to 'DSparkDraftConfig', raises a 'ValueError' during model initialization if the flag is set, and includes unit tests. The reviewer suggested making the 'speculators_model_type' comparison case-insensitive to avoid potential detection failures due to casing differences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…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
force-pushed
the
fix/dspark-speculators-convention
branch
from
July 11, 2026 18:35
b09cedd to
31f2b16
Compare
5 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
3 participants
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.
Superseded by #30982
This PR was automatically closed when its base branch (
sglang-dspark) merged intomainThe identical fix, rebased onto current
main, is open at #30982