Skip to content

[NIXL] Drop unreachable producer-side notif counting in push mode - #62210

Merged
njhill merged 2 commits into
vllm-project:mainfrom
sawsa307:nixl-push-drop-producer-notif-count
Oct 17, 2026
Merged

njhill merged 2 commits into
vllm-project:mainfrom
sawsa307:nixl-push-drop-producer-notif-count

Conversation

@sawsa307

@sawsa307 sawsa307 commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Remove the producer-side branch of NixlPushConnectorWorker._get_new_notifs, which no push-mode code path can reach. Split out of #61488 / #61580 as an independent simplification, as suggested in #61580 (comment).

Claims

  • No behavior change for P or D in push mode: the removed branch only ran on a plain "<req_id>:<n>" notif arriving at P, which push mode never sends.
  • _get_new_notifs now only counts consumer-side completion notifs and always returns an empty set; P keeps reporting finished sends from its WRITE handles in get_transfer_results.
  • 26 insertions, 59 deletions.

Validation

  • pytest tests/v1/kv_connector/unit/test_nixl_push_connector.py tests/v1/kv_connector/unit/test_nixl_heartbeat.py: 103 passed.
  • pytest tests/v1/kv_connector/unit/test_nixl_connector.py: 182 passed. The two test_abort_timeout_on_prefiller cases fail locally on main as well.
  • With a tripwire in the removed branch, the push, heartbeat and NIXL connector unit tests pass without reaching it.
  • On GB200, the tripwire was not reached in end-to-end push runs with TP1 → TP1, producer TP2 → consumer TP1 and producer TP1 → consumer TP2, and those runs met their expected outputs. These runs used the same push_worker.py change on the [KV Transfer] Connector-agnostic KV checksums for P/D disaggregation #61488 branch.

Details

In push mode, D sends P only two kinds of notifs:

  • PUSH_REG, consumed by the writer thread before notifs are forwarded to _get_new_notifs;
  • HB:, handled at the top of _get_new_notifs before the counting.

Plain "<req_id>:<n>" notifs to P come from _pending_recv_notifs, which only pull_worker.py fills. P → D completion notifs are attached to the push WRITEs and are counted on D, which is the branch this PR keeps.

The removed code is the producer-side counting copied from pull mode, the transfer_topo assert it needed, and the counter cleanup for finished pushes in get_transfer_results. One edge case changes: on a kv_both instance where one request is both loading and saving, a completion notif for it is now counted as a load instead of a send. Push mode never sends such a notif to the producer side.

Not a duplicate: #55470 also edits _get_new_notifs (it adds a failed-WRITE notif path) but keeps the producer-side branch. Whichever merges second needs a small rebase.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill. (Mandatory for agents, optional for humans).

  • AI assistance was used during the creation of this PR.

  • Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.

  • Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.

  • Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.

  • Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.

🤖 Generated with Claude Code

In push mode, the only notifs D sends to P are PUSH_REG, which the
writer thread consumes, and HB, which _get_new_notifs handles before
the counting. Only pull_worker.py fills _pending_recv_notifs, so no
code path sends P a plain "<req_id>:<n>" notif. The producer-side
branch of NixlPushConnectorWorker._get_new_notifs was carried over
from pull mode and never runs: P reports finished sends from its WRITE
handles in get_transfer_results.

Remove the branch, its guard, the transfer_topo assert it needed and
the counter cleanup for finished pushes. _get_new_notifs now counts
only consumer-side completion notifs and always returns an empty set.
A completion notif for a request that is also being sent (one request
both loading and saving on a kv_both instance) is now counted as a
load instead of as a send.

Tests
-----
test_nixl_push_connector.py no longer sets transfer_topo or checks the
return value of _get_new_notifs; the consumer-side assertions are
unchanged.

Validation
----------
- test_nixl_push_connector.py and test_nixl_heartbeat.py: 103 passed.
  test_nixl_connector.py: 182 passed; the two
  test_abort_timeout_on_prefiller cases fail locally on main as well.
- With a tripwire in the removed branch, the push, heartbeat and NIXL
  connector unit tests pass without reaching it.
- On GPU, the tripwire was not reached in end-to-end push runs with
  TP1 -> TP1, producer TP2 -> consumer TP1 and producer TP1 -> consumer
  TP2, and those runs met their expected results.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: David Cheung <d7cheung@gmail.com>

@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 added the kv-connector label Oct 17, 2026
@sawsa307

Copy link
Copy Markdown
Contributor Author

cc @njhill

@njhill njhill left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @sawsa307!

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 17, 2026
@njhill

njhill commented Oct 17, 2026

Copy link
Copy Markdown
Member

/ci run

@github-actions

Copy link
Copy Markdown

✅ @sawsa307, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • Run commands test your branch with every commit currently on its upstream target branch. If it is behind, the bot merges the target branch into it first (pull before pushing again). Append --allow-stale to a run command to test an outdated branch as is, at your own risk.
  • /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

Copy link
Copy Markdown

✅ Triggered Buildkite CI #95617 for commit 6736f5362cb1.

ℹ️ This PR is 4 commits behind upstream main. Merged upstream main into this branch as 6736f5362cb1 before starting CI. Pull the branch before pushing more changes.

@njhill
njhill enabled auto-merge (squash) October 17, 2026 19:11
@njhill
njhill merged commit 7879f36 into vllm-project:main Oct 17, 2026
71 checks passed
michael-gpu pushed a commit to practical-inference/vllm-ampere-extended that referenced this pull request Oct 20, 2026
…lm-project#62210)

Signed-off-by: David Cheung <d7cheung@gmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: vllm-agent <claw@inferact.ai>
sohom-cs added a commit to sohom-cs/vllm that referenced this pull request Oct 29, 2026
Push mode learns of finished sends from its WRITE handles (vllm-project#62210), so the
push test now completes rank 0's WRITE instead of mocking _get_new_notifs.
The pull test sends rank 0 a consumer notification through the NIXL
wrapper instead of patching the private method.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Sohom Chakraborty <16609933+sohom-cs@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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