Skip to content

[KV Connector][NIXL] Opt-in whole-row regions for replicated BLHNC layers - #60564

Closed
stu-cao wants to merge 13 commits into
vllm-project:mainfrom
stu-cao:fix/nixl-blhnc-whole-row-optin
Closed

stu-cao wants to merge 13 commits into
vllm-project:mainfrom
stu-cao:fix/nixl-blhnc-whole-row-optin

Conversation

@stu-cao

@stu-cao stu-cao commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #58471 and #57662 (both open). This branch carries their commits. Only the last three commits belong to this PR (+328/−19 across base_worker.py, test_nixl_connector.py and the connector docs):

  • 57978684fc: opt-in whole-row regions;
  • edb4a3c77b: reject draft pages packed into the target row;
  • 9adc138cfc: let the test fake take a mixed-spec group.

Reviewing #57662 and #58471 first will shrink this diff to those three commits.

Purpose

Fixes #60561.

Since #53781, a device-resident BLHNC MLA cache registers one NIXL region per layer, because page_contiguous is true for every layer. For GLM-5.3 that is 99 descriptors per block instead of one, so BLHNC transfers cost more than LBHNC's (104 vs 62 strides on a decode rank).

This PR adds kv_connector_extra_config.whole_row_regions (default false):

Why #57662 is needed. Under BLHNC every KV cache group starts at byte 0 of the shared row, so the first draft page aliases the first target page's base address. Without #57662's (base_addr, block_len) key, the draft region folds into a target region. The rebase keeps main's rule that a packed PP producer never dedups aliased regions.

Not a duplicate:

None of them restores whole-row registration for page-contiguous caches in pull mode.

Separate issue: the DFlash2 acceptance drop seen on BLHNC deployments (#60560) does not come from NIXL registration. It comes from the hybrid KV cache manager's sliding-window draft group, and is fixed by #60767. This PR neither causes nor fixes it.

Test Plan

pytest tests/v1/kv_connector/unit/test_nixl_connector.py tests/v1/kv_connector/unit/test_nixl_desc_geometry.py tests/v1/kv_connector/unit/test_tp_mapping.py

New tests:

  • test_blhnc_gqa_draft_reads_own_head_slice_from_tp1_prefill, parametrized over whole_row in {False, True} and tp_rank in {0, 3}. A TP1 prefill and a TP4 decode each register a BLHNC MLA target plus a GQA draft. With whole-row on, the target row must be one region. Every draft layer must be its own region and read its own head slice.
  • test_whole_row_regions_rejects_host_buffer.
  • test_whole_row_regions_rejects_draft_packed_into_target_row.

End to end: GLM-5.3 (NVFP4) with a DFlash2 draft on 2× B300, DEP8 prefill to 2× TP4 decode over NIXL (RDMA), BLHNC block size 64, whole_row_regions on both roles, agentic multi-turn traffic (AgentX) at 32 concurrent sessions.

Test Result

Unit tests. One B300 GPU, test_nixl_connector.py, test_nixl_desc_geometry.py and test_tp_mapping.py. Each tree was built with VLLM_USE_PRECOMPILED=1 on the kernels of nightly 51ef239768.

  • main (81d9c45e4b): 424 passed, 2 failed.
  • [Bugfix][KV Connector][NIXL] Shard a GQA draft's KV by head under an MLA target #58471 rebased: 438 passed, 2 failed.
  • This PR's head: 445 passed, 2 failed.
  • Same two failures everywhere. test_abort_timeout_on_prefiller[None] and [ray] start a real engine that wants 50% of GPU memory, and only 17.6 of 268 GiB were free on the shared GPU. No failures are introduced by this PR.
  • New tests (-k "blhnc_gqa_draft or whole_row_regions"): 6 of 6 pass.
  • Lint: ruff and mypy pass on the changed files.

End to end, with this PR's code. These are from the B300 runs above. The NIXL connector's own metrics are summed over the profiling window of each run (~5,700 transfers each):

BLHNC, block 64 regions descriptors / transfer descriptors / MB transfer time / GB
whole_row_regions on (4 runs) target row: 1; draft: per layer 1,309 – 1,860 0.48 – 0.54 48 – 69 ms
per-layer regions 1 per layer 46,692 49.3 77 ms

The per-layer row ran with --disable-hybrid-kv-cache-manager, which changes what is transferred (decode prefix hits rise and less KV is pulled), so its transfer time is indicative only. The descriptor reduction is the direct effect of this PR.

Earlier variant, for context. On GB200 (128 sessions), a predecessor of this change produced:

  • Registration: 1 stride / 26,104 descriptors on a decode rank, vs 62 / 2,488,080 with LBHNC block 1024.
  • Transfer time: mean NIXL transfer time 85.23 → 22.30 ms.
  • TTFT: p50 848 → 730 ms.

That predecessor also folded the head-sharded draft into the whole-row region, which this PR deliberately avoids, so these are not this PR's numbers. The target's registration is the same; this PR adds the draft's per-layer regions on top.

Not covered: push mode (main's packed-MLA push path is unchanged) and non-MLA models (the flag does nothing there).

AI assistance

AI assistance (Claude) was used to write the code, tests and this description, and to run and analyze the experiments. @stu-cao reviewed the change and is responsible for it.

🤖 Generated with Claude Code

@mergify

mergify Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Documentation preview: https://vllm--60564.org.readthedocs.build/en/60564/

@mergify mergify Bot added documentation Improvements or additions to documentation kv-connector labels Oct 8, 2026
@mergify

mergify Bot commented Oct 8, 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, @stu-cao.

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 8, 2026
@stu-cao
stu-cao marked this pull request as ready for review October 8, 2026 08:12

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

@stu-cao

stu-cao commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Test results on the final head 4f86d7da69, vs base c0a10889e0 (#58471 head).

Setup: one B300 GPU; vLLM built from each tree with VLLM_USE_PRECOMPILED=1 (merge-base nightly 711fc55c10).

pytest tests/v1/kv_connector/unit/test_nixl_connector.py -k "blhnc_gqa_draft or whole_row_regions"
  head: 6 passed
  rejection test with e75df93337's base_worker.py (no guard): DID NOT RAISE ValueError (fails, as intended)

pytest tests/v1/kv_connector/unit/test_nixl_connector.py
  base: 153 passed, 3 failed
  head: 159 passed, 3 failed   (same 3 failures as base; no regressions)

All three failures are environmental:

  • test_kv_transfer_handshake: a live engine held the inherited VLLM_NIXL_SIDE_CHANNEL_PORT. It passes on both trees with that variable unset.
  • test_abort_timeout_on_prefiller[ray] / [None]: they start a real engine that wants 50% of GPU memory, and only 17.6 of 268 GiB were free on the shared GPU.

stu-cao and others added 13 commits October 8, 2026 21:35
NIXL builds one TPMapping per remote engine from the model-wide topology.
For an MLA target that mapping treats every region as replicated
(rank_offset_factor=0, total_num_kv_heads=1), including the KV regions of
a GQA draft such as DFlash, which are split by head across TP. With
heterogeneous TP (e.g. prefill TP1 -> decode TP8) this:

- rejects the correct draft geometry at handshake ("KV cache sizes must
  match between P and D when replicated"), since model_replicated is
  forced by use_mla, while accepting an unscaled draft block;
- reads draft head 0 on every decode rank once past the check, which
  collapses draft acceptance (output stays correct, the target verifies);
- skips the hetero-TP layout guards for the draft because use_mla is set.

Resolve the draft's KV head count from speculative_config when an MLA
target runs a non-MLA draft, and build a second TPMapping with
compute_tp_mapping(head_sharded_kv_heads=...) for the draft's SPLIT
regions. Reads, handshake size checks and layout guards use it for those
regions only; MLA regions are unchanged. Prefill TP > decode TP and DCP
are rejected with NotImplementedError for such drafts. CSA-linear and
mamba models, and MLA drafts such as MTP, keep today's behaviour.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
…t layout

With BLHNC one page holds every layer of a block, so a region's stride is
the page rather than its block_len. Cover that the draft head offset still
lands inside the draft layer of each page across several blocks.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
…t helper

Replace the **overrides passthrough in _compute_mapping with an explicit
keyword, matching the helper's other parameters.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
- Branch once on replicated in _build_fa_remote instead of two ternaries,
  and index the draft mapping directly: it always exists for draft regions.
- Replace the compute_tp_mapping Args block with one sentence.
- Trim comments that restated the code or the error message.
- Test _resolve_head_sharded_draft_kv_heads, which gates the fix.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
…y check

Two fixes for review comments on vllm-project#58471 (not yet posted):

1. The new "head-sharded draft under prefill TP > decode TP" guard used
   tp_ratio's sign directly, but tp_ratio is purely local-vs-remote and which
   side is local flips between pull (local=decode) and push (local=prefill,
   since prefill is the side that needs decode's write-target addresses).
   Reusing the pull-mode sign check in push mode would reject the common
   P_TP < D_TP topology instead of the one it's meant to catch. Derive the
   condition from _TRANSFER_MODE instead.

2. _is_head_sharded_draft_region inferred "SPLIT under a fully-MLA target"
   implies draft purely by exclusion. Track each SPLIT region's own KV head
   count at registration (mirroring _region_is_mla) and assert it's
   consistent with the resolved draft's head count, tying the classification
   to an actual per-layer model property instead of the absence of one.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Signed-off-by: Stu Cao <stu@primeintellect.ai>
_is_head_sharded_draft_region now reads worker._region_num_kv_heads,
so the object.__new__-based worker fixture in
test_overlaid_transfer_groups_share_region_geometry needs the
attribute initialized alongside _region_is_mla or registration raises
AttributeError.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Signed-off-by: Stu Cao <stu@primeintellect.ai>
…ed push

Signed-off-by: Stu Cao <stu@primeintellect.ai>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
ruff E501.

Signed-off-by: Stu Cao <stu@primeintellect.ai>
Region registration deduplicated NIXL transfer regions by base address
alone, and the collapsed branch only refreshed block geometry for MLA
regions. When a hybrid-attention HMA layer with a different block length
aliases another layer's base address (DeepSeek-V4.1-Flash compressor
state_cache over the swa_cache it serves, and its indexer k_cache), the
collapsed layer silently inherited the first region's block length, so
its transferred state was truncated and misaligned — producing garbled
output in P/D disaggregated serving while identical-geometry models ran
fine.

Deduplicate by (base_addr, block_len) so regions with distinct transfer
geometry stay separate; identical-geometry aliases still fold as before.

Co-authored-by: ZCode <noreply@zcode.ai>
Co-authored-by: chaterm <chaterm.dev@gmail.com>
Signed-off-by: 葛昊宇 <haoyu_ge@intsig.net>
…yers

vllm#53781 made per-layer regions the default whenever a layer's page is
contiguous, which a HiSparse decoder needs. For a device-resident BLHNC cache
it costs one descriptor per layer per block (99 for GLM-5.3 instead of 1).

Add kv_connector_extra_config.whole_row_regions (default false): every
replicated (MLA) layer of a block-major row registers as one whole-row region.
Head-sharded layers, such as a GQA draft under an MLA target, never take the
whole-row branch: they keep per-layer SPLIT regions so each TP rank reads its
own head slice of the prefill page. Host-staged buffers and host-resident
groups are rejected.

Builds on vllm-project#58471 (draft head slicing) and vllm-project#57662 (regions keyed by
(base_addr, block_len)); a block-outermost layout starts every KV cache group
at byte 0 of the shared row, so the first draft page aliases the first target
page's base address.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
…the target row

With the hybrid KV cache manager disabled, a block-outermost layout packs the
head-sharded draft pages into the target's KV cache group and block row. Their
size depends on the TP size, so a whole-row region would differ between a TP1
prefill and a TP4 decode and fail the handshake later ("KV cache sizes must
match between P and D when replicated"). Fail at registration instead, naming
the layer and group.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
FakeNixlConnectorWorker derives its reference KV shape from the first KV cache
group's spec, which has no head count when that group mixes specs
(UniformTypeKVCacheSpecs). Use the group's first spec, as a packed
block-outermost layout has, so the packed-draft rejection test can register.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
@stu-cao
stu-cao force-pushed the fix/nixl-blhnc-whole-row-optin branch from 4f86d7d to 9adc138 Compare October 9, 2026 16:55
@mergify

mergify Bot commented Oct 9, 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, @stu-cao.

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

@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 PR and sry for the delayed review! IIUC, #61074 fixed the whole row issue and can supersede this PR. #57662 may still be needed though but that one seems still ongoing

@stu-cao

stu-cao commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

@ivanium Yes, I validated the fix is already in main

┌────────────────────┬────────────────────────────┬───────────┬─────────────┬────────────┬─────────┐
│        Run         │    Regions per worker      │ Transfers │ Descriptors │    Per     │   MB    │
│                    │     (prefill / decode)     │           │             │  transfer  │  moved  │
├────────────────────┼────────────────────────────┼───────────┼─────────────┼────────────┼─────────┤
│ main (#61074),     │ 1 / 1                      │       192 │         432 │        2.2 │  37,671 │
│ BLHNC              │                            │           │             │            │         │
├────────────────────┼────────────────────────────┼───────────┼─────────────┼────────────┼─────────┤
│ #60564 rebuild,    │ 1 / 1                      │       192 │         420 │        2.2 │  37,671 │
│ flag off           │                            │           │             │            │         │
├────────────────────┼────────────────────────────┼───────────┼─────────────┼────────────┼─────────┤
│ #60564 rebuild,    │ 1 / 1                      │       192 │         436 │        2.3 │  37,671 │
│ flag on            │                            │           │             │            │         │
├────────────────────┼────────────────────────────┼───────────┼─────────────┼────────────┼─────────┤
│ main, LBHNC        │ 27 / 27                    │       192 │      12,420 │       64.7 │  37,671 │
│ (control)          │                            │           │             │            │         │
└────────────────────┴────────────────────────────┴───────────┴─────────────┴────────────┴─────────┘

Closing as superseded by #61074

@stu-cao stu-cao closed this Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation kv-connector needs-rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Performance][NIXL] BLHNC MLA caches lost whole-row registration in #53781 and now need more NIXL descriptors than LBHNC

3 participants