Repository navigation
[Core] Retain structured configs through runtime startup - #6849
Conversation
|
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 appears to belong to: docs/design/module/stage_runtime.md. Module owners: @tzhouam @fake0fan @maithilijoshi20, 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. |
bfb6c5e to
80fb909
Compare
80fb909 to
7b8c1d3
Compare
87db810 to
104bcf6
Compare
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
|
I think we need to add sufficient end-to-end tests to demonstrate that the configuration replacement works correctly. |
2d03cde to
65a9230
Compare
linyueqian
left a comment
There was a problem hiding this comment.
Reviewed at 25f68031 after the rebase across #7413. The rebase itself is clean: vllm_omni/entrypoints/duplex/, vllm_omni/engine/duplex/, duplex_omni_engine.py and duplex_orchestrator.py are byte-identical to main, and the only omni_engine_base.py delta is the async_chunk read moving onto connector_config. The typed diffusion admission budget and the Higgs model resolution I asked about last round both check out: stage_admission.py reads cache_config.gpu_memory_utilization first and typed construction populates it, and both Higgs adapters resolve through resolve_stage_model_path.
The change I am asking for comes from the one structural move this PR makes: resolver.py stops projecting stages through stage.to_omegaconf() and hands the typed OmniStageConfig objects straight to runtime startup and to the frontend. That is the right direction, but three readers downstream still assume the OmegaConf shape and now degrade silently instead of failing. They are inline as [important]: per-stage runtime.env is dropped because _to_dict cannot convert a pydantic dataclass, nested --stage-overrides mappings now replace whole sections where the OmegaConf bridge deep-merged them, and the two video capability checks in serving_video.py still look for engine_args.model_class_name, the same reader this PR already fixed in video/generation/helpers.py. One [suggestion] on resolve_stage_model_path precedence. Once those three are covered (each is a small change plus a regression test with a typed stage) I expect to approve.
Validation: static read of the worktree against origin/main and a two-model panel; no PR code executed. GitHub Actions pre-commit, wheel build and DCO are green; the general Buildkite lane was re-fired at this head and is still running.
linyueqian
left a comment
There was a problem hiding this comment.
Inline items for the request-changes review above (pullrequestreview-5226741717); they went out without their anchors, so here they are on the lines they refer to.
| """Prefer typed stage model overrides, then legacy and served-model values.""" | ||
| stages = getattr(engine_client, "stage_configs", ()) or () | ||
| for stage in stages: | ||
| model_path = getattr(getattr(stage, "model_config", None), "model", None) |
There was a problem hiding this comment.
[suggestion] Returning the first stage that has model_config.model is fine for the shipped Higgs profiles, where no stage overrides the served model, but typed construction fills every stage's model_config.model with the served model when nothing is set, so a deployment that overrides only stage 1 would resolve stage 0's inherited value here while the legacy loop below would have returned the stage 1 override. If a later-stage override is meant to win, iterate stages in reverse or skip values equal to the served model; if not, a one-line docstring note that stage 0 wins would stop the next reader from chasing it.
Resolve the integration with current main while preserving typed config ownership through runtime startup. Signed-off-by: maithilijoshi20 <97733343+maithilijoshi20@users.noreply.github.com>
Signed-off-by: maithilijoshi20 <97733343+maithilijoshi20@users.noreply.github.com>
6468054 to
9a2a193
Compare
Signed-off-by: maithilijoshi20 <97733343+maithilijoshi20@users.noreply.github.com>
linyueqian
left a comment
There was a problem hiding this comment.
Approving at 211d7e06, which is 9a2a1935 plus a test-only follow-up (the nested-override test now reads the Cosmos3 policy stage's diffusion_config.model_config, and the ComfyUI e2e test patches _should_serve_duplex after #7675 renamed it); the source hunks are unchanged. All three items from my request-changes round are fixed at the source, each with a test that would have caught the original: _to_dict in entrypoints/stage_utils.py now converts dataclass instances with asdict, so a typed OmniStageRuntimeConfig keeps its env through stage_runtime_env (test_stage_runtime_env_accepts_typed_runtime_config sets a variable through a typed config and reads it back inside the context); _stage_engine_values deep-merges mapping-valued CLI overrides through the same _get_recursively_merged_dict the deploy overlay uses, with omni_kv_config deliberately kept atomic, so a partial --stage-overrides no longer deletes sibling keys (test_nested_stage_override_deep_merges_structured_model_config checks that guardrails flips while policy_server_config survives, which is the Cosmos3 case I described); and the two ServingVideo capability checks now resolve the class name through one shared _stage_diffusion_model_class_name helper that video/generation/helpers.py also uses, trying diffusion_config.model_class_name, then model_config.model_arch, then the legacy engine_args, with test_typed_stage_drives_video_capability_checks feeding typed stages to both checks.
The rebase onto current main also picked up #7544 correctly: the engine's async_chunk now reads the typed connector_config first and falls back to legacy engine_args, so the any-stage rule from that PR holds for both shapes. The Higgs precedence note from last round stays a suggestion and does not affect the shipped profiles.
Validation: static comparison of the PR patch at 25f68031 against the patch at this head plus a read of the three new tests; no PR code executed. The branch is zero commits behind main with no conflicts. I re-fired the general lane at this head after the push and will merge on green.
…t#6849) Signed-off-by: maithilijoshi20 <97733343+maithilijoshi20@users.noreply.github.com> Signed-off-by: Matthieu Laneuville <matthieu.laneuville@surf.nl>
…t#6849) Signed-off-by: maithilijoshi20 <97733343+maithilijoshi20@users.noreply.github.com>
Purpose
Complete the structured-config runtime-startup slice of RFC #6500.
BaseVllmOmniStageConfiginstances through resolution, runtime planning, standard startup, and headless startup.VllmOmniConfig; the typed path no longer converts to or passes through an OmegaConf runtime stage.Test Coverage
Runtime-startup boundaries
VllmOmniDiffusionStageConfigwith typed sampling, parallel, runtime, and model owners.StageRuntimeandlaunch_diffusion_stage_replica, stubbing only the model/process-client boundary.run_headless,launch_headless_diffusion_replicas, metadata extraction, config building, replica-group planning, and replica launch.E2E configuration effects
These GPU tests inspect the live
engine.stage_configsafter startup. They verify that deploy settings have an observable runtime owner and cannot be silently ignored while the stage still starts.trust_remote_code; default sampling parameters; connector edges; CUDAenforce_eageroverrides.Validation
git diff --checkpass.The Qwen3-Omni, WAN2.2, and HunyuanImage-3 tests are hardware-marked. Qwen3-Omni requires 2 GPUs, WAN2.2 requires 1 GPU, and HunyuanImage-3 requires 8 H100 GPUs; CI provides execution for these hardware-specific tests.
vLLM Version:
2ae2bf5da39bc5069edc6f57270b307613c016c8vLLM-Omni Commit:
64a03ab6b