Repository navigation
[CI/Build] Clean up Qwen3-Omni nightly tests - #6570
Conversation
Signed-off-by: psv666 <2693925048@qq.com>
|
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 be related to model: qwen-omni. Model owners: @amy-why-3459 @psv666 @psv666, 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. |
|
@yenuo26 @amy-why-3459 PTAL |
|
Thanks for the cleanup — the CUDA nightly/weekly split is the right
Smaller: the 40-request The SPDX copyright-line tweak and the extra parametrize on the |
| [ | ||
| { | ||
| "test_name": "test_qwen3_omni_chunk_cuda_nightly", | ||
| "mark": [ |
There was a problem hiding this comment.
Why add a new JSON? Can't we just use Mark to distinguish it?
There was a problem hiding this comment.
Good point. I checked the perf loader: marks are attached per top-level config object, so separate files do not provide additional isolation. I merged the NPU, CUDA nightly, and CUDA weekly entries into test_qwen3_omni_async_chunk.json. The existing slow marker selects the weekly entry, nightly uses not slow, and both temporary CUDA JSON files were deleted. CI commands and the L4 perf documentation were updated in a0051ea.
| @hardware_test(res={"cuda": "H100", "rocm": "MI325"}, num_cards=2) | ||
| @pytest.mark.parametrize("omni_server", test_params + test_token_params, indirect=True) | ||
| @pytest.mark.parametrize("omni_server", test_params + reduced_token_params, indirect=True) | ||
| def test_mix_to_text_audio_001(omni_server, openai_client) -> None: |
There was a problem hiding this comment.
I recall that we said this test case could be deleted?
There was a problem hiding this comment.
Yes. It was initially retained because the skipped case consumed no runtime, but that also means it provides no executable coverage. Since this PR is cleaning up dead and redundant cases, keeping the no-op case is not useful; it is deleted in a0051ea.
|
|
||
| @hardware_test(res={"cuda": "H100", "rocm": "MI325"}, num_cards=2) | ||
| @pytest.mark.parametrize("omni_server", test_params[:1], indirect=True) | ||
| def test_invalid_audio_format_rejected(omni_server, openai_client) -> None: |
There was a problem hiding this comment.
We may need to check whether it is covered in xxx. If not, we should add this test case there.
There was a problem hiding this comment.
I checked the exact contract rather than treating it as obsolete. The existing MiniCPM case validates an invalid input-audio container, while this Qwen3-Omni regression validates unsupported output audio.format in chat completions. The same aac request is now in tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py, using the HTTP helper and asserting 400 plus the supported-format message. The L4 copy is removed only after that migration.
| { | ||
| "test_name": "test_qwen3_omni_chunk_cuda_nightly", | ||
| "mark": [ | ||
| { |
There was a problem hiding this comment.
I recall that we mentioned some multi-replica performance test scenarios could also be removed. Additionally, if the omni test scenarios have already been trimmed, we may want to check whether the corresponding scenarios in vllm-text are redundant as well.
There was a problem hiding this comment.
I compared the input/output contracts. Multi-replica already covers Async Chunk default text+audio for random and random-mm at 10/40/100, so those default-output cases were removed from the single-replica Async Chunk CUDA entries. The text-only output cases remain because multi-replica does not cover that contract: random 1/8/32 runs weekly and random-mm 10/100 runs nightly. I also trimmed vllm-text to the same 1/8/32 and 10/100 comparison points. Multi-replica keeps the 40-request image+video point.
congw729
left a comment
There was a problem hiding this comment.
Thanks for working on reducing the Qwen3-Omni nightly workload. I left two suggestions about preserving regression coverage while consolidating these cases.
|
|
||
| @hardware_test(res={"cuda": "H100", "rocm": "MI325"}, num_cards=2) | ||
| @pytest.mark.parametrize("omni_server", test_params[:1], indirect=True) | ||
| def test_invalid_audio_format_rejected(omni_server, openai_client) -> None: |
There was a problem hiding this comment.
This test was added as regression coverage when #4718 fixed #4716, where audio.format in chat completion requests was incorrectly handled. If we remove it directly from this expansion suite, Qwen3-Omni will no longer have equivalent coverage for rejecting unsupported audio formats.
Moving the test out of this L4 suite makes sense, but could we first migrate it to tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py, and then remove it from here?
There was a problem hiding this comment.
Agreed on preserving the regression contract. I verified that _resolve_audio_format still rejects aac, then migrated the request to tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py with the HTTP helper and 400/message assertions before removing the expansion copy. The target collects under the existing weekly H100 invalid-parameter job, and the audio-format resolver plus perf metadata unit tests pass (40 passed).
| @@ -139,28 +138,6 @@ def get_max_batch_size(size_type="few"): | |||
|
|
|||
| @hardware_test(res={"cuda": "H100", "rocm": "MI325"}, num_cards=2) | |||
| @pytest.mark.parametrize("omni_server", test_params, indirect=True) | |||
There was a problem hiding this comment.
One personal observation: after removing test_text_to_audio_001, test_text_to_text_audio_001 is now parametrized with only test_params, so the reduced-token configuration previously attached to this test is no longer covered.
Since this case sends five concurrent text requests and get_batch_token_config() is intended to exercise the smaller max_num_batched_tokens path, I suggest keeping test_params + reduced_token_params on test_text_to_text_audio_001.
There was a problem hiding this comment.
Accepted in e710314. I initially kept the reduced-token configuration only on the 120-second audio stress case, but after reconsidering the coverage boundary, the five-request text-only path can exercise token batching behavior that is distinct from long multimodal preprocessing. Preserving that exact reduced max_num_batched_tokens regression path is worth the additional configuration. test_text_to_text_audio_001 now uses test_params + reduced_token_params; collect-only confirms default, async_chunk, and batch_token_2048.
Signed-off-by: psv666 <2693925048@qq.com>
|
@amy-why-3459 I addressed the review points, with the latest adjustment in e710314:
Also: the 40-request image+video point remains in multi-replica; single-replica Async Chunk and vLLM-text retain the 10/100 endpoints. The Async Chunk entries are now in one JSON selected by H100/NPU plus slow/not slow. Every Async Chunk random input/output length is reduced from 2500/900 to 100/100, and the affected H100 baseline was removed pending remeasurement. The perf documentation is updated, and post-cleanup expansion collection is 26 tests. |
|
Author self-review:
I did not find an additional correctness issue after these checks. |
|
Thanks for the follow-up in a0051ea — the first-round items look addressed: Two things before this can land:
Non-blocking: the CHELSIE+Chinese fold and dropping the 40-req |
Signed-off-by: psv666 <2693925048@qq.com>
… and enhance performance metrics Signed-off-by: psv666 <2693925048@qq.com>
…ghtly-cleanup Signed-off-by: psv666 <2693925048@qq.com> # Conflicts: # tests/dfx/reliability/invalid_param_test/test_invalid_omni_chat.py # tests/e2e/online_serving/test_qwen3_omni_expansion.py
Signed-off-by: psv666 <2693925048@qq.com> Signed-off-by: AndyZhou952 <jzhoubc@connect.ust.hk>
Signed-off-by: psv666 <2693925048@qq.com>
Signed-off-by: psv666 <2693925048@qq.com>
Purpose
CHELSIEspeaker lookup and Chinese-language output intest_speaker_001;test_text_to_text_audio_001, in addition to the stronger long-audio stress case;modalities=["audio"], while the expansion suite focuses on broader output cases.slowscheduling marker.randomtext-only coverage to weekly at concurrency 1/8/32. Keep nightlyrandom-mmtext-only endpoints at 10/100 requests.randomworkload from input/output lengths 2500/900 to 100/100 to lower test pressure. Remove the affected weekly H100 latency baseline because it must be remeasured for the new workload.randomand 10/100 forrandom-mm. The multi-replica suite retains its 40-request image+video point.Test Plan
vLLM Version: 0.27.0
vLLM-Omni Commit: e710314
Test Result
test_speaker_001[default]: passedtest_speaker_001[async_chunk]: passed40 passed21 passedtest_text_to_text_audio_001: default, async_chunk, and batch_token_2048 selectedgit diff --check: passedpre-commiton all changed files: passedPrevious GPU model inference pytest log
This log validates the unchanged consolidated speaker/language request path. The final cleanup changes other cases, so its module-level collection count is intentionally superseded by the post-cleanup collect-only result above.