Skip to content

[NIXL][PCP] Report replicated-PCP ranks > 0 as done sending instead of hiding them - #53903

Merged
NickLucche merged 4 commits into
vllm-project:mainfrom
LucasWilkinson:fix/nixl-replicated-pcp-finished
Sep 10, 2026
Merged

NickLucche merged 4 commits into
vllm-project:mainfrom
LucasWilkinson:fix/nixl-replicated-pcp-finished

Conversation

@LucasWilkinson

@LucasWilkinson LucasWilkinson commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

With replicated KV under PCP only PCP rank 0 takes part in NIXL transfers, so NixlConnector on ranks > 0 cleared done_sending and get_finished_count() was lowered to TP x PP so the scheduler only waited for rank 0.

That breaks as soon as NixlConnector is wrapped in a MultiConnector with a sibling that saves on every rank (e.g. OffloadingConnector):

  • MultiConnector.get_finished_count() returns None when any child returns None, so the KVOutputAggregator expects world_size finishes per request;
  • MultiConnector records an extra async save per request and only reports it finished once every child on that worker has — and NIXL on ranks > 0 never does.

Requests therefore never finish sending, their blocks are never freed and the prefill engine stalls at Running: 0 reqs, Waiting: N reqs with the KV cache full. Observed with GLM-5.2-FP8 PCP8 prefill, MultiConnector = NixlConnector(kv_producer) + OffloadingConnector(TieringOffloadingSpec): 14/35 requests completed at concurrency 16, then nothing.

Fix: ranks > 0 report the requests they are asked to send as finished as soon as the connector metadata arrives, and get_finished_count() is back to world_size. Rank 0 still reports only when the consumer has read the blocks, so the aggregate completes exactly when it did before.

Test Plan

  • E2E: GLM-5.2-FP8, prefill --prefill-context-parallel-size 8 + MultiConnector (NIXL producer + OffloadingConnector), DP8 decode; agentic trace benchmark at concurrency 16/32/64.
  • Plain NixlConnector under PCP8 (no MultiConnector) unchanged behaviour.

Test Result

Before: prefill stalls after ~14 requests (0 running / 16 waiting / KV 70%). After: pending — will update with the benchmark run.

🤖 Generated with Claude Code; reviewed by author

…f hiding them

With replicated KV under PCP only PCP rank 0 takes part in transfers, so
NixlConnector ranks > 0 cleared `done_sending` and `get_finished_count`
was lowered to TP x PP so the scheduler only waited for rank 0.

That breaks as soon as NixlConnector is wrapped in a MultiConnector with a
sibling that saves on every rank (e.g. OffloadingConnector):

* `MultiConnector.get_finished_count` returns None when any child returns
  None, so the aggregator expects `world_size` finishes per request;
* `MultiConnector` records an extra async save per request and only
  reports it finished once *every* child on that worker has, and NIXL on
  ranks > 0 never does.

Requests therefore never finish sending, their blocks are never freed and
the prefill engine stalls at 0 running / N waiting with the KV cache full
(observed with GLM-5.2 PCP8 prefill + NIXL + CPU offloading).

Fix: ranks > 0 report the requests they are asked to send as finished as
soon as the connector metadata arrives, and the finished count is back to
`world_size`. Rank 0 still reports only when the consumer has actually
read the blocks, so the aggregate completes exactly when it did before.

Signed-off-by: Lucas Wilkinson <lwilkinson@neuralmagic.com>
@mergify mergify Bot added the kv-connector label Aug 26, 2026
LucasWilkinson added a commit to neuralmagic/vllm that referenced this pull request Aug 31, 2026
…ng instead of hiding them

With replicated KV under PCP only PCP rank 0 takes part in transfers, so
NixlConnector ranks > 0 cleared `done_sending` and `get_finished_count`
was lowered to TP x PP so the scheduler only waited for rank 0.

That breaks as soon as NixlConnector is wrapped in a MultiConnector with a
sibling that saves on every rank (e.g. OffloadingConnector):

* `MultiConnector.get_finished_count` returns None when any child returns
  None, so the aggregator expects `world_size` finishes per request;
* `MultiConnector` records an extra async save per request and only
  reports it finished once *every* child on that worker has, and NIXL on
  ranks > 0 never does.

Requests therefore never finish sending, their blocks are never freed and
the prefill engine stalls at 0 running / N waiting with the KV cache full
(observed with GLM-5.2 PCP8 prefill + NIXL + CPU offloading).

Fix: ranks > 0 report the requests they are asked to send as finished as
soon as the connector metadata arrives, and the finished count is back to
`world_size`. Rank 0 still reports only when the consumer has actually
read the blocks, so the aggregate completes exactly when it did before.

Signed-off-by: Lucas Wilkinson <lwilkinson@neuralmagic.com>
@LucasWilkinson
LucasWilkinson marked this pull request as ready for review August 31, 2026 19:20

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

Agree semantics of _expected_finished_count is a bit weird here, but at least it's not particularly inefficient as we have to poll from the worker anyway due to offloading.

I left some comments about unifying the _replicated_pcp_done_sending file change @LucasWilkinson

Comment thread vllm/distributed/kv_transfer/kv_connector/v1/nixl/connector.py Outdated
Comment thread vllm/distributed/kv_transfer/kv_connector/v1/nixl/connector.py Outdated
Co-authored-by: Nicolò Lucchesi <nicolo.lucchesi@gmail.com>
Signed-off-by: Lucas Wilkinson <LucasWilkinson@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: b1ed140b-21f3-455f-b258-b140d5183ce6

📥 Commits

Reviewing files that changed from the base of the PR and between 903a021 and 5725a12.

📒 Files selected for processing (3)
  • vllm/distributed/kv_transfer/kv_connector/v1/nixl/base_worker.py
  • vllm/distributed/kv_transfer/kv_connector/v1/nixl/connector.py
  • vllm/distributed/kv_transfer/kv_connector/v1/nixl/pull_worker.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Fixed completion reporting for replicated key-value cache transfers during distributed inference.
    • Requests handled by replicated context-parallel workers are now recognized as finished promptly.
    • Improved scheduler and connector coordination so distributed operations no longer wait unnecessarily for completion signals.

Walkthrough

The NIXL connector now tracks requests skipped by replicated PCP ranks and reports them as finished. It also removes the fixed completion count for multi-PCP KV producers.

Changes

Replicated PCP completion reporting

Layer / File(s) Summary
Track replicated PCP requests
vllm/distributed/kv_transfer/kv_connector/v1/nixl/base_worker.py, vllm/distributed/kv_transfer/kv_connector/v1/nixl/pull_worker.py
The worker initializes a completion set and records requests handled by replicated PCP ranks before the early return.
Report replicated PCP completion
vllm/distributed/kv_transfer/kv_connector/v1/nixl/connector.py
The connector reports and clears tracked requests for replicated PCP ranks. get_finished_count now returns None for these KV producer cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 5725a

Replicated PCP ranks now report skipped KV sends as complete, allowing multi-connector requests to finish and KV blocks to be freed without changing rank-0 transfer completion behavior. No concrete current-head merge risk remains.

Suggested reviewers: nicklucche

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the replicated-PCP completion issue, the MultiConnector interaction, the fix, and the planned validation. It is directly related to the changeset.
Title check ✅ Passed The title clearly summarizes the main change: replicated PCP ranks greater than 0 now report completion instead of hiding their sends. It is concise and specific.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

LucasWilkinson and others added 2 commits September 10, 2026 05:09
Co-authored-by: OpenAI Codex <codex@openai.com>

Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
@LucasWilkinson

Copy link
Copy Markdown
Contributor Author

/ci run

@LucasWilkinson LucasWilkinson added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 10, 2026
@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #88052 for commit 53e1737a7159.

@NickLucche
NickLucche merged commit 93911fc into vllm-project:main Sep 10, 2026
54 checks passed
ItsRoy69 pushed a commit to ItsRoy69/vllm that referenced this pull request Sep 10, 2026
…f hiding them (vllm-project#53903)

Signed-off-by: Lucas Wilkinson <lwilkinson@neuralmagic.com>
Signed-off-by: Lucas Wilkinson <LucasWilkinson@users.noreply.github.com>
Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
Co-authored-by: Nicolò Lucchesi <nicolo.lucchesi@mistral.ai>
Signed-off-by: Jyotirmoy Roy <jyotirmoyroy649@gmail.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.

2 participants