Repository navigation
[Feature / Core] Support Independent Stage Execution & External Routing - #6883
RishabhSaini wants to merge 21 commits into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/entrypoints.md. Module owners: @alex-jw-brooks @linyueqian @NickCao @RishabhSaini, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
f8046b3 to
e4419b0
Compare
Test ResultValidated end-to-end on 2x NVIDIA H200 (K8s pod, Setup: Test (using Output verification:
|
|
@vllm-omni-review-bot for review |
|
@alex-jw-brooks @bian6 PTAL |
|
Follow-on PR for omni disagg + connector plumbing: #6893 (draft) |
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Co-authored-by: RishabhSaini <rishabhsaini01@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
39dca5e to
9dfde39
Compare
Review is outdated (on previous iteration)
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
1 similar comment
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
Changes since the previous review
- 2 new inline finding(s); 1 finding(s) below.
CI at
6df8c770c050(2026-10-10T16:46:17.944664+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/cursor/cursor-grok-4.6-highcould not complete this review, so it was produced by the fallback armdirect/cursor/auto. It is excluded from the routing experiment.
Full review analysis
PR description
This change adds an experimental POST /v1/run endpoint that executes one pipeline stage per call. Stage 0 takes a prompt dict; later calls send back the previous response, which carries a base64 msgpack NextStageInputMessage. The orchestrator stops at the yielded stage and returns either that next-stage input or the final stage output, instead of submitting the next stage itself. When the following stage consumes a full payload, the producer attaches the serialized payload to the request so the next call does not depend on connector state left on a particular replica.
Change flow
flowchart LR
Client["[EXISTING] POST /v1/run caller"]:::existing --> Run["[NEW] ServingRun encodes one stage"]:::new
Run --> Orch["[CHANGED] Orchestrator yields at stage"]:::changed
Orch --> Payload["[CHANGED] Full payload returned on the request"]:::changed
Payload --> Next["[EXISTING] Next stage or final output"]:::existing
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
Findings
- [P1] Run handoff still looks up connector data under the new request id —
vllm_omni/engine/orchestrator.py:215
Existing thread: #6883 (comment)
Evidence for Run handoff still looks up connector data under the new request id
A later /v1/run call rebuilds the receiver request in _update_stale_request_metadata and sets both request_id and external_req_id to the new call id. The receiver then builds the connector key as {external_req_id}_{from_stage}_{chunk} in _stage_payload_recv_spec, while the producer stored the payload under the previous call's id in send_full_payload_outputs. The serialized payload is attached only when the next stage's takes_full_payload_input is true (return_stage_payload in _handle_next_stage_input). Any other handoff, including KV or connector transfer, still fetches a key that was never written, so the next headless replica cannot load the upstream payload. Keep a stable transfer id, or inline the payload for every yielded stage and fail the call when it is missing.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
| The codec returns structs as plain containers, which we rebuild based on stage type. | ||
| """ | ||
| fields = OmniMsgpackDecoder().decode(base64.b64decode(data)) | ||
| if not is_diffusion(stage_types[fields["receiver_stage_id"]]): |
There was a problem hiding this comment.
[P2] A decodable stage_input with a bad receiver id returns HTTP 500
Evidence and suggested fix
decode_stage_input indexes stage_types[fields["receiver_stage_id"]] before checking that the id matches the request. A msgpack object that decodes but has a missing key, a non-int id, or an id past the pipeline raises KeyError, TypeError, or IndexError. run_stage maps only ValueError and OmniClientError to HTTP 400, so that payload becomes an unhandled 500. Validate the receiver id against the loaded stage list and return 400 before indexing.
| return True | ||
|
|
||
|
|
||
| @router.post("/v1/run", dependencies=[Depends(validate_json_request)]) |
There was a problem hiding this comment.
[P1] PR body does not record the engine and entrypoint test selectors
Evidence and suggested fix
The body says tests exist, but it has no Test Plan or Test Result and names no pytest target, marker, or run level. The covering ready step Simple · Engine&Entrypoints Test runs pytest -sv tests/entrypoints tests/engine -m 'core_model and cpu', which is the selector for tests/entrypoints/openai_api/test_serving_run.py, tests/entrypoints/test_async_omni.py, and tests/engine/test_orchestrator.py. Scheduler and connector changes are outside that step; Simple · Other Test runs pytest -sv tests/ -m 'core_model and cpu' --ignore=tests/diffusion --ignore=tests/model_executor --ignore=tests/entrypoints --ignore=tests/engine. Please record which of those selectors were run, and which paths were not.
Omni ReviewBot: finding feedback[p1] PR body does not record the engine and entrypoint test selectors — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: finding feedback[p1] Run handoff still looks up connector data under the new request id — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Purpose
Supports running stages independently, thereby allowing independent routing and orchestration. This is done without new CLI flags and is largely model agnostic in approach. To use it, a server should be spun up with all remote stages running as
--headless, and all requests should to/v1/run./v1/runAPI Details:A new endpoint
/v1/runis added, which is the only endpoint that should be accessed when running a stage in isolation. The intended behavior of/v1/runis that a user should be able to sequentially call it, e.g., in a loop piping the previous stage output, to yield the output from the next stage until the final output.NOTE: The API will change a bit later on to reflect integration with different entrypoints, replica routing, etc, since this first pass is passing and returning raw payloads. So while the core implementation in the orchestrator & related components is complete, the API is documented as experimental and may break once when we add support for rerouting run requests through route behaviors, e.g., chat completions.
Design Considerations
Some of the more important design points of this implementation are as follows:
/v1/rundoes not hold server side state per request.Requests to
/v1/runare completely self contained, meaning everything needed to run the next stage is completely encapsulated in the payload.- This means that things that would normally go through connectors are instead attached and pulled from the run payload, which ensures that we avoid situations like accidental routing to a replica missing connector info, which could break the request.
Avoiding server side state to hold for later calls helps a lot with cleaner finish / timeout / abort semantics and ensures that we don't end up sending a replica a request that it can't handle for later stages, e.g., due to data missing in connectors. IMO this is a very important design invariant, as it keeps stuff much cleaner.
Intermediate request payloads are opaque (but serializable)
Since the requests to any stages except for the first one are model specific & not part of the server API, the user should never have to manually format them or care about what is in them. I.e., the guaranteed API after this is fully implemented should be:
^ Given that pre / post processing is route specific, and this is already a very large PR, we'll open a separate PR to propose refactoring entrypoints for compatibility with
.run, as well as other applicable follow-ups, e.g., async chunk support.Current State of this PR
Example Usage & Follow-ups
Docs have been added for experimental support! Please see the corresponding section for a full runnable example of running Qwen3-TTS with a client & server, as well as the related section on follow-ups, which we will open an RFC for in the near future.