Repository navigation
[bugfix] Fix CLI flags that modify model_config when using --deploy-config - #8289
Conversation
Signed-off-by: Maciej Bala <mbala@nvidia.com>
|
This PR appears to belong to: docs/design/module/vllm_omni_config.md, docs/design/module/entrypoints.md, docs/design/module/stage_runtime.md. Module owners: @alex-jw-brooks @lishunyang12 @NickCao Routing: @alex-jw-brooks via module of the changed files, semantic router, CODEOWNERS; @lishunyang12 via module of the changed files, semantic router, CODEOWNERS; @NickCao via module of the changed files, CODEOWNERS @MaciejBalaNV, 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. |
MaciejBalaNV
left a comment
MaciejBalaNV
left a comment
There was a problem hiding this comment.
This is a simple 2 lines change that targets how the config is merged.
| } | ||
| runtime_overrides = build_stage_runtime_overrides(stage_id, dict(cli_overrides)) | ||
| global_stage_fields = _global_stage_cli_fields() | ||
| global_stage_fields = _global_stage_cli_fields() | _OWNER_ONLY_GLOBAL_STAGE_CLI_FIELDS |
There was a problem hiding this comment.
The main point of the change.
Omni ReviewBot routing recordAssigned Strict 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
PR description
With --deploy-config, global CLI model_config overrides (including --no-guardrails → guardrails: false) were filtered out of per-stage CLI overlays because model_config was not treated as a global stage field. This change adds an owner-only allowlist so diffusion stages keep that override and deep-merge it into deploy model_config, while LLM stages still skip it. User-visible effect: --no-guardrails (and other global model_config CLI knobs) apply under --deploy-config instead of being silently ignored.
Change flow
flowchart TD
A["[EXISTING] CLI --no-guardrails / global model_config"]:::existing --> B["[CHANGED] _stage_cli_overrides global field filter"]:::changed
C["[NEW] _OWNER_ONLY_GLOBAL_STAGE_CLI_FIELDS"]:::new --> B
B --> D["[EXISTING] Diffusion stage deep-merge into deploy model_config"]:::existing
B --> E["[EXISTING] LLM stages skip unowned model_config"]:::existing
D --> F["[NEW] tests: global merge + LLM skip"]:::new
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
CI at
c22ca2f42f9c(2026-09-29T20:14:12.526897+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
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] Test Result omits covering L1 Simple · Other selector — Covering always-on L1 guard This finding appears in the bot's COMMENT review, but GitHub could not place it as an inline diff comment. If you are the PR author and disagree, react 👎 to this comment. The disagreement will be shown to the maintainer; it does not approve or merge the PR. |
hsliuustc0106
added
bug
hsliuustc0106
added
the
high priority
ChatGPT Codex Connector
|
@vllm-omni-review-bot Is there anything blocking from merging this simple fix? |
There was a problem hiding this comment.
Omni ReviewBot review
Correction: The two earlier P2 findings about generic model_config forwarding and hypothetical future ownership validation are withdrawn. Neither demonstrates a current defect at the reviewed head. One test-integrity finding remains.
Scan:
| Category | Result |
|---|---|
| Tests / verification | 1 finding |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
The production overlay is accepted: the owner-only allowlist forwards model_config to DIFFUSION stages and omits it from LLM_AR stages. Forwarding an optional value to a consumer that does not use it does not establish breakage. _validate_global_stage_cli_ownership checks _global_stage_cli_fields(), which does not include model_config at this head; the earlier validator finding depended on a hypothetical future change.
The remaining finding is the skipped merge test: test_global_model_config_override_deep_merges_structured_model_config selects cosmos3_nano_sim_bimanual, which is absent from OMNI_PIPELINES. resolve_pipeline_config returns None, and _resolve_pipeline_or_skip calls pytest.skip before the new merge assertions execute. Use a registered diffusion pipeline or the existing synthetic builder to exercise the override and preservation of nested values.
Verdict: REQUEST CHANGES
Original CI snapshot (unchanged by this correction):
CI at
fab4be3d5c40(2026-10-02T10:52:20.632483+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
🤖 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] test_global_model_config_override_deep_merges_structured_model_config (tests/co… — test_global_model_config_override_deep_merges_structured_model_config (tests/config/test_omni_config.py:182) never runs its merge assertions. It calls Evidence: Changed test at PR-time head: tests/config/test_omni_config.py:184-186 Suggestion: config = _build_single_diffusion_config( This finding appears in the bot's COMMENT review, but GitHub could not place it as an inline diff comment. If you are the PR author and disagree, react 👎 to this comment. The disagreement will be shown to the maintainer; it does not approve or merge the PR. |
Signed-off-by: Maciej Bala <mbala@nvidia.com>
Omni ReviewBot attempt recordReview attempt ended as failed (step 'fetch' (pr.fetch_diff): cannot pin PR #8289 to 3766676: git fetch failed: From https://github.com/vllm-project/vllm-omni
|
|
Fixed the comment from review above and verified the tests pass:
I've also verified the new tests by commenting out the
|
Omni ReviewBot attempt recordReview attempt ended as stale. |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
…onfig (vllm-project#8289) Signed-off-by: Maciej Bala <mbala@nvidia.com>
This PR fixes an issue with silently dropping CLI options that modify
model_configwhen using--deploy-configsetup. One example of such CLI option that was being dropped is--no-guardrails.Purpose
Before this PR when we launched e.g.
vllm serve nvidia/Cosmos3-Nano --omni --no-guardrails --deploy-config <custom_deploy_config>the--no-guardrailsflag would be ignored. This PR fixes this issue.Test Plan
Unit tests
We add 2 new unit tests targeting this change.
Serving tests
To test this PR in practice, first modify
vllm_omni/deploy/cosmos3_omni.yamldeploy config file so that it hasguardrails: true, or use any other config file that targets Cosmos3 model and doesn't already disable the guardrails. That's because we want to test whether the--no-guardrailsflag works and it wouldn't be possible if the deploy config file already disabled them.After the correct deploy config file is created, run this command:
vllm serve nvidia/Cosmos3-Nano --omni --deploy-config vllm_omni/deploy/cosmos3_omni.yaml --no-guardrails.Before this PR, you are going to see
[guardrails.py:148] Cosmos3 guardrails initialized.in logs, which means that--no-guardrailsflag is ignored and the guardrails are initialized. After this PR this is fixed and the guardrails are not initialized.vLLM Version:
0.30
vLLM-Omni Commit:
ToT
Test Result
Unit tests
All the unit tests from
tests/config/test_omni_config.pypassServing tests
The guardrails are indeed disabled with the setup mentioned above.
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.
(anything written below this line will be removed by GitHub Actions)