[Bugfix][Spec Decode] Keep EAGLE cache registration on the partial-hash-hit path - #52419
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment Once the PR is approved or has the If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban. 🚀 |
cbaf454 to
b13757b
Compare
TheEpicDolphin
left a comment
There was a problem hiding this comment.
Thanks for the fix!
|
✅ @mispa-ms, CI is now available for this PR.
|
|
/ci run |
|
✅ Triggered Buildkite CI #84039 for commit |
…sh-hit path `HybridKVCacheCoordinator.cache_blocks` decides once how far a request may be registered in the prefix-cache hash map. With fine-grained partial hash hits that bound is the raw token count, because a hit no longer has to land on a `scheduler_block_size` boundary. vllm-project#50062 rewrote the EAGLE branch to re-derive its own bound from `num_finalized_computed_tokens` with an unconditional // self.scheduler_block_size * self.scheduler_block_size so the rounding comes back on the one path that had removed it. Registration is then capped at `floor(n / scheduler_block_size) * scheduler_block_size + manager.block_size` instead of `n`, and everything between the last aligned boundary and the tail stops being registered -- `(n % scheduler_block_size) - manager.block_size` tokens per call, which is most of a segment whenever the group's own block is much smaller than the scheduler block. Only EAGLE-family groups take the branch, so a model without speculative decoding never sees it. Measured on Kimi-K3 MXFP4 (hybrid: 24 MLA + 69 KDA layers) on B300, TP8, with DSpark speculative decoding and an agentic replay workload. TokenSpeed MLA sets `block_size=32` and the attention block is forced to 1536 to cover the mamba page, so the cap drops up to 1,503 tokens of every prefix tail (1,472 when `n` is a multiple of 32): | concurrency | before vllm-project#50062 | with vllm-project#50062 | with this fix | |---|---|---|---| | 8 | 5,146 tok/s/GPU | 4,767 (-7.4%) | 5,183 (+0.7%) | | 16 | 7,669 tok/s/GPU | 6,611 (-13.8%) | 7,654 (-0.2%) | GPU prefix-cache hit rate at concurrency 16: 86.3% before, 77.4% with vllm-project#50062, 86.2% with this fix. The non-speculative arm of the same ladder is unaffected in all three (c1 to c78 within -1.3% to +1.8%). Each number is one 60-minute run; the regressed pair was repeated and agreed to within 0.3%. The fix routes both bounds through one helper, `_align_cacheable`, so the exemption cannot be lost again by re-deriving it in a second place. Reachability upstream: `enable_partial_hash_hits` requires a Mamba "align" group with `block_size > hash_block_size`, prefix caching, and `dcp_world_size == 1`. Our own runs enable it under DCP > 1 via a local carry, which is why we hit it first, but a stock `dcp_world_size == 1` hybrid model with EAGLE-family speculative decoding is affected as written. One thing this change does not address, noted for a reviewer: the soundness guard above (`scheduler_block_size >= num_prefill_lookahead`) is justified in its comment by "hits land on scheduler-block boundaries", which is true only when `enable_partial_hash_hits` is off -- `_cache_hit_alignment_tokens` returns `hash_block_size` when it is on. That guard predates this fix and only matters for multi-module MTP on a hybrid Mamba model, a combination we do not run and cannot measure, so it is left alone rather than adjusted on a guess. Test plan: `tests/v1/core/prefix_cache/test_partial_prefix_cache_hits.py:: test_eagle_group_registers_unaligned_tail_under_partial_hash_hits` fails before the change and passes after. `tests/v1/core/test_prefix_caching.py::test_hybrid_cache_blocks_clamped_to_lcm` still passes: it has no Mamba group, so `enable_partial_hash_hits` is False and the helper returns the rounded value exactly as before. Assisted-by: Claude Opus 5 (Anthropic) Signed-off-by: Mi Sun Park <misunp@nvidia.com>
b13757b to
8aa47ec
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #84043 for commit |
|
/ci retry |
|
✅ Queued 1 failed job(s) for retry in Buildkite CI #84043. |
|
CI is green on this PR except
|
…sh-hit path (vllm-project#52419) Signed-off-by: Alessandra005 <aurib032@fiu.edu>
…sh-hit path (vllm-project#52419) Signed-off-by: Wyett <wyettzeng@gmail.com>
Purpose
HybridKVCacheCoordinator.cache_blocksdecides once how far a request may beregistered in the prefix-cache hash map. With fine-grained partial hash hits that
bound is the raw token count, because a hit no longer has to land on a
scheduler_block_sizeboundary.#50062 rewrote the EAGLE branch to re-derive its own bound from
num_finalized_computed_tokenswith an unconditionalso the rounding comes back on the one path that had removed it. Registration is
then capped at
floor(n / scheduler_block_size) * scheduler_block_size + manager.block_sizeinstead ofn, and everything between the last alignedboundary and the tail stops being registered —
(n % scheduler_block_size) - manager.block_sizetokens per call, which is most of a segment whenever thegroup's own block is much smaller than the scheduler block. Only EAGLE-family
groups take that branch, so a model without speculative decoding never sees it.
This routes both bounds through one helper,
_align_cacheable, so the exemptioncannot be lost again by re-deriving it in a second place.
Reachability.
enable_partial_hash_hitsrequires a Mamba"align"groupwith
block_size > hash_block_size, prefix caching, anddcp_world_size == 1.mamba_cache_modeis acache_configknob rather than a model property, so anyhybrid Mamba model served with
--mamba-cache-mode alignand an EAGLE-familydrafter reaches this path; the remaining condition — every group's manager
supporting fine-grained lookup — holds automatically for the
full-attention + Mamba pair.
A stock
dcp_world_size == 1hybrid model with EAGLE-family speculative decodingis affected as written. We run that path under DCP > 1 through a local carry,
which is why we hit it on every run.
Out of scope, flagged for a reviewer. The soundness guard
scheduler_block_size >= num_prefill_lookaheadis justified in its comment by"hits land on scheduler-block boundaries", which holds only when
enable_partial_hash_hitsis off —_cache_hit_alignment_tokensreturnshash_block_sizewhen it is on. That guard predates this change and only mattersfor multi-module MTP on a hybrid Mamba model, a combination we do not run and
cannot measure, so it is left alone rather than adjusted on a guess.
Not a duplicate
No open PR addresses this. The nearest neighbours, checked before opening:
the closest in subject but a proposal for different hashing, not this bug
AI assistance
This change was developed with AI assistance (Claude). I reviewed every changed
line, ran the tests below myself, and can defend the change end to end.
Test Plan
pytest tests/v1/core/prefix_cache/test_partial_prefix_cache_hits.py -k eagle_group_registers_unaligned_tail pytest tests/v1/core/prefix_cache/ pytest tests/v1/core/test_prefix_caching.py -k clamped_to_lcm pytest tests/v1/core/ # before/after, to check nothing else movesEnd to end: Kimi-K3 MXFP4 (hybrid, 24 MLA + 69 KDA layers), 8x B300, TP8, DSpark
speculative decoding with
num_speculative_tokens=7, agentic replay workload,60 minutes per point.
Test Result
The new test fails on the parent commit (
acb0f1dcdb) and passes with the fix.The failure is the arithmetic above: the EAGLE group registers
floor(22/8)*8 + 2 = 18tokens where it should register 22.tests/v1/core/in full, same command on both trees: +1 passed, −1 failed,no other test changes state. The one that moves is the new test. My sandbox
cannot import the compiled extensions, so a large fixed set of tests in that
directory errors out identically on both trees and is not informative here;
the targeted suites above are the ones that run clean.
End to end, one 60-minute run per cell:
GPU prefix-cache hit rate at concurrency 16: 86.3% → 77.4% → 86.2%. The
non-speculative arm of the same ladder is unaffected throughout (13 points from
concurrency 1 to 78, all within −1.3% to +1.8%). The regressed pair was repeated
and the two runs agreed to within 0.3%.
Effect on output
The change only widens which already-computed tokens may be registered in the
hash map; it does not alter what is computed, what a hit returns, or the
correctness conditions #50062 added (
num_reprefillable_tokensstill excludesre-prefillable tail tokens, and the EAGLE last-block drop is untouched). Output
is bit-identical in the sense that no sampling path changes; the measured effect
is throughput and prefix-hit rate only. No accuracy evaluation was run, and none
is claimed.
For scale on this model:
TOKENSPEED_MLAsetsblock_size=32and the attentionblock is forced to 1536 to cover the mamba page. The unregistered part of a
prefix tail is
(n % 1536) - 32, so up to 1,503 tokens go unregistered percall — 1,472 when
nis a multiple of 32.