Repository navigation
[Core] Split input wait out of vllm_queued_ms in StageRequestStats - #9050
MinhaoLi0318 wants to merge 5 commits into
Conversation
On a downstream stage that waits for its input, vllm_queued_ms (QUEUED to first SCHEDULED, added in vllm-project#8575) also counts the time before that input arrives: - async_chunk: the orchestrator pre-submits chunk-receiving stages together with stage 0, and the input gate keeps the request out of the waiting queue (WAITING_FOR_CHUNK) until the first upstream chunk lands. - requires_full_payload_input: the request waits in WAITING_FOR_INPUT until the connector delivers the payload. The scheduler of such a stage now records, once per request, the step in which the request is first left WAITING in a waiting queue after its input gate ran. The time is sent with the engine-core events, and the output processor splits vllm_queued_ms into vllm_input_wait_ms (QUEUED to input ready) and vllm_ready_queued_ms (input ready to first SCHEDULED). Both are None on stage 0, on stages without an input gate, and when the time does not fall inside the queued interval, for example an AR streaming split that covers a later segment. The generation scheduler stamps with the timestamp it uses for SCHEDULED, because that timestamp is taken before the gates run. Existing fields keep their values. The new fields sit next to the phase split in the [StageRequestStats] table, stay out of the duplex per-response table, and are not added to the stage_metrics snapshot. Signed-off-by: MinhaoLi0318 <lminhao039@gmail.com>
The metrics design doc said the phase-split intervals match upstream FinishedRequestStats. That holds for vllm_queued_ms, vllm_prefill_ms, vllm_decode_ms and vllm_num_preemptions, but not for vllm_input_wait_ms and vllm_ready_queued_ms, which split vllm_queued_ms at a time recorded by the Omni scheduler and have no upstream counterpart. Say so. Signed-off-by: MinhaoLi0318 <lminhao039@gmail.com>
…dy time _stamp_input_ready visits kv_holding_waiting as well as waiting, but no test put a request in kv_holding_waiting, so dropping that queue went unnoticed. Add one. The input-ready tests only went through OmniSchedulingCoordinator. Add tests on OmniChunkTransferAdapter, the async_chunk gate of the v1 model runner: the time is the step that sees the first chunk, in ar and generation mode, and is kept afterwards; with active_stream_window, a request without an active-stream slot is not stamped, so its slot wait counts as input wait. Signed-off-by: MinhaoLi0318 <lminhao039@gmail.com>
The gate-flag tests faked the model and vLLM configs with SimpleNamespace, and the engine-output test faked the request. Build them the way the native data-plane test in the same file already does: OmniModelConfig and VllmConfig through object.__new__, a stand-in OmniChunkTransferAdapter through object.__new__, and a vLLM Request whose events list holds the engine-core events. The tests check the same cases. Signed-off-by: MinhaoLi0318 <lminhao039@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This PR appears to belong to: docs/design/module/input_output_modality_contracts.md, docs/design/module/observability.md. Module owners: @lishunyang12 @vraiti @Gaohan123 Routing: @lishunyang12 via module of the changed files, module named in the PR description, CODEOWNERS; @vraiti via module of the changed files, module named in the PR description; @Gaohan123 via module of the changed files, CODEOWNERS @MinhaoLi0318, 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. |
|
Self-review (author). What I checked:
The three questions in the description are where I most need a decision: where to stamp (scheduler scan vs. the five AI assistance: drafted with Claude Code; I reviewed it before posting. |
|
@MinhaoLi0318 I reviewed Dependency: I do not see an RFC-level dependency requiring this bounded, receiver-local queue split to wait for #6472/#8531. Our earlier discussion deferred receiver integration into the cross-stage first-event contract. This PR does not implement that integration or complete the RFC. The distinctions I would preserve are:
Collection point and names: the scan is consistent with the documented scheduler-observation semantics on the paths I inspected; moving the stamp to gate-release sites would be a separate precision/maintenance choice. I have no RFC-side objection to the current names with those qualifications kept explicit. Scheduler/metrics owners should make the final collection-point and naming decision; this is not maintainer approval. Next step: please keep this PR focused and retain the untested-path/runtime limitations listed in your description. I will keep the #6472 first-event identity/lifecycle/schema coordination separate; #8531 implementation and its review fixes remain with its author. No receiver-event or public-schema expansion is requested here. I found no concrete newly introduced P0-P2 defect in this static review. I did not run this fork's tests, a served model, or GPU validation; the earlier #8575 H100 example does not validate this branch. This comment is coordination, not a formal approval or a request to merge. AI assistance: TRAE assisted with the source/RFC inspection and drafting. |
|
Thanks @EchoHayate, this answers the dependency question. I'll keep the PR scoped as you describe: That leaves the two questions for the scheduler and metrics owners: stamp in the scheduler scan or at the gate-release sites, and the field names. @tzhouam @lishunyang12, could one of you decide these two? I'll change the code to match either choice. AI assistance: drafted with Claude Code; I reviewed it before posting. |
…-wait # Conflicts: # tests/distributed/omni_connectors/test_chunk_transfer_adapter.py
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Let people who investigate downstream-stage latency see how long a request waited for its input and how long it waited for scheduling after the input was ready, so that queueing bottlenecks are misjudged less often.
2. What is the highest-leverage variable for that outcome?
Whether the time at which the input gate releases the request can be recorded accurately. Without this boundary, the two kinds of waiting can only stay mixed together.
3. How will this work move that variable?
It records the time when the input gate first releases the request, passes it to the stats side, and splits the existing queued time into two parts. When the split cannot be made reliably, the new fields are left empty, and the original field stays unchanged.
4. Whose customer or user needs does this address, and what are those needs?
Mainly developers and operators who troubleshoot latency in multi-stage inference. They need it to decide whether to look next at upstream input delivery or at downstream scheduling.
Is a PR needed? Yes. This boundary has to be recorded by the scheduler and passed to the stats side; changing the docs alone cannot produce the data. The related code, tests and documentation belong together in one PR.
Purpose
This is the follow-up suggested in #8575 (merged as 0d4d09c), which added the engine-core phase split to
StageRequestStatsand left input wait insidevllm_queued_ms.Before. On a downstream stage that waits for its input,
vllm_queued_ms(QUEUEDto firstSCHEDULED) also counts the time before that input arrives:async_chunk: the orchestrator submits a chunk-receiving stage together with stage 0, and the input gate holds the request inWAITING_FOR_CHUNKuntil the first upstream chunk lands.requires_full_payload_input: the request stays inWAITING_FOR_INPUTuntil the connector delivers the payload.So a large
vllm_queued_msdoes not say whether the stage waited for upstream input or for a scheduling slot. One case from a 2026-10-16 H100 run, taken with #8575's code before this PR: Qwen3-TTS (Qwen/Qwen3-TTS-12Hz-1.7B-CustomVoice,--async-chunk --enforce-eager). The first request after server start reported a stage-1vllm_queued_msof 86,443 ms. The server log shows the request reaching stage 1 at 06:41:18 and stage 1 polling for its input until stage 0 enqueued its first chunk at 06:42:44, about 86 s later, so almost all of it was input wait. That request was a cold start that included a one-time FlashInfer JIT build (Building JIT module sampling); the next eight requests reported 53-421 ms. Fromvllm_queued_msalone, the cold request looks the same as a scheduling backlog.After. With
--log-stats, the scheduler of a stage with an input gate records, once per request, the first step in which the request is leftWAITINGin a waiting queue after the gate ran. The time is sent with the engine-core events, and the output processor splitsvllm_queued_ms:vllm_input_wait_msQUEUEDevent to the step in which the stage's input gate first saw the input readyvllm_ready_queued_msSCHEDULEDeventvllm_queued_ms.schedule()takes itsSCHEDULEDtime. The generation scheduler stamps with theSCHEDULEDtimestamp it takes before its gates run, so a request scheduled in the step its input arrived reportsvllm_ready_queued_ms= 0.None(not measured, as opposed to a measured 0) on stage 0, on a downstream stage without an input gate, with--log-statsoff, and when the recorded time falls outside the queued interval, for example an AR streaming-input split that covers a later segment.active_stream_window, a request without an active-stream slot receives no chunks, so its wait for a slot counts as input wait.Contract.
docs/design/metrics.md, section "LLM-stage phase split inStageRequestStats": two table rows; the sentence about matching upstreamFinishedRequestStatsnow covers only the first four fields; two new paragraphs on the semantics, the precision and theNonecases; and the sentence "The split does not separate input wait." is removed.docs/design/module/observability.md(draft) has no per-field content and is unchanged.What users see. Two more rows in the per-request
[StageRequestStats]table (DEBUG,--log-stats), and two more keys in every stage row ofbuild_and_log_summary()["stage_table"](Nonewhere not measured).Compatibility.
OmniEngineCoreOutputgetsinput_ready_ts: float | None = None, set only on outputs that carry engine-core events. When unset it adds one byte per output on the msgpack wire (vLLM'sMsgpackEncoder: 28 to 29 bytes for one default output, 107 to 110 for a batch of three inOmniEngineCoreOutputs), as the earlieris_coalescedandnum_generation_tokensfields do.Noneon every stage (existing behavior), so a measuredvllm_ready_queued_msof 0 appears only in the summary.Not changed. The values of
vllm_queued_ms,vllm_prefill_ms,vllm_decode_msandvllm_num_preemptions; waits for later chunks (still insidevllm_decode_mswithasync_chunk); diffusion stages; Prometheus families and labels; thestage_metricssnapshot, so the benchmark JSON; the duplex per-response table, which leaves the new fields out like the rest of the split.Related.
_waiting_sincein the input coordinator and the chunk adapter. This PR records the time in the scheduler instead; question 1 below.consumer_first_accept, core in [Core] Add request-owned stream-edge first-event recording #8531, still open), and my comment on [RFC]: Request-scoped stream-edge first-event telemetry #6472 said this work would follow that boundary. This PR adds no edge event: both endpoints are engine-core monotonic timestamps of the receiving stage, so no cross-process clock is involved, and only the first input of engine-core stages is covered. Question 3 below.omni_generation_scheduler.py) and [Frontend] Emit per-turn --log-stats tables for native duplex #6892 (metrics/stats.py) already conflict with main in those files, and this branch adds no new conflicting file. [Bugfix] Preserve full-payload scheduling in OmniGenerationScheduler #8324 also changes the full-payload path of the generation scheduler: it defers payloads that do not fit the step budget and drops the fallback to upstreamschedule(). This PR stamps before that path, so a payload deferred there would count asvllm_ready_queued_ms; this is reasoned from the diff, not tested with both changes. [Core][Frontend] Add OpenTelemetry tracing for Omni inference #8836 and [Core] Add request-owned stream-edge first-event recording #8531 merge cleanly with this branch.Questions for reviewers:
_stamp_input_ready): one place, only in the schedulers, covering the coordinator and both chunk-adapter gate paths. Stamping next to each_waiting_since.popinOmniSchedulingCoordinatorandOmniChunkTransferAdapterwould sit closer to the gate decision, but touches five call sites in two files. Which do you prefer?vllm_queued_ms,vllm_prefill_msandvllm_decode_msare intervals between engine-core events, while the input-ready time here is recorded by the Omni scheduler. Keepvllm_input_wait_ms/vllm_ready_queued_ms, or rename?Test Plan
vLLM Version: v0.31.0 (
db9527a468, theVLLM_VERSIONindocker/Dockerfile.ci), used as a source tree onPYTHONPATHwithout compiled extensions (vllm._Cabsent).vLLM-Omni Commit:
9913d9f2019ecba9e5987157763eaa1352d7b8eb(4 commits on upstream/main303b8df65)Environment: macOS 26.6 (arm64), Python 3.12.14, torch 2.13.0 CPU, pytest 9.1.1, msgspec 0.22.0. No GPU; not the CI image. Other dependencies come from an existing local virtualenv and from wheels already unpacked in the local uv cache, put on
PYTHONPATH; nothing was installed for these runs.janus,soundfile,wcwidthandtorchaudioare not available. Where noted, the first three are replaced by empty stub modules so that a file can be collected; a test that really used one of them would fail.Lint with the versions pinned in
.pre-commit-config.yaml, on the changed files: ruff 0.14.10checkandformat --check, typos 1.49.0, markdownlint-cli2 0.23.2 ondocs/design/metrics.md,tools/pre_commit/mypy.py 3.10, andtools/pre_commit/{check_test_marks,check_spdx_header,check_forbidden_imports,check_torch_cuda}.py.Test Result
9913d9f20)303b8df65)test_stats.pytest_orchestrator.pystage-pool teststest_engine_session.pyjanus(14),soundfile(5) andwcwidth(2), and 8SharedMemoryConnectortests that need/dev/shm, which macOS does not have. The 2 tests deselected in run 1 render the table and needwcwidth; run 3 covers them.waitingand notkv_holding_waiting, sending the input-ready time on outputs without engine-core events, and treating a sender-onlyasync_chunkstage as gated each make one test fail.tools/pre_commitchecks andgit diff --checkare clean; markdownlint reports 0 issues. mypy adds no errors: the 5 inoutput_processor.pyand the 3 intest_engine_session.pyare also on upstream/main.Not run:
vllm_input_wait_ms/vllm_ready_queued_msnumbers from a served model on this branch yet; the H100 case above used [Core] Add LLM-stage queue/prefill/decode split to StageRequestStats #8575's code.--log-statson vs off.requires_full_payload_inputpath with real connector delivery; only CPU unit tests cover it.schedule(), and a gate that releases a request whose first chunk carries no token (the generation fast path would then skip it, so the stamp would be early and the extra time would land invllm_ready_queued_ms). Both are reasoned from the code, not tested.tests/core/sched/test_omni_ar_scheduler_payloadless_outputs.pyandtests/core/sched/test_omni_ar_scheduler_streaming.py, which needtorchaudio.pre-commit run; the hooks above were run one by one, and trailing newlines, line endings and debug statements in the changed files were checked by hand. shellcheck and the Buildkite check do not apply to these files.AI assistance: I used Claude Code to implement and test this change and to draft this description; I reviewed every change and can explain it.