Skip to content

[Feat] Override single stage CLI args when stage_configs_path is set in OmniEngineArgs#2684

Merged
princepride merged 4 commits into
vllm-project:mainfrom
timzsu:feat/engine-args-stage-configs-path
Apr 13, 2026
Merged

[Feat] Override single stage CLI args when stage_configs_path is set in OmniEngineArgs#2684
princepride merged 4 commits into
vllm-project:mainfrom
timzsu:feat/engine-args-stage-configs-path

Conversation

@timzsu
Copy link
Copy Markdown
Contributor

@timzsu timzsu commented Apr 10, 2026 •
edited
Loading

Companion to verl-project/verl#5947.

Purpose

Add custom_pipeline_args field to OmniEngineArgs and strip single-engine args when stage_configs_path is set, so verl can use a single unified code path for both single-stage and multi-stage engine initialization:

engine_args = asdict(OmniEngineArgs.from_cli_args(args))
engine_client = AsyncOmni(**engine_args)

Key changes:

  • Add custom_pipeline_args field to OmniEngineArgs (passed through to diffusion stage engine).
  • Guard create_model_config() when stage_configs_path is set (per-stage configs resolve their own).
  • Add _strip_single_engine_args() to AsyncOmniEngine: when stage_configs_path is set, strip parent EngineArgs fields and stage_configs_path from kwargs before the per-stage merge. custom_pipeline_args and worker_extension_cls are kept (consumed by _create_default_diffusion_stage_cfg).
  • Warn when explicitly-set top-level engine args are ignored.

Test Plan

python -m pytest tests/engine/test_arg_utils.py -v
  • test_stage_configs_path_field — construction with stage_configs_path
  • test_stage_configs_path_blocks_create_model_config — guard on create_model_config()
  • test_from_cli_args_picks_up_stage_configs_path — CLI namespace propagation
  • test_strip_single_engine_args — stripping, allowlist, and passthrough

Test Result

tests/engine/test_arg_utils.py  12 passed
Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan. Please provide the test scripts & test commands.
  • The test results.
  • (Optional) The necessary documentation update.
  • (Optional) Release notes update.

@timzsu timzsu requested a review from hsliuustc0106 as a code owner April 10, 2026 10:53
@chatgpt-codex-connector
Copy link
Copy Markdown

chatgpt-codex-connector Bot commented Apr 10, 2026

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.

@timzsu timzsu changed the title [Feat] Add stage_configs_path support to OmniEngineArgs [WIP][Feat] Add stage_configs_path support to OmniEngineArgs Apr 10, 2026
@timzsu timzsu mentioned this pull request Apr 10, 2026
Open
8 tasks
@timzsu
Copy link
Copy Markdown
Contributor Author

timzsu commented Apr 10, 2026

@princepride @SamitHuang This PR is related to verl-project/verl#5947, so it might be good to review both together.

timzsu and others added 2 commits April 10, 2026 22:01
@timzsu
Support stage config in engine args.
bc03c7f 
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
@timzsu @claude
Strip single-engine args when stage_configs_path is set
bc84e12 
When stage_configs_path is provided, per-stage engine args come from the
YAML. Top-level EngineArgs fields (compilation_config,
tensor_parallel_size, etc.) must not leak into per-stage configs via the
base_engine_args merge in load_stage_configs_from_yaml — they cause type
errors (e.g. compilation_config as a JSON string rejected by VllmConfig)
or silently override YAML values.

Add _strip_single_engine_args() to AsyncOmniEngine which removes parent
EngineArgs fields from kwargs before the merge, keeping
worker_extension_cls (needed for colocate worker setup). Warns when
explicitly-set overrides are ignored.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
@timzsu timzsu force-pushed the feat/engine-args-stage-configs-path branch from a618694 to be3e750 Compare April 10, 2026 15:03
@timzsu @claude
Fix stage_configs_path leaking into per-stage engine args
67a370b 
stage_configs_path is an OmniEngineArgs field (not parent EngineArgs),
so _strip_single_engine_args did not remove it. It leaked through
load_stage_configs_from_yaml's merge into per-stage engine_args, causing
per-stage create_model_config() to hit the stage_configs_path guard.

Strip stage_configs_path explicitly as an orchestrator-level field.
custom_pipeline_args is intentionally kept — it is consumed by
_create_default_diffusion_stage_cfg, not by per-stage OmniEngineArgs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
@timzsu timzsu force-pushed the feat/engine-args-stage-configs-path branch from be3e750 to 67a370b Compare April 10, 2026 15:05
@timzsu timzsu changed the title [WIP][Feat] Add stage_configs_path support to OmniEngineArgs [Feat] Add stage_configs_path support to OmniEngineArgs Apr 11, 2026
@timzsu timzsu changed the title [Feat] Add stage_configs_path support to OmniEngineArgs [Feat] Override single stage CLI args when stage_configs_path is set in OmniEngineArgs Apr 11, 2026
@princepride
Copy link
Copy Markdown
Collaborator

princepride commented Apr 11, 2026

@lishunyang12 PTAL

lishunyang12
lishunyang12 reviewed Apr 11, 2026
View reviewed changes
Comment thread vllm_omni/engine/async_omni_engine.py
if v != default:
overridden.append(k)

if overridden:
Copy link
Copy Markdown
Collaborator

@lishunyang12 lishunyang12 Apr 11, 2026

Choose a reason for hiding this comment

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

model is always in kwargs (callers set it via from_cli_args/asdict), so overridden will always contain it — this warning will always lead with model, .... Worth filtering it out so the list only surfaces the actually-surprising overrides.

Copy link
Copy Markdown
Contributor Author

@timzsu timzsu Apr 11, 2026

Choose a reason for hiding this comment

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

Thanks for pointing this out. Fixed in the latest commit.

Copy link
Copy Markdown
Collaborator

@lishunyang12 lishunyang12 Apr 12, 2026

Choose a reason for hiding this comment

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

Looks good, thanks.

@timzsu
Exclude model from override warning in _strip_single_engine_args
ec1a089 
model is always set by callers (via from_cli_args/asdict) and always
differs from its default, so including it in the "ignored overrides"
warning adds noise without surfacing genuinely surprising overrides.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
@timzsu timzsu requested a review from lishunyang12 April 11, 2026 06:08
lishunyang12
lishunyang12 approved these changes Apr 12, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@lishunyang12 lishunyang12 left a comment

Choose a reason for hiding this comment

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

LGTM

@hsliuustc0106 hsliuustc0106 added the ready label to trigger buildkite CI label Apr 12, 2026
hsliuustc0106
hsliuustc0106 reviewed Apr 12, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@hsliuustc0106 hsliuustc0106 left a comment

Choose a reason for hiding this comment

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

LGTM, no blockers. One minor observation:

_strip_single_engine_args (lines 1255-1266): when v is None the method skips with continue. If an EngineArgs field has a default_factory that returns None (some optional fields), the warning would be suppressed. This is a minor edge case and the current behavior is reasonable.

Tests cover the key paths well.

@princepride princepride merged commit eb1a801 into vllm-project:main Apr 13, 2026
8 checks passed
daixinning pushed a commit to daixinning/vllm-omni that referenced this pull request Apr 13, 2026
@timzsu @claude
[Feat] Override single stage CLI args when stage_configs_path is set …
a07c2fb 
…in OmniEngineArgs (vllm-project#2684)

Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@amy-why-3459
Copy link
Copy Markdown
Contributor

amy-why-3459 commented Apr 14, 2026

This PR caused Qwen3-Omni's nightly-test to fail. I will remove the stage_configs_path check; please check if it has any impact. Please contact me when you have time. #2741

lengrongfu pushed a commit to lengrongfu/vllm-omni that referenced this pull request May 1, 2026
@timzsu @claude @lengrongfu
[Feat] Override single stage CLI args when stage_configs_path is set …
7e354c1 
…in OmniEngineArgs (vllm-project#2684)

Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
clodaghwalsh17 pushed a commit to clodaghwalsh17/nm-vllm-omni-ent that referenced this pull request May 12, 2026
@timzsu @claude @clodaghwalsh17
[Feat] Override single stage CLI args when stage_configs_path is set …
474f640 
…in OmniEngineArgs (vllm-project#2684)

Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hsliuustc0106 hsliuustc0106 hsliuustc0106 left review comments

@lishunyang12 lishunyang12 lishunyang12 approved these changes

Assignees

No one assigned

Labels

ready label to trigger buildkite CI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@timzsu @princepride @amy-why-3459 @lishunyang12 @hsliuustc0106