Conversation
|
cc @benchislett @MatthewBonanni — would appreciate a review when you have a chance. This fixes a Tagging you since this touches the padded-drafter path from #24539, and it's closely related to the cudagraph-padding buffer issue fixed in #50065. |
|
Independent validation of this PR on a second platform. I filed #53488; this fix resolves it here. Setup
Since
Three legs, each the first and only vLLM row of its own boot (this box does not fully return unified memory when a container exits), all launched at MemAvailable 105.1–105.2 GiB. 1. The corruption reproduces here, and the 512/513 boundary reproduces exactly Scoring a fixed 8-chunk corpus with
Length scan over prefixes of the same corpus, each request standalone, HTTP 200 unless noted:
The 2. One symptom worth adding to the issue: it can escalate to HTTP 400 At the 500-token point the unpatched server does not return garbage — it fails outright: The overwritten hidden states can produce NaN logprobs that vLLM's own JSON encoder refuses to serialize. So besides silently wrong numbers, this bug can present as a hard request failure. I hit this twice from independent runs against the same unpatched server — my first scan script died on it before I hardened the scan to record it — so it is repeatable, not a one-off. After the patch, that point returns a normal 3. After the patch the boundary is gone, not moved
To rule out the obvious alternative explanation — that the patched leg simply stopped capturing graphs, or stopped speculating — both legs logged the identical 4. Speculation is fully preserved; no generation-side regression
The non-speculative baseline leg runs at 13.01 / 13.03 / 13.01 tok/s, so speculation is genuinely active on both speculative legs. The tok/s improvement crosses a reboot and a different thermal state (48.6 W vs 35.9 W mean GPU power), so I would not claim the patch makes anything faster — only that nothing regressed. That is the expected shape: the added Caveats, stated plainly
LGTM from my side — this fixes the issue I reported, on hardware and a checkpoint different from the ones the PR was developed against. Disclosure: this validation was run and written up with AI assistance; the hardware, the runs, and the numbers are real and reproducible, and the account owner reviewed the result before posting. |
Signed-off-by: hoobnn <111053672+hoobnn@users.noreply.github.com>
98224f2 to
78e8798
Compare
|
Correction to my validation comment above, plus the discriminating run I said I had not done. 1. One caveat item was wrong. I wrote that — and 2. The discriminating single-variable run has now been done, and the boundary follows the CUDA graph capture limit. Same box (Jetson AGX Thor, sm110a), same image (
Boundary: 256 polluted / 257 clean, against 512 / 513 on the otherwise identical row with the default capture size of 512.
Two honest notes on this run: the corpus and scoring request are bit-identical to my earlier rows, but I had to lower So this correction strengthens the conclusion rather than weakening it: my 3. On the force-push. I re-checked head Separately, and from reading the config code only — I have not measured this: #53183 ("Use MRV2 for all models by default") landed between this PR's old and new base and removed the hybrid/architecture gate in Disclosure: this analysis and the runs behind it were done with AI assistance; the hardware, the runs and the numbers are real and reproducible, and the account owner reviewed the content before posting. Raw artifacts available on request. |
|
One more data point for the reviewers, from the first nightly after #55375 landed —
Two things follow. The corruption is still there on the V1 path on current Full context, including how this relates to the Mamba state-index stride class, is in my comment on #53488: #53488 (comment) Disclosure: the runs and the write-up were done with AI assistance; the hardware, the runs and the numbers are real and reproducible; the account owner reviewed the content before posting. |
Purpose
FIX #53488
When speculative decoding runs a padded GPU drafter (MTP/EAGLE-style),
prompt_logprobsare silently corrupted for requests executed through piecewise CUDA graphs, while the same server without--speculative-configscores every request correctly.Root cause
In the V1 GPU model runner, the padded GPU drafter runs before
_bookkeeping_sync():All CUDA graphs share one global memory pool, so the drafter's forward can reclaim the memory backing the target model's graph output. Prompt logprobs are then computed from draft-model output instead of the target hidden states.
This exactly explains the sharp boundary reported in #53488:
I reproduced this on different hardware from the reporter (RTX 5880 Ada, x86_64, vs. their aarch64 GB10) with one of their exact checkpoints, confirming DEBUG-log
cudagraph_modeflips PIECEWISE→NONE precisely at the corruption boundary (512→513 tokens with--max-cudagraph-capture-size 512).Fix
Add
_get_bookkeeping_hidden_states(): only when (1) a GPU model drafter will run before bookkeeping and (2) the current batch contains a request still needing prompt logprobs, snapshot the unpadded scheduled slice of the target hidden states (hidden_states[:total_num_scheduled_tokens].clone()) before the drafter is enqueued, and pass the snapshot to_bookkeeping_sync().All other paths are zero-cost and unchanged:
Not a duplicate
Draft PR #53506 is explicitly "not a fix": it adds scheduler chunk-accounting test coverage for multi-module MTP prefill lookahead and does not touch the GPU model runner, prompt-logprob computation, or CUDA graph buffer lifetime. The reporter's checkpoint has a single MTP layer (
use_multi_module_mtp()is False), so it does not take that path. No other open PR fixes #53488.Test Plan
Unit (new regression tests, red before the fix / green after):
Static checks:
End-to-end:
cyankiwi/Qwen3.8-27B-AWQ-BF16-INT4(one of the issue's exact checkpoints) on RTX 5880 Ada. Plain vs. MTP ({"method":"mtp","num_speculative_tokens":3}) servers with identical flags (--max-model-len 32768 --no-enable-prefix-caching --enable-chunked-prefill --max-num-batched-tokens 2048 --max-num-scheduled-tokens 2048 --max-cudagraph-capture-size 512). 18 deterministic prompts spanning 448–1714 tokens scored via/v1/completionswith"max_tokens": 1, "temperature": 0, "prompt_logprobs": 0, strictly verifying identical prompt token IDs and greedy output token IDs between servers.Test Result
Unit:
4 passed, 44 deselected. Pre-commit and mypy-3.12: all applicable hooks passed.E2E, MTP/plain mean-NLL ratio per prompt length:
Across all 18 prompts the ratio range goes from
0.99892x – 55.59636x(before) to0.99734x – 1.00194x(after). Plain-server numbers are bit-identical before and after the fix (no regression on the non-speculative path), and greedy generation token IDs remain identical between Plain and MTP servers. The worst case (457 tokens) drops from PPL ≈ 2.2e8 back to 1.41, matching plain.Reproduction script, server logs, and all 72 raw API responses are available on request.
AI assistance was used for reproduction, root-cause analysis, implementation, and testing. I have reviewed every changed line and take responsibility for the change end-to-end.