Skip to content

fix: guard omni-specific CLI args behind use_omni check - #8698

Merged
NickCao merged 2 commits into
vllm-project:mainfrom
khairulkabir1661:fix/strat-2749-guard-omni-cli-args
Oct 23, 2026
Merged

NickCao merged 2 commits into
vllm-project:mainfrom
khairulkabir1661:fix/strat-2749-guard-omni-cli-args

Conversation

@khairulkabir1661

Copy link
Copy Markdown
Contributor

Summary

Guard --stage-init-timeout, --init-timeout, and --log-stats behind the use_omni flag in the test helper iter_omni_server.

Previously these omni-specific CLI args were always appended to server_args, causing non-omni servers (e.g. Whisper with use_omni=False) to fail with unknown-argument errors.

Changes

  • tests/helpers/runtime.py: nest timeout and log-stats args inside if params.use_omni: block

@hsliuustc0106 hsliuustc0106 added core related to core module: cache, scheduler, engine, worker, modelrunner bug Something isn't working labels Oct 12, 2026
@khairulkabir1661
khairulkabir1661 marked this pull request as ready for review October 15, 2026 01:39
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-15T01:39:59.952770Z c5769a6 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@vllm-omni-review-bot

Copy link
Copy Markdown

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.

@NickCao
NickCao force-pushed the fix/strat-2749-guard-omni-cli-args branch from c5769a6 to 6f46599 Compare October 15, 2026 12:10
@NickCao NickCao added the ready label to trigger buildkite CI label Oct 15, 2026
@NickCao
NickCao enabled auto-merge (squash) October 15, 2026 12:12
@vllm-omni-review-bot

vllm-omni-review-bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Omni ReviewBot: superseded

The CI failure noted on 6f4659919145 refers to an earlier head; the pull request now points at d6985c03f24b.

@NickCao
NickCao force-pushed the fix/strat-2749-guard-omni-cli-args branch from 6f46599 to d6985c0 Compare October 15, 2026 14:53
@NickCao NickCao added ready label to trigger buildkite CI and removed ready label to trigger buildkite CI labels Oct 15, 2026
@vllm-omni-review-bot

vllm-omni-review-bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Omni ReviewBot: superseded

The CI failure noted on d6985c03f24b refers to an earlier head; the pull request now points at 27628a1b8a63.

auto-merge was automatically disabled October 19, 2026 18:38

Head branch was pushed to by a user without write access

@khairulkabir1661
khairulkabir1661 force-pushed the fix/strat-2749-guard-omni-cli-args branch from d6985c0 to 27628a1 Compare October 19, 2026 18:38

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Add a focused CPU regression for the argument assembly in iter_omni_server, and record the command/results on baseline da572cddb38383deae9caef33b8d490e292ef071 and this head. With use_omni=False, capture the arguments passed to the server and assert none of the three Omni-only flags is auto-added; with use_omni=True, retain default/custom timeouts and the explicit --disable-log-stats behavior. The non-Omni assertion should fail before this change and pass after it. This is the missing evidence for the unknown-argument fix, not a new code defect; the existing stage-CLI fixture test only asserts deploy-config handling. Author: khairulkabir1661. I inspected the source but did not execute these tests.

@khairulkabir1661

Copy link
Copy Markdown
Contributor Author

CPU regression test results

Baseline da572cddb — 1 FAILED

tests/helpers/tests/test_runtime_omni_args.py::test_non_omni_skips_omni_flags FAILED
tests/helpers/tests/test_runtime_omni_args.py::test_omni_defaults PASSED
tests/helpers/tests/test_runtime_omni_args.py::test_omni_custom_timeouts PASSED
tests/helpers/tests/test_runtime_omni_args.py::test_omni_disable_log_stats_wins PASSED

FAILED test_non_omni_skips_omni_flags - AssertionError:
  Omni-only flags leaked into non-Omni args: {'--log-stats', '--init-timeout', '--stage-init-timeout'}

1 failed, 3 passed

Head 883c2984c — 4 passed

tests/helpers/tests/test_runtime_omni_args.py::test_non_omni_skips_omni_flags PASSED
tests/helpers/tests/test_runtime_omni_args.py::test_omni_defaults PASSED
tests/helpers/tests/test_runtime_omni_args.py::test_omni_custom_timeouts PASSED
tests/helpers/tests/test_runtime_omni_args.py::test_omni_disable_log_stats_wins PASSED

4 passed

Command

python -m pytest tests/helpers/tests/test_runtime_omni_args.py -v

Signed-off-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
Signed-off-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
@NickCao
NickCao force-pushed the fix/strat-2749-guard-omni-cli-args branch from 883c298 to bcef9d6 Compare October 23, 2026 13:02
@NickCao NickCao added ready label to trigger buildkite CI and removed ready label to trigger buildkite CI labels Oct 23, 2026
@NickCao
NickCao enabled auto-merge (squash) October 23, 2026 13:02
@NickCao
NickCao merged commit cbe4cc4 into vllm-project:main Oct 23, 2026
8 of 11 checks passed
tzhouam pushed a commit to tzhouam/vllm-omni that referenced this pull request Oct 25, 2026
…#8698)

Signed-off-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
Co-authored-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
(cherry picked from commit cbe4cc4)
Signed-off-by: tzhouam <tzhouam@connect.ust.hk>
hsliuustc0106 pushed a commit that referenced this pull request Oct 25, 2026
 #9339 (#9352)

Signed-off-by: Chendi Xue <chendi.xue@intel.com>
Signed-off-by: tzhouam <tzhouam@connect.ust.hk>
Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
Signed-off-by: Nick Cao <ncao@redhat.com>
Signed-off-by: Cao Xuanning <2107254305@qq.com>
Signed-off-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
Signed-off-by: lm10-arg <347490427+lm10-arg@users.noreply.github.com>
Co-authored-by: Chendi.Xue <chendi.xue@intel.com>
Co-authored-by: amy-why-3459 <wuhaiyan17@huawei.com>
Co-authored-by: Nick Cao <ncao@redhat.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Cao Xuanning <109304380+advanced233@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Khairul Kabir <250204754+khairulkabir1661@users.noreply.github.com>
Co-authored-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com>
Co-authored-by: lm10-arg <linluo@icloud.com>
Co-authored-by: lm10-arg <347490427+lm10-arg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working core related to core module: cache, scheduler, engine, worker, modelrunner ready label to trigger buildkite CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants