Skip to content

[Bugfix][UMBP] Copy every KV cache layout's per-head runs - #64677

Merged
ivanium merged 15 commits into
vllm-project:mainfrom
AndreasKaratzas:akaratza_umbp_kv_layout_segments
Nov 4, 2026
Merged

ivanium merged 15 commits into
vllm-project:mainfrom
AndreasKaratzas:akaratza_umbp_kv_layout_segments

Conversation

@AndreasKaratzas

@AndreasKaratzas AndreasKaratzas commented Oct 30, 2026 •

Copy link
Copy Markdown
Member
  • Split each layer's page into per-head runs in data.py, using the allocator's compute_layout_strides.
  • Register the same runs in the embedded.py bulk path, falling back to per-range copies for a group whose stride doesn't match.
  • Fail at startup instead of corrupting KV when a page is split and kv_cache_layout is unset.
  • Round-trip all 6 layouts through the bulk and range paths in test_umbp_embedded.py.

UMBP took cache.stride(0) as the block stride and copied one contiguous page per layer, both on the range path in data.py and on the bulk path in _register_load_layouts, as noted in the review of #61794. Under LHBNC, the default with ROCM_AITER_FA and VLLM_ROCM_SHUFFLE_KV_CACHE_LAYOUT=1, K and V are head planes spanning every block, so the engine fails at init with KV region bytes (65536) exceed its stride (32768), and under BHLNC heads of different layers interleave inside a block row, so UMBP silently restores the wrong KV. pytest -q v1/kv_connector/unit/test_umbp_embedded.py -k every_kv_cache_layout gives 4 failures without the fix and 12 passed with it. On MI350X with Qwen3-0.6B, 16 greedy tokens after a 192-token UMBP hit now match recompute under BHLNC (TRITON_ATTN with VLLM_KV_CACHE_LAYOUT=BHLNC) and under AITER shuffle-KV, and stay identical under the default layouts; the batch-invariant tests in test_mori_token_e2e.py likewise fail without the fix and pass with it under VLLM_KV_CACHE_LAYOUT=BHLNC and LHBNC. Batch invariance rules out ROCM_AITER_FA, so those tests run on TRITON_ATTN, and AITER shuffle-KV is covered only by the greedy check above. Other layouts and MLA keep the single-span copy, and refusing LHBNC and BHLNC instead would make UMBP unusable with AITER shuffle-KV, since forcing LBHNC trips the shuffle decode path. LHBNC and BHLNC now issue one bulk range per head instead of one per layer, which I haven't benchmarked yet, and no open PR touches UMBP layout handling.

AI assistance (Claude Code) was used to draft this change. Every line was reviewed by the submitter.

charxwu and others added 14 commits October 29, 2026 09:05
Add embedded runtime contract tests, MORI GPU integration, token-level restore coverage, reusable test helpers, user documentation, and AMD CI wiring.

Signed-off-by: CharlesWu <Charles.Wu@amd.com>
Co-authored-by: Yichao Zhu <Yichao.Zhu@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

(cherry picked from commit d61f813)
(cherry picked from commit 967175e)
(cherry picked from commit 68cef07)
Verify completed stores do not retain publication-key history.
Use native residency and byte checks for eviction coverage.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: CharlesWu <Charles.Wu@amd.com>
(cherry picked from commit 16c79a7)
(cherry picked from commit c4b05e1)
Move the scheduler factory fix into the embedded test slice so its lookup instance matches the workers at this PR head.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: CharlesWu <Charles.Wu@amd.com>
The TP2 x PP2 native roundtrip launches four ranks. Use the existing
mi355_4 pool and request four GPUs for this integration step.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: CharlesWu <Charles.Wu@amd.com>
Install gRPC and Protobuf dependencies in both ROCm image bases, request
BUILD_UMBP=ON, and reject silently disabled builds through the CMake cache.
Check UMBPClient and four visible GPUs before running the integration suite
so a missing native backend cannot turn module skips into a green job.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: CharlesWu <Charles.Wu@amd.com>
Keep the added embedded unit and GPU integration tests independent of
Buildkite's long TMPDIR, including multiprocess workers and token restore.
Share the existing short directory fixture across connector test suites.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: CharlesWu <Charles.Wu@amd.com>
Remove the module-wide ROCm/MORI skip from the CPU contract suite so the
four regressions merged with the embedded implementation continue to run.
All 52 cases pass without MORI; native GPU tests retain their own guards.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: CharlesWu <Charles.Wu@amd.com>
Run the integration suite with the image's pytest instead of a separate
virtualenv. Fail collection when MORI lacks UMBPClient and fail the
TP2 x PP2 round-trip when fewer than four GPUs are visible, so a
misconfigured image or agent still cannot pass through skips.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: CharlesWu <Charles.Wu@amd.com>
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
- Docker: install gRPC/Protobuf build deps only in build_mori; final gets
  the runtime libs and a UMBPClient import check; drop no-op BUILD_UMBP.
- CI: single-GPU integration group in test_areas and test-amd.yaml, with
  complete source dependencies.
- Tests: drop tautological and fake-only tests, use a fake-clock fixture
  for timeouts, move the hybrid-tail restore into the unit suite with a
  non-periodic fill, rewrite the e2e restore with vllm_runner and an
  external-hit assertion, and gate on ROCm + UMBP via pytestmark.
- Accuracy: add a ROCm-gated UMBP GSM8K row that asserts the reload.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
…eduler

- e2e: under VLLM_BATCH_INVARIANT=1 a UMBP restore must reproduce the cold
  run's tokens and logprobs, for a batch ending mid-block and for a restore
  that extends a shorter GPU prefix hit, with exact external-hit counts.
- unit: an object evicted between lookup and load must fail or recompute
  per kv_load_failure_policy through the real Scheduler, for one and two
  KV cache groups, and never leave a stale prefix-cache hit.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
KVLayoutPlanner and the embedded bulk path assumed a layer's block page is
one contiguous run. Under LHBNC and BHLNC a page is one run per head, so
UMBP failed at init under LHBNC (AITER FA with shuffle-KV) and silently
restored the wrong bytes under BHLNC. Derive the runs from the same layout
strides the allocator uses, copy them in both paths, and fail closed when
the layout is unknown and the page is not one run.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
@mergify mergify Bot added ci/build rocm Related to AMD ROCm bug Something isn't working labels Oct 30, 2026
@mergify mergify Bot added the kv-connector label Oct 30, 2026
@github-project-automation github-project-automation Bot moved this to Todo in AMD Oct 30, 2026
@mergify

mergify Bot commented Oct 30, 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, @AndreasKaratzas.

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 Oct 30, 2026
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
@mergify mergify Bot removed the needs-rebase label Oct 30, 2026
@AndreasKaratzas
AndreasKaratzas marked this pull request as ready for review October 30, 2026 18:57

@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.

@AndreasKaratzas

Copy link
Copy Markdown
Member Author

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #98771 for commit f217b63ac798.

@charxwu

charxwu commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the fix, @AndreasKaratzas! I re-ran this PR on MI350X (gfx950) with vLLM built from f217b63 (0.33.0rc1.dev401+gf217b63ac), Qwen3-0.6B. As the baseline I used the same build with umbp/data.py and umbp/runtime/embedded.py taken from 913fccd, so the two runs differ only by this PR. The fix checks out on my side:

  • test_every_kv_cache_layout_restores_blocks_exactly: 4 failures (LHBNC/BHLNC × bulk/ranges) without the fix, 12 passed with it, and all 242 UMBP unit tests pass with the fix.
  • test_mori_token_e2e.py: with VLLM_KV_CACHE_LAYOUT=BHLNC both tests fail without the fix (restored tokens differ from token 0) and pass with it. With LHBNC, init fails without the fix (KV region bytes (65536) exceed its stride (8192)) and both tests pass with it.

One small thing about the coverage note in the description, unless I'm missing a setup step: I don't think the batch-invariant tests in test_mori_token_e2e.py can reach ROCM_AITER_FA. That means they don't exercise the AITER shuffle-KV (LHBNC) case, even with VLLM_ROCM_USE_AITER=1 VLLM_ROCM_SHUFFLE_KV_CACHE_LAYOUT=1.

As far as I can tell:

On the build above, running test_mori_token_e2e.py with those two env vars logs the following, once per test:

Found incompatible backend(s) [ROCM_AITER_FA, ROCM_AITER_UNIFIED_ATTN, TURBOQUANT, ROCM_OCTAVE, ULTRAQUANT] with AttentionType.DECODER. Overriding with TRITON_ATTN out of potential backends: ['TRITON_ATTN'].
Using LBNHC KV cache layout.

Both tests pass with and without the fix. Forcing the backend with VLLM_BATCH_INVARIANT=1 gives ValueError: Selected backend AttentionBackendEnum.ROCM_AITER_FA is not valid for this configuration. Reason: ['batch invariance not supported'], while the same call with VLLM_BATCH_INVARIANT=0 starts on ROCM_AITER_FA. So I read the "pass with it under both" part as showing the fix doesn't break the TRITON_ATTN path, rather than as coverage of AITER shuffle-KV.

For AITER shuffle-KV itself, I ran a non-batch-invariant check modeled on your description, with attention_backend="ROCM_AITER_FA", VLLM_ROCM_USE_AITER=1 and VLLM_ROCM_SHUFFLE_KV_CACHE_LAYOUT=1. It used 3 prompts of 192 tokens plus a partial block, and 16 greedy tokens with logprobs=5. The layout resolves to LHBNC there. Without the fix the engine fails at init with KV region bytes (65536) exceed its stride (32768), matching the description. With the fix, all 576 prefix tokens are restored from UMBP, and the output matches a local prefix-cache hit token-for-token with identical logprobs on all 3 prompts. Since AITER FA isn't batch invariant, I compared against a local prefix-cache hit rather than recompute. That way both runs take the same cached-prefix path and differ only in where the KV bytes come from.

@AndreasKaratzas

Copy link
Copy Markdown
Member Author

Thanks for rerunning this and for the AITER shuffle-KV check! You're right: the fixture forces VLLM_BATCH_INVARIANT=1, which rules out ROCM_AITER_FA, so those e2e tests only exercise BHLNC/LHBNC through TRITON_ATTN. I've corrected that sentence in the description.

@ivanium ivanium left a comment

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.

Thanks for the fix!

@ivanium ivanium added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 4, 2026
@ivanium

ivanium commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

/ci run

@ivanium
ivanium enabled auto-merge (squash) November 4, 2026 23:51
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

✅ CI is already running for this commit: https://buildkite.com/vllm/ci/builds/98771

@ivanium
ivanium merged commit 575cc1f into vllm-project:main Nov 4, 2026
49 of 50 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in AMD Nov 4, 2026
@AndreasKaratzas
AndreasKaratzas deleted the akaratza_umbp_kv_layout_segments branch November 4, 2026 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ci/build kv-connector ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants