Skip to content

[Bugfix][HiSparse][PD] Align region-mapped pulls across logical block sizes - #57077

Merged
NickLucche merged 3 commits into
vllm-project:mainfrom
NickLucche:fix/nixl-region-page-alignment
Sep 16, 2026
Merged

NickLucche merged 3 commits into
vllm-project:mainfrom
NickLucche:fix/nixl-region-page-alignment

Conversation

@NickLucche

@NickLucche NickLucche commented Sep 15, 2026 •

Copy link
Copy Markdown
Member

Purpose

Fix a NIXL pull that can silently load the wrong prompt pages when P and D allocate KV differently. The motivating setup has plain NIXL on P and NIXL + HiSparse on D, with --block-size 128 on both servers.

The same command-line block size does not guarantee the same allocation. A logical block is the unit the scheduler allocates; a physical page is the unit used by the attention kernel and transfer. With a kernel that requires 64-token pages, plain P can keep 128-token logical blocks and split each into two physical pages. HiSparse's setup on D resolves its MLA scheduler blocks to the native 64-token size directly. That produces this geometry in the affected path:

Command-line block size Scheduler's logical block Physical/kernel page
P: plain NIXL, HiSparse disabled 128 128 64
D: NIXL + HiSparse 128 64 64

For a 160-token prompt, P must allocate two logical blocks: capacity for 256 tokens, or four physical pages. D needs only three physical pages. Let A and B each hold 64 tokens, and C hold the remaining 32:

P advertises:       [ A | B | C | unused ]
D has space for:    [   |   |   ]

Current selection:  [ B | C | unused ]  <- last three producer pages
Correct selection:  [ A | B | C ]

Logical-to-physical expansion already happens. The mistake comes afterward: the region-based suffix matcher assumes that any extra pages on P represent a prefix D has already cached. Here the extra page is unused capacity at the end, so that assumption shifts the whole transfer. If D has actually cached A, the remaining transfer should select B, C; the old logic selects C, unused.

The fix selects pages by token position. Pass the producer's existing remote_num_tokens through to the worker, combine it with each destination group's cached-prefix offset, and select the corresponding physical pages. Whole-page transfers remain: C is transferred with its unused rows, but the entirely unused fourth page does not shift the selection. This adds no prefill on D.

HiSparse also exposes a separate assumption at transfer completion. P can keep main KV and indexer KV in one merged cache group. D separates host KV and GPU indexer KV into different groups, so NIXL uses regions to match the corresponding memory buffers. The old get_transfer_results() still pairs groups by position. Looking up P's group 1 for D's group 1 can fail because P has only group 0, or pair the wrong blocks when the index exists.

Completion now uses the region read plan. The worker records region_blocks_to_zero: whole destination pages that were allocated but excluded from the read. For example, in the reverse allocation geometry, D may reserve four pages while P supplies three:

D after the read:   [ A | B | C | untouched ]
D after completion:[ A | B | C | zeroed    ]

For two example regions with destination IDs [10, 11, 12, 13] and [20, 21, 22, 23], receiving three pages into each produces a cleanup plan of [[13], [23]]. Completion clears page 13 only in its region and page 23 only in its region, then skips the old positional group processing.

This plan is local to one request on the receiving worker. None means use the existing completion path; empty per-region lists mean a region-mapped read with nothing to zero. In the original P-four-pages/D-three-pages example, D receives all its pages, so there is no zeroing. The plan does not include cached prefix pages or unused rows within the transferred partial page. It is discarded with the request's receive metadata; failed reads skip this cleanup.

Scope: pure MLA region pulls with equal physical page sizes, DCP=1, and one cache-group owner per region. Heterogeneous logical geometry without the required metadata or with shared-group regions is rejected. Existing homogeneous fallback behavior remains.

Dependency: the empty-receive fixes belong to #57049 (also present in #56946, originally from #55398). They are excluded from this PR, which will be rebased after that prerequisite lands. #57049's HiSparse tail-import policy remains separate from this transfer-alignment fix.

Duplicate checks included related issue #57019, its comments, and open PR searches for that issue, NIXL logical/physical geometry, and region padding. #57027 fixes layer binding offsets. #55374 introduces broader piecewise token-window loading in the ordinary group-based pull/push paths; its current diff does not wire that through the region-based path introduced by #53780. This PR addresses that region path without adding the piecewise-loading API.

AI assistance: OpenAI Codex assisted with investigation, implementation, and validation. The human submitter is responsible for reviewing the changed lines and validating the patched GPU deployment. GPU accuracy validation remains pending.

Test Plan

.venv/bin/python -m pytest tests/v1/kv_connector/unit/test_nixl_connector_hma.py -m cpu_test -q

uv run --no-project pre-commit run --files \
  vllm/distributed/kv_transfer/kv_connector/v1/nixl/metadata.py \
  vllm/distributed/kv_transfer/kv_connector/v1/nixl/base_worker.py \
  vllm/distributed/kv_transfer/kv_connector/v1/nixl/pull_worker.py \
  tests/v1/kv_connector/unit/test_nixl_connector_hma.py

GPU follow-up: apply this fix alongside #57049 and #57027, restore block size 128, and repeat the same 100-question GSM8K sample with the same model, prompt/scoring settings, and concurrency 10. A full matched evaluation remains pending.

Test Result

  • CPU suite: 91 passed, 1 deselected. Includes real receive-path page expansion and descriptor selection with transport mocked, producer/consumer padding, unequal per-region cache hits, and completion zeroing across strided layer views.
  • Negative control: restoring main's mapping methods in memory makes the 160-token regression fail on the source descriptors: [21, 22, 23, 121, 122, 123] versus [20, 21, 22, 120, 121, 122].

Use the advertised token extent and per-group prefix offsets to distinguish allocation padding from cached prompt pages. Track clipped destinations by region for receive completion.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: NickLucche <nicolo.lucchesi@mistral.ai>
@mergify mergify Bot added bug Something isn't working kv-connector labels Sep 15, 2026
Rename the worker-local cleanup plan and document its None/empty semantics. Remove the duplicated empty-receive fixes and their tests; PR vllm-project#57077 will be rebased after the prerequisite lands.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: NickLucche <nicolo.lucchesi@mistral.ai>
@NickLucche
NickLucche marked this pull request as ready for review September 15, 2026 21:47

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

@NickLucche NickLucche changed the title [Bugfix][NIXL] Align region-mapped pulls across logical block sizes [Bugfix][HiSparse][PD] Align region-mapped pulls across logical block sizes Sep 15, 2026
@NickLucche

Copy link
Copy Markdown
Member Author

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #89191 for commit 6891a87ce839.

@LucasWilkinson LucasWilkinson added this to the v0.30.0 cherry picks milestone Sep 16, 2026
for region, blocks in enumerate(block_ids):
if not blocks:
continue
cache = self.device_kv_caches[self.region_names[region]]

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.

should self.device_kv_caches

self.host_xfer_buffers if self.use_host_buffer else self.device_kv_caches

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

Overall looks good except (other then maybe my comment above)

@NickLucche
NickLucche merged commit 804e537 into vllm-project:main Sep 16, 2026
60 checks passed
vllm-agent pushed a commit to vllm-agent/vllm that referenced this pull request Sep 16, 2026
…er fixture

PR vllm-project#57077's region_pull_worker fixture constructs NixlConnectorWorker via
object.__new__() and sets attributes manually. It does not set
_transfer_layer_group_ids, which base_worker.py:198 reads
(self._transfer_layer_group_ids) since vllm-project#50494 merged. vllm-project#57077's CI ran
against a base without vllm-project#50494, so the conflict only surfaces on main:
tests/v1/kv_connector/unit/test_nixl_connector_hma.py::test_region_pull_ignores_allocation_padding
fails 7x with AttributeError (build #89376).

Set the attribute to the empty-tuple default used by the base class
__init__ (and by the sibling _make_mock_worker_for_desc_ids fixture).

Signed-off-by: kimi-no-na-wa <kimi-no-na-wa@users.noreply.github.com>
khluu pushed a commit that referenced this pull request Sep 17, 2026
… sizes (#57077)

Signed-off-by: NickLucche <nicolo.lucchesi@mistral.ai>
ch2lab added a commit to ch2lab/vllm that referenced this pull request Oct 19, 2026
Conflict resolution (30 files):
- 19 files: adopted v0.32.0 (our matching cherry-picks vllm-project#57554 vllm-project#56545 vllm-project#57871
  vllm-project#53187 vllm-project#57049 vllm-project#57077 vllm-project#57570 are now upstream).
- Re-reverted vllm-project#53007 (attention.py / kv_cache_utils.py / test_backend_per_kind.py):
  page_budget=shared_page, restore _largest_kernel_block_within candidates logic.
- flashinfer.py: adopted upstream SM12x NVFP4 KV (vllm-project#46963/vllm-project#60452); re-applied our
  D256 prefill (_nvfp4_d256_prefill + FIPrefill fields + call sites).
- kernel_warmup.py: adopted upstream autotune refactor; re-applied our W4A16
  CuTe-DSL autotune skip.
- qwen3_dflash.py: adopted upstream quantized-draft KV fallback (dropped our
  NVFP4 fused dequant fast-path; DFlash2 is backup).
- dflash/eagle utils.py: kept our shared-cache cache_config handling (avoid
  replace-copy layout loss) + upstream get_draft_load_config.
- entrypoints: union imports (our per-request logging + upstream additions).
michael-gpu pushed a commit to practical-inference/vllm-ampere-extended that referenced this pull request Oct 20, 2026
… sizes (vllm-project#57077)

Signed-off-by: NickLucche <nicolo.lucchesi@mistral.ai>
@MatthewBonanni MatthewBonanni added the hisparse Related to HiSparse label Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working hisparse Related to HiSparse kv-connector

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants