Repository navigation
[Bugfix] Preserve generic diffusion deploy config with FLUX runtime coverage - #9051
shunwendongan wants to merge 3 commits into
Conversation
…overage Preserve deploy YAML inputs through the generic typed diffusion configuration merge. Add native tiny FLUX startup/request acceptance for mp and uni, worker-side backend execution probes, and configuration/helper regressions. Keep the existing component smoke offline with a local model path. Signed-off-by: wu kui <wukui1030660585@163.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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/vllm_omni_config.md, docs/design/module/stage_runtime.md, docs/design/module/entrypoints.md, docs/design/module/diffusion/index.md. Module owners: @alex-jw-brooks @lishunyang12 @david6666666 Routing: @alex-jw-brooks via module of the changed files, module named in the PR description, CODEOWNERS; @lishunyang12 via module of the changed files, module named in the PR description, CODEOWNERS; @david6666666 via module named in the PR description @shunwendongan, 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. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
BLANKETusers
left a comment
There was a problem hiding this comment.
Supersedes my earlier review on this PR, which was posted in Chinese. GitHub does not allow editing or deleting a submitted review body, so the findings are re-stated here in English with P0/P1/P2 severities. The findings are unchanged.
Independent review (frozen head 4e067548b980)
Scope: vllm_omni/config/config_factory.py, vllm_omni/config/resolver.py, plus the added/modified tests and CI wiring. Snapshot: head 4e067548b9805b2f1849c63b308a8cdd0da3d8d7, mergeable=CONFLICTING, all checks green (build 3.11/3.12, pre-commit, CodeQL, DCO, Analyze, readthedocs).
P0 — Validation evidence is not bound to the current head, and the config ownership audit was not redone after the rebase
- Location: PR description "Test Plan" /
vllm_omni/config/config_factory.py:793-912 - Trigger path: The description states
Head: a7821c4cf6d36958dbd2525e3c8f49dc0da78c73andbase: 8da036a81e5a5680e6ff0b77cb759e45da9b4194, and claims "all 13 outgoing file hashes were verified unchanged before committing". The actual PR head is now4e067548b9805b2f1849c63b308a8cdd0da3d8d7and contains twoMerge branch 'main'commits; GitHub reportsmergeable=CONFLICTING. - Current behavior: The runtime acceptance runs in the Test Plan (
tests/model_tests/diffusion/test_backend_runtime.pyFLUXmp/uni,tests/config/test_backend_selection.py,tests/diffusion/models/flux/test_backend_selection.py) were all produced from the pre-rebase working tree, not from the reviewed revision. - Impact: This PR changes exactly the deploy → typed projection field ownership and precedence contract. If either upstream main merge added dataclass fields, default factory parameters, or pipeline-wide fields, the auto-merge result must be treated as a new semantic diff and the field sets plus final per-stage values re-compared — otherwise the conclusion "the deploy YAML is no longer dropped" may hold only on the old base.
- Minimal fix: Update the description's head/base to the frozen revision, re-run the Test Plan's three commands on the current base and report the selector used with the output, and re-walk the config ownership audit for
_create_typed_diffusion_from_deployafter the rebase.
P1 — The deploy dtype materialization duplicates an existing fallback and pushes dtype into the highest-precedence opaque engine_extras layer
- Location:
vllm_omni/config/config_factory.py:811-812if deploy.dtype is not None: stage_deploy.engine_extras.setdefault("dtype", deploy.dtype)
- Trigger path:
resolve_omni_config→_create_typed_diffusion_from_deploywrites the YAML top-leveldtypeinto stage 0'sengine_extrasbeforeVllmOmniConfig.from_pipeline_configruns_apply_platform_overrides. - Current behavior: In the legacy builder,
engine_extrasis merged last and therefore wins over everything else (stage_config.py:1258-1269: pipeline-wide fields → per-stage fields →engine_args.update(ds.engine_extras)). The typed projection already has an equivalent fallback (omni_config.py:2273-2274:if "dtype" not in diffusion_kwargs and deploy.dtype is not None), so these lines are redundant for the final value. The comment's claim that "platform and explicit caller inputs still overlay it normally" holds only because_apply_platform_overrideshappens to write the sameengine_extras["dtype"]key (stage_config.py:1138-1139) — not because the layer precedence actually works that way. - Impact: A pipeline-wide field is promoted to a stage-owned opaque extra that outranks every other deploy layer. If a platform overlay ever expresses the same setting through a first-class field, or a platform stage carries no explicit
dtype, the materialized value silently defeats the platform value — while the comment implies it cannot. - Minimal fix: Drop these two lines and rely on the existing
omni_config.py:2273-2274fallback; if the materialization must stay, materialize after the platform overlay is merged, or have the platform branch overwrite the same key explicitly with an assertion.
P2 — Probe teardown lacks the hasattr guard, so the test's finally assertion masks the real failure
- Location:
tests/helpers/diffusion_backend_probe.py:194-205 - Trigger path:
DiffusionBackendProbe.stop_backend_runtime_probe()iteratesself._probe_linear_originalswithout a guard, whileshutdown()in the same file (lines 207-211) guards the identical call withhasattr. - Current behavior: If
load_modelfails early (probe attributes never installed),test_backend_runtime.py:146-148runsfinally: assert await engine.collective_rpc("stop_backend_runtime_probe", ...), which raisesAttributeErrorand replaces the real model-loading exception. - Impact: A failure in this test surfaces a misleading stack trace in CI, increasing triage cost — and it is the exact anti-pattern this repo's review guidance forbids (assertions must bind to real behavior).
- Minimal fix: Add the same
hasattrguard used byshutdown()insidestop_backend_runtime_probe()(or make thefinallyblock clean up only, without asserting).
Verified as sound
- Regression coverage:
tests/config/test_backend_selection.py:124-163parametrizes over thedeploy-yamlcase (no CLI kwargs at all) and asserts each of the four previously-wrong values (linear_backend, attention,enforce_eager,distributed_executor_backend), keeping anexplicit-kwargs-controlcomparison. This satisfies the requirement that a bugfix pins the original failure path. - CI wiring is complete: the new runtime test carries
core_model/diffusion/cuda/L4/cards_1marks and is selected by-m 'core_model and cuda'at.buildkite/cuda/test-ready.yml:97; the new config regressions carrycore_model+cpuand are selected by thetests/step at.buildkite/cuda/test-ready.yml:34;diffusion_tiny_model's source-file dependencies covertests/model_tests/diffusion/. No guard step is declared but never run. - The probe observes rather than substitutes: it wraps and still delegates to the original
_gemm_impl/ attentionforward, is revertible, and its assertions bind to real call counts plus transformer output shape and finiteness. - No duplicate findings: this PR has no inline review comments, and the Codex review ended
failedon usage limits without producing findings.
[Bugfix] Preserve generic diffusion deploy config with FLUX runtime coverage
Follow-up to #8092. Head:
a7821c4cf6d36958dbd2525e3c8f49dc0da78c73; base:8da036a81e5a5680e6ff0b77cb759e45da9b4194.Contributor intent (required human answers)
The following public-safe summaries reflect the human contributor's explicitly confirmed intent for this follow-up. The public technical reference is the maintainer's acceptance-test recommendation on #8092.
Scope against the #8092 follow-up recommendation
This PR implements the diffusion startup/request part of the recommendation and the associated configuration regressions. The AR MoE deploy/prefill/decode run and the diffusion-MoE real-forward run remain separate follow-up work: tiny FLUX is dense, so this PR does not claim MoE coverage.
The configuration portions are covered by regression tests for resolved diffusion attention selection, backend-specific CLI-over-YAML precedence, different backend values per stage, and equivalent canonical spellings such as
TORCH/torchwithout a falseengine_extrasconflict warning.1. What outcome do you want to achieve?
Complete the diffusion startup/request follow-up suggested when #8092 was merged, as an independent contribution.
2. What is the highest-leverage variable for that outcome?
Runtime acceptance coverage: extend the existing backend configuration/component tests to a deployed worker that actually serves requests.
3. How will this work move that variable?
Add small native FLUX startup/request tests for both
mpanduni, and fix the deploy YAML configuration loss exposed while preparing that validation.4. Whose customer or user needs does this address, and what are those needs?
vLLM-Omni maintainers and users need evidence that requested diffusion backends reach the worker and execute, rather than only surviving configuration parsing. This contribution supplies that follow-up validation in a small test workload.
Purpose
Follow up on #8092 with deployment-to-request coverage for dense tiny FLUX.
Before this change, generic diffusion fallback did not pass the supplied deploy YAML into its typed builder. A YAML-only regression requesting
linear_backend=torch, SDPA, eager mode, anduniinstead observedauto, no explicit attention selection,False, and no explicit executor. Supplying those values again through kwargs passed, which isolated the missing deploy path.The resolver now supplies that path to the generic typed builder. The builder merges the complete deploy config through
VllmOmniConfig.from_pipeline_configbefore deriving generic defaults; terminal dtype/cache/parallel defaults are not promoted to explicit CLI overrides. YAML/platform/caller precedence, model output metadata, and generic parallel/device/sampling/extras behavior are regression-tested. The existing no-YAML path is retained. Engine aliases enter the merge only in canonical stage form, soquantizationand auxiliary encoder overrides do not reappear as conflicting raw global kwargs.The new
test_backend_runtime.pyuses the shared async runner lifecycle and the existing tiny FLUX builder. A worker extension observes the real selector, Torch GEMM/SDPA calls, and finite transformer outputs while delegating computation to the original implementations. Backend, dtype, eager, and executor settings come only from deploy YAML. Each executor handles two sequential requests with counters reset before each request. PID assertions distinguish the multiprocessing worker from this single-stage, single-replica inlineuniworker.The existing synthetic FLUX backend component test now passes its local temporary directory as the model path instead of the fake model ID
unused. This keeps the component smoke offline and allows it to reach the real backend computation; no production model-path behavior is changed.The configuration module design page is a deferred draft and identifies current code/tests as authoritative. This fixes an existing deployment input path without adding fields or changing the public support matrix. The attention selection contract and user guides are unchanged because selection policy and kernels are unchanged.
Test Plan
vLLM Version: 0.31.0
vLLM-Omni Commit:
a7821c4cf6d36958dbd2525e3c8f49dc0da78c73, based on8da036a81e5a5680e6ff0b77cb759e45da9b4194. Validation ran on the corresponding pre-commit working tree; all 13 outgoing file hashes were verified unchanged before committing.Environment: WSL2 Ubuntu 24.04.4, Python 3.12.3, NVIDIA RTX 3080 10 GiB (sm_86), driver 616.56, torch 2.13.0+cu130 / CUDA runtime 13.0, Triton 3.7.1, transformers 5.14.1, pytest 9.1.1.
Runtime workload: generated random tiny FLUX weights, default/native weight loading, bfloat16, eager, batch 1, 128×128, 2 steps, guidance 0, maximum sequence length 64, seed 42. Configuration/tokenizer inputs were staged from the official FLUX.1-schnell revision
741f7c3ce8b383c54771c7003378a50191e9efe9; model processes ran withHF_HUB_OFFLINE=1andTRANSFORMERS_OFFLINE=1. Full FLUX weights are not required.Test Result
mpanduni, two each). Each request executed all 29 tracked unquantized transformer linear layers, all 4 tracked SDPA attention layers, and 2 transformer forwards with finite[1, 64, 64]outputs; completed image outputs had the requested count and dimensions.KernelConfig.linear_backendtoautoafter successful startup producedassert 'auto' == 'torch'at the intended worker-backend assertion. The outgoing diff has no mutation. After restoring and verifying the editable candidate source, the final fresh-process rerun was 2 passed, with four successful real requests again.default_unquantized_gemmlinear layers, 2SDPAImplattention layers, and finite(1, 4, 64)output. The original fake-model failure was reproduced against the original base before changing the test input.mypy-3.10via the repository wrapper in an isolated Linux environment with exactly the declared hook dependencies: passed, 2 production files + 11 test files.torch.cudacall checks: passed on all 13 candidate files. These hooks ran on Windows; mypy ran separately in Linux because the pre-existingos.getuidreference is not a Windows API.The two explicitly deselected tests use uncached fake model IDs (
unusedandgeneric-diffusion) and fail withLocalEntryNotFoundErroroffline. Both failures were separately reproduced with the original HEAD factory/resolver loaded in memory. They were not changed or counted as passes.The existing CUDA ready-pipeline sweep includes the new module's
core_model/cudamarks and path; no CI YAML change is needed. These results are local validation, not an official Buildkite/GitHub CI result. The full project CUDA ready sweep was not run locally. AR MoE, diffusion MoE, alternative hardware, image quality, and performance are not validated by this dense tiny test.Review and AI assistance
OpenAI Codex assisted with source inspection, implementation, local test execution, and the PR description. The contribution follows the human contributor's confirmed #8092 follow-up intent. The human contributor reviewed and approved the final diff and public description and authorized submission; agent reviews are not presented as human review.