Repository navigation
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment Once the PR is approved or has the If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban. 🚀 |
|
This pull request has merge conflicts that must be resolved before it can be |
njhill
left a comment
There was a problem hiding this comment.
This should be fixed in the connector, not the scheduler.
In PD-disaggregation the MoRIIO connector's READ mode treated the remote KV load as synchronous (`get_num_new_matched_tokens` returned `load_kv_async=False`), so the decode request -- whose `request_id` is the router-embedded compound id (`...___prefill_addr_...___decode_addr_..._<uuid>`) -- was sent straight to RUNNING and into `scheduler_output.num_scheduled_tokens`. On a step where the worker produced no output row for it, that compound id was absent from `model_runner_output.req_id_to_index`, and the scheduler's unconditional `req_id_to_index[req_id]` lookup in `update_from_output` raised `KeyError`, killing the EngineCore with `EngineDeadError` on the first inference request of MoRIIO 1P1D/1P2D/2P2D runs. Fix this in the connector (per review feedback on vllm-project#54301: "This should be fixed in the connector, not the scheduler."), without touching core `scheduler.py`: - `MoRIIOConnectorScheduler.get_num_new_matched_tokens` (READ branch) now returns `(n, True)` when `n > 0`, so the request is held in `WAITING_FOR_REMOTE_KVS` instead of being scheduled before its KV exists. It falls back to `(n, False)` only for the tiny-prompt/nothing-to-load edge case (the scheduler asserts `>0` for async loads). - `MoRIIOConnectorWorker.get_finished` (READ branch) now surfaces `done_recving = self._pop_done_transfers()` (previously discarded), giving the scheduler the `finished_recving` signal to promote the request out of `WAITING_FOR_REMOTE_KVS`. The request is therefore never scheduled-but-unindexed and the KeyError is structurally impossible, while the core scheduler invariant (every scheduled req_id has an output row) is preserved untouched -- mirroring the async handling MoRIIO's WRITE-mode consumer leg already uses. Validated on 1P1D GLM-5.3-FP8 WideEP PD-disagg (MI300X): READ mode exercises the async path on the exact compound PD req_ids with WAITING_FOR_REMOTE_KVS promotion confirmed and zero KeyError/EngineDeadError; WRITE mode shows no regression across a multi-hour run. Adds `tests/v1/kv_connector/unit/test_moriio_async_read.py` covering READ->async, edge-case->sync, WRITE unchanged, and that the producer never loads. Signed-off-by: Mohit Deopujari <Mohit.Deopujari@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
781c00f to
91c4c04
Compare
|
Thanks @njhill — addressed. I've dropped the Instead of tolerating the broken state in the core scheduler, READ mode now uses vLLM's existing async KV-load lifecycle: Branch was also rebased onto latest |
| assert conn.get_num_new_matched_tokens(request, 0) == (0, False) | ||
|
|
||
|
|
||
| def test_scheduler_update_from_output_is_not_monkeypatched(): |
There was a problem hiding this comment.
Suggestion: Don't think this one makes sense to keep
|
Thanks @simondanielsson! Repro: 1P1D PD-disagg, MoRIIO connector in READ mode ( Root cause: in READ mode On the lifecycle concern — agreed, that's the crux. I went async specifically to reuse the path the WRITE-mode consumer already uses ( Test: I'll drop |
|
@MohitAMD Thanks, can you share the full commands for the engine and workload? I'd like to understand why/when this can happens
|
|
This pull request has merge conflicts that must be resolved before it can be |
Summary
In PD-disaggregation the MoRIIO connector's READ mode treated the remote KV load as synchronous (
get_num_new_matched_tokensreturnedload_kv_async=False). The decode request — whoserequest_idis the router-embedded compound id (...___prefill_addr_...___decode_addr_..._<uuid>) — was therefore sent straight toRUNNINGand intoscheduler_output.num_scheduled_tokens. On a step where the worker produced no output row for it, that compound id was absent frommodel_runner_output.req_id_to_index, so the scheduler's unconditionalreq_id_to_index[req_id]lookup inupdate_from_outputraisedKeyError→EngineDeadError, on the first inference request of MoRIIO 1P1D/1P2D/2P2D runs.What changed (connector-side, per review)
Per @njhill's review ("This should be fixed in the connector, not the scheduler."), this is now fixed entirely in the MoRIIO connector — core
vllm/v1/core/sched/scheduler.pyis untouched. The earlier scheduler guard has been dropped.The READ-mode load now uses vLLM's existing async KV-load lifecycle, mirroring what MoRIIO's WRITE-mode consumer leg already does:
MoRIIOConnectorScheduler.get_num_new_matched_tokens(READ branch): returns(n, True)whenn > 0, so the request is held inWAITING_FOR_REMOTE_KVSinstead of being scheduled before its KV exists. Falls back to(n, False)only for the tiny-prompt / nothing-to-load edge case (the scheduler asserts> 0for async loads).MoRIIOConnectorWorker.get_finished(READ branch): now surfacesdone_recving = self._pop_done_transfers()(previously discarded), giving the scheduler thefinished_recvingsignal to promote the request out ofWAITING_FOR_REMOTE_KVS.The request is therefore never scheduled-but-unindexed, so the
KeyErroris structurally impossible while the core scheduler invariant (every scheduled req_id has an output row) is preserved untouched.Tests
Adds
tests/v1/kv_connector/unit/test_moriio_async_read.py(CPU-only): asserts READ→async, edge-case→sync, WRITE unchanged, producer never loads, and thatScheduler.update_from_outputis not wrapped/patched.Validation
Validated on 1P1D GLM-5.3-FP8 WideEP PD-disagg (MI300X):
WAITING_FOR_REMOTE_KVSpromotion confirmed, endpoints reach startup and serve HTTP 200, zeroKeyError/EngineDeadError.Risk
The async path relies on
get_finishedreportingdone_recvingfor every READ transfer (a missed signal would leave a request waiting inWAITING_FOR_REMOTE_KVSrather than crashing); the cluster run confirms the signal fires and requests complete.🤖 This change was developed with AI assistance (Cursor); all changes were reviewed and validated by the author. See the
Co-authored-bytrailer on the commit.