Repository navigation
fix: include diffusion scheduler wait in queue metrics - #8170
Asthenia0412 wants to merge 2 commits into
Conversation
Signed-off-by: yancy <yancyxiaox8@gmail.com>
14ba895 to
41c1c06
Compare
|
This PR appears to belong to: docs/design/module/observability.md, docs/design/module/diffusion/index.md. Module owners: @david6666666 @Isotr0py @princepride Routing: @david6666666 via module named in the PR description; @Isotr0py via module named in the PR description; @princepride via module named in the PR description @Asthenia0412, 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. |
Signed-off-by: yancy <yancyxiaox8@gmail.com>
| - `stage_gen_time_s`, `stage_in_queue_s`, image workload, inference-step, and per-stage peak-memory values are emitted from `StageRequestStats` when a stage finishes. `image_pixels` and `image_count_total` are restricted to image outputs; `num_inference_steps` is restricted to diffusion stages. | ||
| - Diffusion engine timings are accumulated per request by `OrchestratorAggregator.accumulate_diffusion_metrics`. Engine-side millisecond values are converted to seconds before `observe_modality_at_finalize(...)` dispatches the per-replica Histogram observations. | ||
| - `request_queue_wait_s` is emitted once at final request completion from `pipeline_timings["queue_wait_ms"]`. A present value of `0` is a valid observation; a missing key produces no sample. | ||
| - `request_queue_wait_s` is emitted once at final request completion. It includes the orchestrator's `pipeline_timings["queue_wait_ms"]` and, when present, diffusion's `scheduler_queue_wait_s`; these are sequential admission queues. A measured `0` is a valid observation, while a missing value produces no sample. |
There was a problem hiding this comment.
Minor: the combined value also overlaps stage_gen_time_s / stage_*_gen_ms. The diffusion admission wait is still measured inside the stage's generation window (that is where #5924 observed the missing ~135s), so for diffusion stages queue_wait + stage_gen_time now double-counts it. One sentence noting the overlap would keep consumers from reconstructing E2E latency as a sum of the two.
| obj.prom_metrics.set_peak_memory.assert_not_called() | ||
| obj.mod_metrics.observe_image_ttfp.assert_called_once_with("1", "0", 0.25) | ||
|
|
||
| def test_queue_wait_response_and_histogram_include_diffusion_admission(self, mocker) -> None: |
There was a problem hiding this comment.
None of the lanes on this PR execute this suite (the GitHub build lanes don't run pytest and no test-trigger label is set), and the PR body notes pytest couldn't run locally — so these assertions, including the 135131.0 / 135.131 expectations, haven't been executed anywhere. Could you run pytest -q tests/metrics/test_emit_calls.py on a vllm-capable environment (or flag which lane will pick it up)?
Omni ReviewBot routing recordAssigned Direct under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
PR description
Diffusion requests can sit in the stage scheduler after the Orchestrator has already handed them off. This change adds that later admission wait to the existing Orchestrator queue_wait_ms so the response field and the request_queue_wait_s histogram report the full sequential queue time, while stage_in_queue_s stays a per-stage diagnostic. Present zeros are still emitted and missing sources still produce no sample. The user-visible effect is that serialized diffusion jobs no longer look almost unqueued on the request metric while their wait is buried in stage generation time.
Change flow
flowchart TD
A["[EXISTING] Orchestrator queue_wait_ms and diffusion scheduler_queue_wait_s"]:::existing
B["[CHANGED] extract_queue_wait_s sums sequential waits"]:::changed
C["[CHANGED] OmniBase._process_single_result writes combined queue_wait_ms"]:::changed
D["[EXISTING] Response metrics and request_queue_wait_s histogram"]:::existing
E["[NEW] Combined-wait regression tests in test_emit_calls.py"]:::new
F["[CHANGED] docs/design/metrics.md Path 2 wording"]:::changed
A --> B --> C --> D
B --> E
C --> F
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
CI at
9ab163aa7fca(2026-09-26T20:24:45.627414+00:00): required check(s) blocking:buildkite/vllm-omni(missing), and -5 more.
Findings
- [P1] Combined-wait regression tests have no execution evidence —
tests/metrics/test_emit_calls.py:347
The new pins inTestQueueWaitExtractionandtest_queue_wait_response_and_histogram_include_diffusion_admissionare the only check that_process_single_resultwrites135131.0ms and callsobserve_queue_wait(135.131). The PR Validation section states those tests were not run locally becausevllmis missing. Frozen CI for this head at 2026-09-26T20:24:45Z showspre-commitand Read the Docs passed; the requiredbuildkite/vllm-omnicheck is missing, and the GitHub checks that did run do not execute pytest..buildkite/**is not materialized here andgit showcould not be executed, so this does not name a lane or selector. The assertions still need a vllm-capable pytest run, or the required Buildkite check needs to report, before the 135s combine path is trusted.
Existing thread: #8170 (comment) - [P3] Combined queue_wait still undocumented as overlapping stage_gen_time —
docs/design/metrics.md:78
extract_queue_wait_snow addsscheduler_queue_wait_sinto responsequeue_wait_msandrequest_queue_wait_s. The catalog still describesstage_gen_time_sas including in-stage queueing, which is where the missing ~135s admission wait was observed. Summing the two families therefore double-counts that wait. Path 2 at this line describes the combine but does not warn consumers; the table row forrequest_queue_wait_sstill says "Orchestration-layer queue wait", which no longer matches the implementation. Add one sentence that the combined wait already sits inside stage generation time, and update the table row to the combined meaning.
Existing thread: #8170 (comment)
Omni ReviewBot: finding feedback[p1] Combined-wait regression tests have no execution evidence — The new pins in This finding appears in the bot's COMMENT review, but GitHub could not place it as an inline diff comment. If you are the PR author and disagree, react 👎 to this comment. The disagreement will be shown to the maintainer; it does not approve or merge the PR. |
Omni ReviewBot: no human activity for 7 days@Asthenia0412 this pull request has had no human commit, comment or review since 2026-09-26. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot: no human activity for 14 days@Asthenia0412 this pull request has had no human commit, comment or review since 2026-09-26. Please consider marking this PR as draft until work can resume. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
What changed
Requests to diffusion models can wait in the diffusion scheduler after the Orchestrator has already handed them to a stage. That admission delay was recorded separately, so
queue_wait_msand the request queue-wait histogram showed only the earlier Orchestrator wait. This change combines the two sequential waits for the response metric and the aggregate request histogram. The per-stagestage_in_queue_sobservation remains available for diagnosing a specific scheduler.I added regression coverage for the combined value, scheduler-only waits, valid zero waits, and the response/Prometheus metric path. The metrics design doc now describes the combined meaning.
Fixes #5924
Validation
metrics/utils.pyand is skipped in the project's CI configuration.vllmpackage installed. The focused test command ispytest -q tests/metrics/test_emit_calls.py.