Repository navigation
Conversation
|
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 @yenuo26 Routing: @NickCao via CODEOWNERS; @yenuo26 via CODEOWNERS @pujitha24, 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. |
|
Self-review: this only changes test assertions/config, no runtime code.
I verified both thresholds against the actual CI-reported transcripts using the real |
linyueqian
left a comment
There was a problem hiding this comment.
Approving at 410fa7fa. Test-only, and it fixes the two Higgs Audio v3 inline-control flakes at their actual cause rather than by loosening the shared gate: assert_audio_speech_response now reads an optional per-test transcript_similarity_threshold (default unchanged at 0.9), the onomatopoeia case sets 0.75 with the observed 0.839 documented in the docstring, and the whispering case escalates to large-v3 before failing, the same path test_plain_text_wav already uses. The margins are argued from real CI transcripts linked to #7236. Reviewed statically; fork head, no PR code executed. No lane had run on this head, so I added ready; merge on green.
|
Approved above; the only thing between this and a merge is that #7358 just landed on |
410fa7f to
1a35b62
Compare
1a35b62 to
00590f2
Compare
|
Thanks for the quick rebase. The new lane (15313) failed only |
|
I tried to pull the Buildkite log for 15313 to get you the exact failing test name, but the build page requires a login and I don't have Buildkite credentials here, so I hit the same wall you did. What I could check from the GitHub side: this PR's diff is still only |
|
Thanks for checking. You are right that it is not this PR: |
|
Correction to my last note: a rebuild at |
00590f2 to
176dc05
Compare
|
This is approved with green CI and no outstanding comments as far as I can tell — ready whenever you have a moment. Happy to rebase first if you'd like it freshened. |
Omni ReviewBot: supersededThe CI failure noted on |
176dc05 to
2c3de4c
Compare
|
@linyueqian the branch is rebased: head |
Omni ReviewBot routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
…-v3 e2e tests
Two speech tests in TestHiggsAudioV3OnlineInlineControlTokens flake against
the fixed 0.9 cosine-similarity transcript gate in
_assert_transcript_matches, for two different reasons:
- test_inline_sfx_with_onomatopoeia sends `<|sfx:laughter|>Hehe`, and the
model correctly vocalizes the onomatopoeia it was instructed to produce
("Hee hee, ..."), but transcript_expected_text omits it. A real CI run
scored 0.839 against the 0.9 gate for legitimate extra content, not a
wrong transcript.
- test_inline_style_whispering hits a real whisper-small ASR mishear
("It" heard as "This") because whispering softens the initial consonant.
A real CI run scored 0.896. Unlike test_plain_text_wav, which already
handles this identical failure mode via transcript_escalation_model,
this test had no fallback.
assert_audio_speech_response now reads an optional
transcript_similarity_threshold from request_config (default 0.9,
unchanged for every other test) instead of a hardcoded 0.9, following the
same per-test override pattern already used for min_hnr_db elsewhere in
this file. test_inline_sfx_with_onomatopoeia sets it to 0.75 (margin under
the observed 0.839). test_inline_style_whispering opts into the existing
transcript_escalation_model mechanism, unmodified, exactly as
test_plain_text_wav already does.
Validation: no GPU/TTS model is available in this environment, so the
live e2e test was not re-run. This is a test-assertion-only change (no
model/CUDA code touched), so it was validated directly against the real
assertion functions: the real cosine_similarity_text (tests/helpers/media.py)
reproduces both reported values (0.839, 0.896) exactly from the
transcript/expected-text strings quoted in the issue. The real
_assert_transcript_matches / assert_audio_speech_response fail with the
old hardcoded 0.9 threshold (reproducing the reported bug) and pass with
the new per-test override -- a fail-then-passing demonstration through the
actual production assertion path. ruff check, ruff format --check, and
the repo's full local pre-commit run (mypy-3.10, SPDX header check,
forbidden-imports check, torch.cuda check, CI-marks check, etc.) all pass
on the changed files. The transcript_escalation_model path for the
whispering test was not independently re-exercised end-to-end (needs a
real whisper-large-v3 model and real audio bytes, unavailable here); this
is reuse of an already-shipped, unmodified mechanism already exercised in
production by test_plain_text_wav, not new logic.
Report: vllm-project#7236
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
2c3de4c to
a3af613
Compare
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 8a416497-9bf2-42aa-8837-bcc995c2c58e) — check |
Purpose
Fixes the flaky
TestHiggsAudioV3OnlineInlineControlTokenstranscript assertions reportedin the weekly CI run linked from the issue.
Both failures share one root cause:
_assert_transcript_matches(tests/helpers/assertions.py)gates every speech test at a fixed 0.9 cosine-similarity threshold with no per-test way to
account for legitimate audio content the plain
transcript_expected_textstring doesn't cover.test_inline_sfx_with_onomatopoeiasends<|sfx:laughter|>Heheand the model correctlyvocalizes the onomatopoeia ("Hee hee, ...") it was instructed to produce, but
transcript_expected_textomits it (ASR renders laughter inconsistently across runs, sohardcoding the exact wording would just trade one flake for another). The real CI transcript
scored 0.839 against the 0.9 gate.
test_inline_style_whisperingtriggers a real whisper-small ASR mishear ("It" -> "This")because the whispering effect softens the initial consonant. The real CI transcript scored
0.896 against the same 0.9 gate. This test had no fallback, unlike
test_plain_text_wav,which already handles the identical whisper-small-mishear failure mode via
transcript_escalation_model.Approach
assert_audio_speech_responsenow reads an optionaltranscript_similarity_thresholdfromrequest_config(default0.9, unchanged for every other test) instead of a hardcoded0.9,mirroring the existing per-test override pattern already used for
min_hnr_dbelsewhere inthis file.
test_inline_sfx_with_onomatopoeiasetstranscript_similarity_threshold: 0.75, ~0.09 marginbelow the observed 0.839, with a docstring explaining why (legitimate extra onomatopoeia
content, not a real content mismatch). It still gates a genuinely wrong transcript.
test_inline_style_whisperingsetstranscript_escalation_model: "large-v3", reusing thealready-shipped escalation mechanism unchanged (re-verify with a stronger ASR before failing
on a fast whisper-small mishear), exactly as
test_plain_text_wavalready does.OnlineOmniClient.send_audio_speech_request's docstring documents the newtranscript_similarity_thresholdkey.Test Plan
vLLM Version: 0.28.0
vLLM-Omni Commit: b58ff5c
No GPU/TTS model is available in this environment, so the live e2e test itself was not re-run.
This is a test-assertion-only change (no model or CUDA code touched), so it was validated by
exercising the real assertion functions directly:
tests/helpers/media.py/tests/helpers/assertions.pyneed(numpy, soundfile, pillow, opencc-python-reimplemented, av) in a throwaway Python 3.11 venv —
no torch/vllm required for this code path.
cosine_similarity_text(tests/helpers/media.py): it reproduced both reported valuesexactly (0.839 for the sfx case, 0.896 for the whispering case).
_assert_transcript_matcheswiththreshold=0.9(old hardcoded value): itraised
AssertionError: Transcript doesn't match input, reproducing the CI failure. Called itagain with
threshold=0.75(new): it passed.assert_audio_speech_responseend-to-end (the function pytest actuallyinvokes) with a fake response object whose
.audio_contentwas set to the issue's exactreported transcript and the fixed test's real
request_config(includingtranscript_similarity_threshold: 0.75) atrun_level="advanced_model": passed. Removingthat key (reverting to the implicit 0.9 default) reproduced the exact same failure again.
transcript_escalation_modelpath for the whisperingtest end-to-end -- it needs a real whisper-large-v3 model and real audio bytes, unavailable
here. This is the same escalation code already shipped and exercised in production by
test_plain_text_wav; only the second test'srequest_configwas changed to opt into it,the escalation logic itself in
assertions.pyis untouched by this diff.ruff check,ruff format --check, and the repo's full localpre-commit runagainst all3 changed files: all hooks pass (mypy for Python 3.10, SPDX header check, forbidden-imports
check,
torch.cudaAPI check, CI-marks check, trailing whitespace, etc).upstream/mainand thatupstream/main'sown CI (pre-commit, Build Wheel, CodeQL) is green.
Test Result
cosine_similarity_textreproduces the issue's reported 0.839 and 0.896 exactly from thereal transcript/expected-text pairs.
_assert_transcript_matches/assert_audio_speech_responsefail-then-pass: raises with theold hardcoded 0.9 threshold (reproducing the reported bug), passes with the new per-test
override.
ruff check,ruff format --check, and full localpre-commit runon all changed files: allgreen.
pytest tests/e2e/online_serving/test_higgs_audio_v3.py -m tts)was not run in this environment (no GPU / TTS model available) -- flagging this limitation
explicitly per the above.
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)
Fixes #7236