Repository navigation
[Bugfix] Reuse one Qwen3-Omni fault payload across concurrent requests - #9407
amy-why-3459 merged 3 commits into
Conversation
…sh espeak. Reliability workers were building speech inside the thread pool. pyttsx3 shares one process-global espeak callback, and overlapping engines segfault pytest. Signed-off-by: yenuo26 <410167048@qq.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 matches CODEOWNERS paths: /tests/. Code owners: @NickCao Routing: @NickCao via CODEOWNERS @yenuo26, 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 noteResolved as of |
…quests. Process-kill workers were synthesizing speech with pyttsx3. Reuse one multimodal body so they only send HTTP. Signed-off-by: yenuo26 <410167048@qq.com>
|
@amy-why-3459 PTAL |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the clear root-cause write-up on #9405 — concurrent generate_synthetic_audio / pyttsx3 from the process-kill thread pool is a convincing explanation for the pytest SIGSEGV while the server was already returning 500.
What looks good
- Caching one multimodal body per model in
_multimodal_requestmatches howProcessKillWorkerNoLoadactually runs:warmupandassert_fast_failboth call it on the main thread before_assert_concurrent_all_failsubmitssubmit_concurrentinto the pool (concurrent_workers=4). After that, the four workers only serialize HTTP and no longer touch espeak. - Scoped to the reliability test helper;
tests/helpers/media.pyand other single-threadedgenerate_synthetic_audiocall sites stay untouched, which is a reasonable cut for this failure mode. - Returning a fresh top-level dict (and a new
modalitieslist) while reusing the cached payload avoids accidental cross-request mutation of the request envelope.
Notes (non-blocking)
- The cache populate path is still check-then-set with no lock (see inline). Safe given the current main-thread warmup / fast-fail ordering; a cold hit from the pool would reintroduce overlapping pyttsx3. A small
threading.Lockaround populate, or building the body once outside_assert_concurrent_all_failand passing it in, would make the invariant local rather than call-order dependent. messagesis shared by reference across concurrent submits. Fine while callers only JSON-encode it; worth keeping that assumption if anything later mutates the message list in place.- Title says “Serialize … TTS”; the change is request reuse/caching, not a lock around espeak — minor naming nit vs the implementation.
- No local re-run of the H100 process-kill case is claimed; Buildkite confirmation on this head is the real signal for #9405.
Direction looks right for stopping the client-side espeak crash on this path.
| cached = _MULTIMODAL_REQUESTS.get(model) | ||
| if cached is None: | ||
| image_data_url = f"data:image/jpeg;base64,{generate_synthetic_image(224, 224)['base64']}" | ||
| audio_data_url = f"data:audio/wav;base64,{generate_synthetic_audio(5, 1)['base64']}" | ||
| messages = dummy_messages_from_mix_data( | ||
| system_prompt=_system_prompt(), | ||
| image_data_url=image_data_url, | ||
| audio_data_url=audio_data_url, | ||
| content_text="Describe the image and the audio in one short sentence.", | ||
| ) | ||
| cached = {"model": model, "messages": messages, "stream": False, "modalities": ["text", "audio"]} | ||
| _MULTIMODAL_REQUESTS[model] = cached |
There was a problem hiding this comment.
Non-blocking: this get → build → assign sequence has no lock. With ProcessKillWorkerNoLoad it is fine because warmup / assert_fast_fail populate the cache on the main thread before _assert_concurrent_all_fail fans out submit_concurrent. If a future caller hits a cold cache from the pool, several workers could still enter generate_synthetic_audio together. A threading.Lock around the populate, or constructing the body once outside the executor and reusing that object, would make the “no concurrent pyttsx3” guarantee independent of call order.
yenuo26
changed the title
|
@amy-why-3459 Updated the title to |
PLEASE FILL IN THE PR DESCRIPTION HERE.
Contributor intent (required human answers)
Answers below are the public description in #9405, which this PR fixes.
1. What outcome do you want to achieve?
The Qwen3-Omni process-kill reliability test should finish and observe HTTP 500 after the stage-0 worker is killed. It must not abort the pytest process with SIGSEGV.
2. What is the highest-leverage variable for that outcome?
Concurrent generate_synthetic_audio calls. The process-kill checker builds a multimodal body on each worker thread, and pyttsx3/espeak is process-global.
3. How will this work move that variable?
Build the short multimodal request once on the calling thread. The process-kill thread pool then sends that same body and does not call pyttsx3.
4. Whose customer or user needs does this address, and what are those needs?
Buildkite reliability CI for Qwen3-Omni. The job needs the fault injection to complete and check the API error, instead of dying inside espeak while the server is already returning 500.
Purpose
Before this change, ests/dfx/reliability/test_qwen3_omni.py::test_reliability_fault_process_kill_worker_no_load[qwen3_omni_reliability_async_chunk__sigterm] exits 139. The server already returns HTTP 500 after StageEngineCoreProc_stage0 is killed. The pytest process segfaults because four worker threads each call _multimodal_request, which calls generate_synthetic_audio. espeak keeps one synth callback and a fixed 350-entry voice table (N_VOICES_LIST).
After this change, _multimodal_request builds the image, audio, and message body once per model. warmup runs on the main thread before the pool, so that first call synthesizes the audio. �ssert_fast_fail and submit_concurrent reuse the same body. The four workers only send HTTP.
Fixes #9405
Test Plan
vLLM Version: 0.32.0 on the failing Buildkite job.
vLLM-Omni Commit: 0881d06 on top of main 2c32448
No new pytest was added. The change is request reuse inside the existing reliability test:
ext tests/dfx/reliability/test_qwen3_omni.py::test_reliability_fault_process_kill_worker_no_loadThat test needs two H100s and Qwen/Qwen3-Omni-30B-A3B-Instruct.
Test Result
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
Note
To avoid agent spam, vLLM-Omni limits contributors without write access to 5 open PRs. If this blocks well-intentioned critical work, please contact a committer to request bypass list access.