Repository navigation
Conversation
Signed-off-by: Itay Etelis <92247226+Etelis@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe change makes ChangesP2P transfer failure handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to P2P NIXL submission failures now follow the existing failure path, release created handles when possible, and finalize each request round once. No unresolved merge-readiness risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
The submission-failure cleanup and the Keeping polling exceptions in #49632 makes sense. We should keep #55181 open until both fixes land, since submission failures and |
liranschour
left a comment
liranschour
left a comment
There was a problem hiding this comment.
The fix works and looks good. One change is requested in nixl.py - treat an error return code also as an exception.
| remote_dlist, | ||
| np.asarray(remote_idxs, dtype=np.int32), | ||
| ) | ||
| self._agent.transfer(handle) |
There was a problem hiding this comment.
transfer() doesn't raise on submission failure — it returns "ERR".
this path isn't covered by the new try/except.
Suggested:
status = self._agent.transfer(handle)
if status == "ERR":
raise RuntimeError(f"NIXL post failed: {status}") # or duplicate the warn + release + return None
| self._agent_name, | ||
| peer_id, | ||
| exc, | ||
| exc_info=True, |
There was a problem hiding this comment.
exc_info=True on a warning plus exc interpolated in the message is doubly redundant; every other failure log in this file (poll, _release_handles, cancel) logs without a traceback. Either logger.exception or drop exc_info.
|
This pull request has merge conflicts that must be resolved before it can be |
|
Closed in favour of a smaller fix required, |
…hing peer
A NIXL error raised while submitting a fetch's WRITE escaped
`NixlTransport.write_blocks` into `P2PSession._on_message`, which only
logs. The round was never finalized, so the requesting peer learned
nothing and only gave up at its load timeout (30 s) before falling back
to recompute — against ~6 s to recompute a 113K-token prefix directly.
The handle from `make_prepped_xfer` also leaked when `transfer` raised.
Three changes:
- `NixlTransport.write_blocks` catches submission errors, releases a
created handle, logs, and returns None. This is what
`DataTransport.write_blocks` already documents ("None if the peer is
not registered or submission failed") and reuses the existing
fail-fast path in `ServerRole._submit_transfer`.
- `ServerRole._submit_transfer` treats a raise from `write_blocks` as a
failed submission, so a transport that violates the contract cannot
wedge a round until the peer's timeout either.
- `ServerRole.on_fetch` returns after submitting. When the submission
failed with nothing else inflight, `_submit_transfer` already
finalized the round and pruned the request, and falling through to
the prefiller-first `finishing` check finalized it a second time —
`KeyError` on the popped request, which the dispatcher then counted
toward its consecutive-error disconnect.
Carries forward the transport catch and the `on_fetch` early return from
vllm-project#55553, which was closed in favour of this, and adds the
`_submit_transfer` guard plus its regression test.
Fixes vllm-project#60204. Addresses the submission half of vllm-project#55181; the
`check_xfer_state()` path there is covered separately by vllm-project#49632 and
vllm-project#60279.
Co-Authored-By: Itay Etelis <92247226+Etelis@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Liran Schour <lirans@il.ibm.com>
3 participants
Addresses #55181.
NIXL submission exceptions leave the decoder waiting and can trigger
double-finalization. Report failed submissions through the existing failure
path, release created handles when possible, and finalize each round once.
Polling exceptions remain covered separately by #49632.
Tests
Validated on 2× NVIDIA H100 80GB:
.venv/bin/python -m pytest tests/v1/kv_offload/tiering/p2p/ -v:219 passed; five regression failures reproduced before the fix.
output. Failures were injected after handle creation.