[Config Refactor] Remove legacy Omni CLI arg helper and align tests with nullified parser defaults#3144
Conversation
hsliuustc0106
left a comment
There was a problem hiding this comment.
Clean removal of the legacy OmniBase.from_cli_args() + detect_explicit_cli_keys() path, superseded by the sentinel-default refactor (RFC #3035). All internal callers migrated to Omni(**vars(args)) with pre-nullified parsers. The vllm serve path was already using direct AsyncOmni(model=..., **kwargs) and is unaffected. OmniEngineArgs.from_cli_args (a separate, unrelated class method) is untouched. Tests updated and passing.
|
This PR's design was dissussed offline yesterday. |
|
@xiaohajiayou. This pr will be integrated after #2989. Please make sure sentinel-default refactor covers all existing refractored models, and has been/would cover rest of models. We should document our new design as model developers might find it useful for integration. |
lishunyang12
left a comment
There was a problem hiding this comment.
Per the earlier comment, this depends on #2989 landing first — worth a TODO in the PR description so reviewers don't merge prematurely. Also: examples/offline_inference/bagel/end2end.py originally used from_cli_args and is updated here, but please sweep the rest of the ~14 examples for the same pattern before this lands.
|
Good point. I restored What changed in this update:
|
hsliuustc0106
left a comment
There was a problem hiding this comment.
Cross-PR Config Refactor Review
This is one of several coordinated config refactor PRs. Reviewed together with #3162, #3160, #3154, #3128, #3120, #3139.
What this PR does
- Converts 15 offline example scripts from Omni.from_cli_args(args) to Omni(**vars(args)) with nullify_stage_engine_defaults(parser) called explicitly in parse_args().
- Adds DeprecationWarning to from_cli_args().
- Updates tests to cover both the new direct path and the deprecated path.
Assessment
Good cleanup. The deprecation is properly gated with a DeprecationWarning.
One issue: merge order dependency with #3160
This PR adds imports of nullify_stage_engine_defaults from vllm_omni.engine.arg_utils in every offline example, but #3160 removes that function from arg_utils.py entirely. This PR must merge before #3160, or the offline scripts will break.
Recommendation: merge this PR before #3160, and have #3160 either keep a backward-compatible shim or remove the function only from production code while leaving it for offline examples until a follow-up cleans them up.
Signed-off-by: xiaohajiayou <923390377@qq.com>
Signed-off-by: xiaohajiayou <923390377@qq.com>
Signed-off-by: xiaohajiayou <923390377@qq.com>
Signed-off-by: xiaohajiayou <923390377@qq.com>
Signed-off-by: xiaohajiayou <923390377@qq.com>
Signed-off-by: xiaohajiayou <923390377@qq.com>
Signed-off-by: xiaohajiayou <923390377@qq.com>
Signed-off-by: xiaohajiayou <923390377@qq.com>
Replace Omni.from_cli_args(args, ...) with Omni(**vars(args)) in ming_flash_omni and ming_flash_omni_tts examples. Signed-off-by: xiaohajiayou <923390377@qq.com>
|
This PR deprecates What changed1. Restored with 2. All examples migrated to direct construction
Pure diffusion examples ( 3.
4. Tests Deprecated
|
Signed-off-by: xiaohajiayou <923390377@qq.com>
|
This PR is ready for review. |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review Summary
Validated: CI all green. The from_cli_args deprecation shim is correctly gated with DeprecationWarning. All 15 migrated examples already had nullify_stage_engine_defaults(parser) in their parse_args() — the mechanical switch from Omni.from_cli_args(args, ...) to Omni(**vars(args)) is consistent across them. Tests cover both the deprecated path (with warning assertion) and the new direct path.
Two items to address:
-
Dead code not removed.
detect_explicit_cli_keysstill lives invllm_omni/entrypoints/utils.py:43but its last caller (test_serve.py) is deleted by this PR. The PR description says "Remove the unuseddetect_explicit_cli_keys()helper from utils.py" — bututils.pyisn't in the diff. After this lands, the function is dead code. -
>10 files — recommend L3 tests. Per the test guide, 18 files changed exceeds the threshold for requesting L3 validation. Could you run the L3 integration tests locally and paste results here?
Not blocking. The approach has sign-off from prior maintainer reviews.
Reviewed by Claude Code with deepseek-v4-pro
I think this helper is still used on the deprecated compatibility path. We intentionally did not hard-remove
Given that, neither |
…ith nullified parser defaults (vllm-project#3144) Signed-off-by: xiaohajiayou <923390377@qq.com>
Purpose
Remove the legacy
OmniBase.from_cli_args()path now that CLI precedence is handled by nullified parser defaults.Before this change, parser-driven callers could rely on:
Omni.from_cli_args(...)detect_explicit_cli_keys(...)to distinguish user-typed values from argparse defaults.
After the sentinel-default refactor, that compatibility path is no longer needed:
NoneTrue/FalseOmni(**vars(args))is the intended construction pathThis PR removes the old helper path and updates entrypoint tests to cover the new mechanism directly.
Test Plan
OmniBase.from_cli_args()from omni_base.py.detect_explicit_cli_keys()helper from utils.py.nullify_stage_engine_defaults(parser)+Omni(**vars(args))Test Result
Executed in the project venv:
Result:
37 passedThe new replacement test verifies:
NoneOmni(**vars(args))works as intended with the new parser mechanismTODO
This pr will be integrated after #2989. Make sure sentinel-default refactor covers all existing refractored models
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model. Please runmkdocs serveto sync the documentation editions to./docs.BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)