Repository navigation
Conversation
Signed-off-by: lm10-arg <347490427+lm10-arg@users.noreply.github.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. |
|
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/ar_runtime.md, docs/design/module/benchmarking.md, docs/design/module/model_integration.md, docs/design/module/observability.md. Module owners: @alex-jw-brooks @tzhouam @Bounty-hunter Routing: @alex-jw-brooks via module of the changed files, module named in the PR description, CODEOWNERS; @tzhouam via module of the changed files, module named in the PR description, CODEOWNERS; @Bounty-hunter via module of the changed files, module named in the PR description @lm10-arg, 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. |
hsliuustc0106
left a comment
There was a problem hiding this comment.
- P2 — stop the request producer when the first timeout completes. @lm10-arg, at the new gather call,
_gather_request_outputsstarts only afterget_request(...)has emitted the entire workload. With finite-rate or self-timed arrivals, an earlier request can time out while that producer is still waiting; subsequent requests continue to be dispatched until the last scheduled arrival, despite--omni-fail-fast-on-timeout. Monitor timeout completion concurrently with production and stop/cancel the producer as well as outstanding requests. Add a regression through the benchmark dispatch path with delayed arrivals: after the first timeout, assert no later request is sent, cancellation cleanup is awaited, completion is bounded, and completed outputs retain their order. The direct gather-helper tests do not exercise this producer/monitor ordering.
Source-only inspection at 5ebde0d8f95ed6e0efd81c5c3e92c66e1b083355; no local runtime or GPU test executed.
amy-why-3459
left a comment
There was a problem hiding this comment.
Reviewed the opt-in Code2Wav watchdog and --omni-fail-fast-on-timeout path on tip 5ebde0d8.
Direction is sound for the AMD Stage-2 hang diagnosis goal: phases reset the deadline, reports fire once without interrupting the hung op, details are bounded, and the gather helper preserves completed outputs in request order while cancelling the rest. Default-off (timeout_s=0 / flag off) keeps serving semantics unchanged. Unit coverage for report/phase/close and the gather-helper contract looks solid.
Points:
-
Producer vs fail-fast (already flagged).
_gather_request_outputsonly runs after the request-production loop has emitted the full workload. With delayed / rate-limited arrivals, later requests can still be dispatched after an earlier request has already timed out. Fail-fast needs to observe timeout completion concurrently with production and stop the producer, with a regression through that path — the direct gather tests do not cover it. -
Timeout classification.
timed_out=Trueis set only onasyncio.TimeoutError/TimeoutError. If the shared session raisesaiohttp.ServerTimeoutError(or anotherClientErrorsubclass) for the same total/sock deadline, the request lands in the generic client-error path withouttimed_out, so fail-fast never trips. Please confirm which exception the 900s omni session actually raises and treat that family as structural timeouts too. -
Watchdog detail merge.
phase()does{**operation.details, **new}, so keys from earlier phases linger on later reports (e.g.codec_framesstill present underdecode/cfm). Prefer replacing details per phase (or namespacing) so a stall report reflects the blocked phase only.
Covered: code2wav_watchdog.py, BatchedToken2Wav / Code2Wav phase hooks, and the benchmark gather/CLI wiring. No local ROCm run.
Signed-off-by: lm10-arg <347490427+lm10-arg@users.noreply.github.com>
|
Author self-review at
Validation: full No known source-level blocker remains from this self-review. The exact-head AMD MiniCPM-o accuracy run is still the required runtime gate; this PR continues not to claim that it fixes the underlying ROCm/GPU hang. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Re-reviewed the delta on tip 761b86a ("Close timeout diagnostic review gaps").
Prior points look addressed:
-
Fail-fast scope.
_validate_fail_fast_on_timeoutnow rejects anything other than immediateopenai-chat-omni+--request-rate inf+ non-self-timed beforeset_fail_fast_on_timeout/main_async, with CLI help and unit coverage. That closes the producer/delayed-arrival gap for the MiniCPM-o Stage-2 path without claiming fail-fast works under rate-limited or self-timed dispatch. -
Timeout classification.
aiohttp.ServerTimeoutErroris in the structural timeout catch, with a regression assertingtimed_out=True. -
Watchdog details.
phase()replaces details via_bounded_details(details)(handlesNone→{}), and the new unit test checks stale keys do not linger across phases.
Also good: Seed-TTS _validate_seed_tts fails closed when timed_out_requests / cancelled_after_timeout_requests are nonzero, with a high-scoring partial-result regression.
No new issues from this tip. Source-only inspection; no local ROCm run.
andyluo7
left a comment
There was a problem hiding this comment.
Approved exact head 761b86a. The exact synthetic merge with main 14874d0 passed GitHub checks plus CUDA build 18080, AMD build 14632, and NPU build 9825. AMD and NPU MiniCPM-o accuracy each completed all 1,088 Seed-TTS requests with zero failures, timeouts, or cancellations, and both configured accuracy suites passed. The prior review thread is resolved.
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Localize the intermittent MiniCPM-o 4.5 Stage-2 Code2Wav hang seen in AMD nightly CI and prevent one permanent hang from producing repeated 900-second client-timeout waves until the job-level timeout.
2. What is the highest-leverage variable for that outcome?
Code2Wav phase and owner-thread stack observability at the stall, together with benchmark cancellation immediately after the first structurally identified request timeout.
3. How will this work move that variable?
Add an opt-in, per-model no-progress watchdog that records phase transitions and reports the blocked Python stack, plus an opt-in benchmark circuit breaker that preserves completed outputs and cancels every remaining queued or in-flight request after the first timeout.
4. Whose customer or user needs does this address, and what are those needs?
CI maintainers and model contributors need actionable evidence about the blocked Stage-2 operation and bounded accelerator time, while retaining partial benchmark metrics and artifacts for diagnosis.
Purpose
The soft failure in AMD build 14607 is part of an intermittent pattern also seen in builds 14528, 14598, 14604, and 14606 across different nodes. Builds 14541 and 14600 are passing controls. The failure boundary is a permanent Stage-2 Code2Wav stall under the nightly's ROCm concurrency of 10; the exact blocked operation is not identifiable from the current logs. The later 900-second client
TimeoutErrorwaves are downstream symptoms.Before this change:
After this change, when explicitly enabled:
VLLM_OMNI_CODE2WAV_WATCHDOG_TIMEOUT_Sreports setup, encoder, CFM mode, cache materialization, HiFT, or output-copy phase details and the blocked owner-thread Python stack once per operation without interrupting it;--omni-fail-fast-on-timeoutis limited to immediately dispatchedopenai-chat-omniworkloads (--request-rate inf, non-self-timed). Unsupported dispatch modes are rejected before the benchmark starts. Python and aiohttp server timeouts are marked structurally; remaining tasks are cancelled and awaited; completed outputs stay in request order; and timeout/cancellation counts are recorded;This PR does not claim to fix the underlying ROCm/GPU hang. It adds the evidence needed to identify the blocked operation and bounds the repeated-timeout failure mode.
The public environment-variable guide is updated. Existing benchmarking, observability, model-integration, and autoregressive-runtime design pages are draft contracts; this opt-in diagnostic behavior does not change their serving or output semantics.
Implementation and PR preparation were assisted by OpenAI Codex and reviewed with the repository's full
precheck-prworkflow.Test Plan
Reproduction in the affected AMD CI environment uses the existing nightly node at concurrency 10:
pytest -s -v \ tests/e2e/accuracy/minicpmo_4_5/test_minicpmo_4_5.py::test_minicpmo_4_5_seed_tts_wer_bench \ -m "full_model and omni and rocm and MI325 and cards_1" \ --run-level full_modelAdded regression coverage verifies:
aiohttp.ServerTimeoutErrorchat deadlines are marked withtimed_out=True;vLLM Version: v0.32.0 API baseline
vLLM-Omni Commit:
761b86ade040ad3da82552212b46c3b5b05a14baTest Result
Passed:
pre-commit run --from-ref origin/main --to-ref HEADtorch.cudaguard, and Buildkite schema validation all passed.python3 -m py_compilefor every changed Python file.close()terminated the daemon monitor thread.git diff --check origin/main...HEAD.Not run locally:
ImportError: Error importing plugin "tests.helpers.fixtures.config": No module named 'torch').Need help on this PR? Tag
@codesmith-botwith what you need.