Skip to content

[Bugfix][KV Offload] Scale UniformTypeKVCacheSpecs groups by DCP - #50883

Merged
orozery merged 1 commit into
vllm-project:mainfrom
drakosha:fix-offloading-dcp-uniform-spec
Sep 2, 2026
Merged

orozery merged 1 commit into
vllm-project:mainfrom
drakosha:fix-offloading-dcp-uniform-spec

Conversation

@drakosha

@drakosha drakosha commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Purpose

#49964 gated the DCP scaling of a group's token span on isinstance(group.kv_cache_spec, AttentionSpec) so that Mamba groups stay unscaled. UniformTypeKVCacheSpecs is a container rather than an AttentionSpec subclass, so DSA models — whose indexer and MLA layers merge into a single uniform group — lost the DCP factor too:

  • build_offloading_config reports tokens_per_block = block_size (64),
  • resolve_kv_cache_block_sizes still returns block_size * dcp (128) for a single group,

so enabling CPU offload under DCP dies at startup:

AssertionError: tokens_per_block=64 not divisible by tokens_per_hash=128.
Hybrid models (e.g. Mamba+Attention) need --enable-prefix-caching to align block sizes.

Reported by @Leoyzen on GLM-5.2, TP8/DCP2, fp8_ds_mla, MTP, OffloadingConnector: #46514 (comment)

The scaling is now keyed off a helper that unwraps the container, so Mamba groups stay unscaled and every attention group keeps its DCP span. Same call site is fixed in resolve_kv_cache_block_sizes for multi-group configs.

Related: #50823 fixes the other half of the same container-not-unwrapped family (block table width).

Test

Three tests, one per call site plus the hybrid case, next to the Mamba tests from #49964:

  • offloading_connector/test_config.py::test_dcp_scales_uniform_type_attention_group_blocks
  • offloading_connector/test_config.py::test_dcp_scales_uniform_type_group_alongside_mamba
  • core/test_kv_cache_utils.py::test_resolve_block_sizes_scales_uniform_type_group_by_dcp

Verified fail→pass in a container built from the current nightly (0.26.1rc1.dev255+g5e35a6f4f); each test fails on unpatched main with the reported assert (tokens_per_block=16 not divisible by tokens_per_hash=32) or a wrong span, and the two suites pass with the fix:

python3 -m pytest tests/v1/core/test_kv_cache_utils.py \
  tests/v1/kv_connector/unit/offloading_connector/test_config.py -q
# 115 passed

Config-level change, no effect on model output; no eval run.

AI assistance (Claude) was used for this work; all changes reviewed.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify

mergify Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @drakosha.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify

mergify Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @drakosha.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify

mergify Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @drakosha.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify

mergify Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @drakosha.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Aug 29, 2026
@cjackal

cjackal commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Seems superceded bywe can reuse the resolve_dcp_kv_block_size from the commit 43bb773 in #53324 (which has already been merged)

* (
parallel_config.decode_context_parallel_size
if isinstance(group.kv_cache_spec, AttentionSpec)
if is_kv_cache_spec_dcp_sharded(group.kv_cache_spec)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole tokens_per_block calculation is now doable with tokens_per_block=resolve_dcp_kv_block_size(group.kv_cache_spec, parallel_config.decode_context_parallel_size).

LucasWilkinson pushed a commit to neuralmagic/vllm that referenced this pull request Aug 31, 2026
isinstance(spec, AttentionSpec) so that Mamba groups stay unscaled.
UniformTypeKVCacheSpecs is a container rather than an AttentionSpec
subclass, so DSA models, whose indexer and MLA layers merge into one
uniform group, stopped being scaled as well: build_offloading_config
reports tokens_per_block=block_size while resolve_kv_cache_block_sizes
still returns block_size * dcp for a single group, and enabling CPU
offload under DCP dies on the divisibility assert.

Key the scaling off a helper that unwraps the container, so Mamba stays
unscaled and every attention group keeps its DCP span.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Mikhail Kostryukov <mike@triptrack.net>
LucasWilkinson added a commit to neuralmagic/vllm that referenced this pull request Aug 31, 2026
…cache spec

Signed-off-by: Lucas Wilkinson <lwilkinson@neuralmagic.com>
LucasWilkinson pushed a commit to neuralmagic/vllm that referenced this pull request Aug 31, 2026
isinstance(spec, AttentionSpec) so that Mamba groups stay unscaled.
UniformTypeKVCacheSpecs is a container rather than an AttentionSpec
subclass, so DSA models, whose indexer and MLA layers merge into one
uniform group, stopped being scaled as well: build_offloading_config
reports tokens_per_block=block_size while resolve_kv_cache_block_sizes
still returns block_size * dcp for a single group, and enabling CPU
offload under DCP dies on the divisibility assert.

Key the scaling off a helper that unwraps the container, so Mamba stays
unscaled and every attention group keeps its DCP span.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Mikhail Kostryukov <mike@triptrack.net>
LucasWilkinson added a commit to neuralmagic/vllm that referenced this pull request Aug 31, 2026
…cache spec

Signed-off-by: Lucas Wilkinson <lwilkinson@neuralmagic.com>
build_offloading_config() decides DCP scaling with
isinstance(spec, AttentionSpec) so that Mamba groups stay unscaled.
UniformTypeKVCacheSpecs is a container rather than an AttentionSpec
subclass, so DSA models, whose indexer and MLA layers merge into one
uniform group, are not scaled either: the connector reports
tokens_per_block=block_size while resolve_kv_cache_block_sizes() returns
block_size * dcp for the same group, and enabling CPU offload under DCP
dies on the divisibility assert.

resolve_kv_cache_block_sizes() already answers this question with
resolve_dcp_kv_block_size(), which unwraps the container. Use that helper
on the offloading side too, so both sides agree by construction and Mamba
groups keep their unscaled per-rank span.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Mikhail Kostryukov <mike@triptrack.net>
@drakosha
drakosha force-pushed the fix-offloading-dcp-uniform-spec branch from 02b320d to 6567ad1 Compare September 2, 2026 07:10
@drakosha

drakosha commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@cjackal You are right, and half of this PR is now redundant: resolve_kv_cache_block_sizes
goes through resolve_dcp_kv_block_size since #53324, so my predicate duplicated yours. Dropped
it, and my test_kv_cache_utils case with it.

What is left is the connector side: build_offloading_config still scales by
isinstance(spec, AttentionSpec), and UniformTypeKVCacheSpecs is a container, not a subclass.
On DSA models, where indexer and MLA merge into one uniform group, the connector reports
tokens_per_block=block_size while the scheduler reports block_size * dcp, and CPU offload
under DCP dies on the divisibility assert. Rebased on c00091e026: nine lines in that file
calling your helper, plus the connector test. On nightly a9a17e709 with current-main files
overlaid, test_config.py is 47 passed with the fix, 2 failed (both dcp=2) without it.

Aside: resolve_dcp_kv_block_size treats a group as sharded when every layer spec is an
AttentionSpec, so sliding window scales, while dcp_world_size_for_kv_cache_spec answers the
same question with FullAttentionSpec only and unwraps containers by next(iter(...)). Not
this PR's business, but they disagree.

AI assistance was used for this work.

@drakosha

drakosha commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

@drakosha, A reviewer with write access must run /ci run, approve the PR, or add the ready label first.

@mergify mergify Bot removed the needs-rebase label Sep 2, 2026
@orozery orozery added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 2, 2026
@orozery

orozery commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

/ci run

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

@drakosha, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
  • /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
  • /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #86859 for commit 6567ad1d8ecd.

@orozery
orozery merged commit ba6c60e into vllm-project:main Sep 2, 2026
31 checks passed
mylibrar pushed a commit to tanyuqian/vllm that referenced this pull request Sep 3, 2026
…m-project#50883)

Signed-off-by: Mikhail Kostryukov <mike@triptrack.net>
Co-authored-by: Claude <noreply@anthropic.com>
sheralskumar pushed a commit to sheralskumar/vllm that referenced this pull request Sep 8, 2026
…m-project#50883)

Signed-off-by: Mikhail Kostryukov <mike@triptrack.net>
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working kv-cache-manager kv-connector ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants