Repository navigation
[Bugfix][KV Offload] Let idle P2P-only engines park - #63618
Conversation
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
… setups Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
|
I’m going to test this live soon with two P2P peers, covering cache transfers while the source is idle, pause/resume, and graceful shutdown. I’ll post the configuration and results here before marking the PR ready for review. Design feedback is welcome meanwhile. |
|
Documentation preview: https://vllm--63618.org.readthedocs.build/en/63618/ |
|
This pull request has merge conflicts that must be resolved before it can be |
liranschour
left a comment
There was a problem hiding this comment.
Thanks.
Two things before this can merge:
- Rebase onto #63779
On main, #63779 added bool(self._deferred_releases) to TieringOffloadingManager.has_pending_work(). Please keep that check when you resolve the conflict, filtered per tier the same way as _jobs in item 2. A deferred release on a polled tier completes inside poll_tiers() → _process_finished_jobs(), so it doesn't need an engine step.
- Required: work the poll thread can finish must not block idleness
When the tier-poll thread is running, an idle lone-P2P producer handles a whole peer request without any engine step: receive the LookupMsg, parent.lookup(), pin the HITs, send LookupRespMsg, take the FetchMsg, NIXL WRITE, complete_read(). The current check still counts two parts of that as pending work:
- the HIT pin in _jobs, which stays until the fetch completes or 30 s pass;
- _server.has_inflight_transfers, through P2PSession.has_pending_work.
So a producer that peers keep fetching from can't finish pause or graceful shutdown. That's the bug this PR fixes, moved from an idle source to a busy one.
Requested change:
- Count only work that needs an engine step. In TieringOffloadingManager.has_pending_work(), when a poll thread is alive, leave out the polled tiers: their has_pending_work(), and any _jobs / _deferred_releases entries whose tier_idx is a polled tier. Keep the rest: pending cascades, jobs on non-polled tiers, the multi-tier keep-alive, and the tier_poll_interval_s <= 0 keep-alive.
- Fix the docs. Both has_pending_work() docstrings should say they report work that needs a step. Add a sentence to kv_offloading_usage.md that a paused engine keeps serving peers from its CPU tier: pause stops scheduling and quiesces the device, not the P2P server.
Pause with clear_cache and sleep don't depend on has_work(), because reset_cache() takes the manager lock and drains every tier. A parked lookup pin can still make that drain assert. That's tracked in #63948 and fixed by #63988, so it's out of scope here.
Tests (unit level, using the existing _attach_fake_transports / paired-manager helpers):
- Live thread, peer HIT pinned and waiting for its FetchMsg: has_pending_work() is False. The fetch then completes with no on_schedule_end(), and _jobs empties.
- Same setup with tier_poll_interval_s=0: has_pending_work() is True.
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
orozery
left a comment
There was a problem hiding this comment.
Can we also make comments shorter?
Co-authored-by: Or Ozeri <or@ozery.com> Signed-off-by: alberto <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
|
/ci run |
|
✅ @albertoperdomo2, CI is now available for this PR.
|
|
✅ Triggered Buildkite CI #98308 for commit ℹ️ This PR is 3 commits behind upstream |
Overview
Let an idle P2P-only offload engine report no pending work, using the tier-poll thread introduced in #58168 to service peers. Related context: #51476.
Claims
max(tier_poll_interval_s, 1 s), and rate-limits error reports.Validation
.venv/bin/python -m pytest -s -q -p no:cacheprovider tests/v1/kv_offload/tiering/: 573 passed, 12 skipped.Instrumented engine probes verified idle queue parking and the shutdown decision; a deterministic fault probe verified bounded logging, backoff, and recovery.
Ruff 0.14.0 check/format checks, Markdown lint, and
git diff --checkpassed.Live H100 A/B validation completed: Qwen3-0.6B, two TP1/DP1 instances, P2P-only, real NIXL transfers over TCP.
mode=wait&clear_cache=falseEach arm transferred 1,008 MiB / 576 chunks with zero promotion failures. Outputs also matched across images. This validates the P2P-only path; sleep, DP, multi-tier and RDMA performance were not tested.
Details
Idle polling belongs to the poll thread. Keep the engine stepping when polling is disabled or multiple secondary tiers require scheduler-driven promotions; those configurations retain their existing pause/shutdown limitation. KVCR is unchanged. Rendered documentation describes the supported behavior.
Duplicate-work searches found related #61667, #62795, and #62929, which address transfer lifetime, finalization ordering, and cache-reset fencing rather than this idle predicate. Design feedback is welcome.
AI assistance was used for implementation, tests, and review.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (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.
Need help on this PR? Tag
@codesmith-botwith what you need.Need help on this PR? Tag
@codesmith-botwith what you need.