Repository navigation
Conversation
b7ab629 to
860febd
Compare
|
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 @khairulkabir1661, 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 routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
2 actionable finding(s).
CI at
860febdb2cfa(2026-10-10T12:31:14.217561+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 2 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [resolved] duplex session.close→session.closed already required in _duplex_recorded_session :952-955 and CPU test_duplex_serving.py:452-455; residual is the new turn-class test, not missing duplex coverage
- [resolved] empty-commit contract exists on this path (connection.py:366-368); residual: substring 'empty' also matches unrelated text; duplex runner uses input_audio_buffer_empty (runner.py:1968) and is not this fixture.
- [claim-verified] four tests added on TestQwen3OmniRealtimeWebSocket: lifecycle, empty_commit, graceful_session_close, abnormal_disconnect (test_qwen3_omni_realtime_websocket.py:702-778)
- [claim-verified] test_empty_commit_returns_error: _handle_audio_commit :366-371 sends error 'Input audio buffer is empty'; helper only raises error.type==server_error (490) so invalid_request_error is collected; errors[0] after assert errors (734-735)
- [claim-verified] class-scoped omni_server + realtime_async_chunk_server_params reuses the async_chunk server with existing merge tests
- [claim-verified] abrupt reconnect can open session.created on turn path (_cleanup :137-140); this is connection-scoped, not an admission slot
Keep two majors on the PR’s new lifecycle tests: test_graceful_session_close cannot fail (timeout swallowed, collected never passed, turn OpenAIFullDuplexConnection has no session.close/session.closed) and test_abnormal_disconnect_no_leak cannot detect a leak (no duplex admission slot; ws.close()+sleep(2) then session.created is the wrong contract vs the in-file duplex sibling). Delete both from TestQwen3OmniRealtimeWebSocket or move them to TestQwen3OmniDuplexPlayback; leave the valid lifecycle and empty-commit cases. All other candidates are no_issue, excluded polish, or duplicates of those two.
Verdict: REQUEST CHANGES
See inline comments below.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
Omni ReviewBot: finding feedback**[p1] This diff adds See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: finding feedback**[p1] This diff adds See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: supersededThe CI failure noted on |
Omni ReviewBot: no human activity for 23 days@khairulkabir1661 this pull request has had no human commit, comment or review since 2026-09-28. 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. |
Add protocol conformance tests to test_qwen3_omni_realtime_websocket.py to reuse the same server instance. Covers session lifecycle event ordering, empty-commit error handling, graceful session close, and session cleanup after abnormal disconnect. Signed-off-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
Remove test_graceful_session_close and test_abnormal_disconnect_no_leak: - test_graceful_session_close: doesn't pass collected=events to recv_until, timeout always occurs, test passes even when broken. session.closed handling already covered by duplex tests. - test_abnormal_disconnect_no_leak: ws.close() is normal close not abrupt disconnect, turn mode has no session slot limit so cannot test leaks. Keep test_session_lifecycle_events and test_empty_commit_returns_error which properly test turn-mode behavior. Signed-off-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
- Remove dangling decorators that were attached to deleted test methods - Remove unused ConnectionClosed import Signed-off-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
b4859ba to
faa632c
Compare
|
Tested |
Summary
Wire-protocol conformance tests added to
test_qwen3_omni_realtime_websocket.py, reusing the same server instance. Covers event sequencing and error responses.New tests (2) in
TestQwen3OmniRealtimeWebSocketsession.createdprecedessession.updated, both exactly once, session object includes idinput_audio_buffer.commitwithout prior audio returns error mentioning "empty"Session lifecycle and disconnect handling are already covered by
tests/entrypoints/duplex/test_duplex_serving.pyand the duplex mode tests in the same file.Run
```bash
pytest -v tests/entrypoints/openai_api/test_qwen3_omni_realtime_websocket.py::TestQwen3OmniRealtimeWebSocket -k "lifecycle or empty_commit" --run-level=advanced_model
```