Repository navigation
[Bugfix][Examples][MammothModa2] Serve T2I via LLM-typed DiT stage (#7199) + benchmark harness and profiler wiring - #7293
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/vllm_omni_config.md, docs/design/module/entrypoints.md, docs/design/module/diffusion/offloader.md, docs/design/module/diffusion/index.md, docs/design/module/profiling.md. Module owners: @david6666666 @wtomin @Isotr0py Routing: @david6666666 via module of the changed files, module named in the PR description; @wtomin via module named in the PR description, CODEOWNERS; @Isotr0py via module named in the PR description @quan-yi-ai, 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. |
|
Thanks for coordinating the overlap. #7134 is now Ready for review at head When rebasing or stacking #7293, the profiler switch can therefore use the existing first-class #7134 will continue to keep the profiler mixin, reusable benchmark harness, and online sweeps out of scope, so ownership remains with #7293/#7215. |
55abe2e to
5e52105
Compare
| omni_kwargs["enable_cache_dit_summary"] = True | ||
| if args.enable_diffusion_pipeline_profiler: | ||
| omni_kwargs["enable_diffusion_pipeline_profiler"] = True | ||
| if args.ulysses_degree and args.ulysses_degree > 1: |
There was a problem hiding this comment.
[P2] Preserve explicit CLI overrides equal to defaults
These conditions silently discard explicit values such as --ulysses-degree 1 and --ulysses-mode strict. With a deploy YAML specifying different values, users can no longer override them, potentially retaining unwanted multi-GPU execution. Use None as the argparse default and forward every explicitly supplied value; add a regression test covering YAML overrides.
There was a problem hiding this comment.
Addressed in 1f574db2 ("Preserve explicit CLI parallel-knob overrides equal to defaults"): argparse defaults for the five knobs are now None, every explicitly supplied value is forwarded (so --ulysses-degree 1 / --ulysses-mode strict can override the deploy YAML again), and TestParallelKnobForwarding::test_script_defaults_are_none + override-forwarding regressions pin the behavior.
…aults Address review feedback on vllm-project#7293: parallel knobs (ulysses_degree, ulysses_mode, ring_degree, cfg_parallel_size, vae_patch_parallel_size) now default to None and every explicitly supplied value is forwarded, so flags like --ulysses-degree 1 can override a deploy YAML instead of being silently dropped. Adds regression tests against the real argparse and forwarding helper.
…aults Address review feedback on vllm-project#7293: parallel knobs (ulysses_degree, ulysses_mode, ring_degree, cfg_parallel_size, vae_patch_parallel_size) now default to None and every explicitly supplied value is forwarded, so flags like --ulysses-degree 1 can override a deploy YAML instead of being silently dropped. Adds regression tests against the real argparse and forwarding helper. Signed-off-by: quan-yi-ai <82917002+quan-yi-ai@users.noreply.github.com>
7eeb651 to
ca920b7
Compare
|
I noticed that the latest commit For MammothModa2, that compatibility path becomes obsolete once #7134 lands: stage 1 is registered as a standard Could we keep the scopes separated by either:
If generic support for other LLM-typed image stages such as Dynin is still needed, that would be better handled as a separate serving PR coordinated with #7199's owner. The current seven-point serving results should also be identified as legacy-topology baseline results until they are rerun after the #7134 migration. |
| ar_width = int((ar_prompt_info.get("ar_width") or [0])[0]) | ||
| ar_height = int((ar_prompt_info.get("ar_height") or [0])[0]) | ||
| if ar_width > 0 and ar_height > 0 and hasattr(default_stage_params, "max_tokens"): | ||
| default_stage_params.max_tokens = ar_height * (ar_width + 1) + 1 |
There was a problem hiding this comment.
This AR grid sizing now exists in three places: here, the offline example, and bench_startup.py in #7312 (ar_height * (ar_width + 1) + 1). Worth lifting into the model_extras spec (e.g. have the T2I builder or a registered helper own it) so a grid-contract change can't silently desynchronize one copy.
There was a problem hiding this comment.
Addressed: the formula now has a single owner — ar_grid_max_tokens(ar_width, ar_height) in vllm_omni/diffusion/utils/param_utils.py (next to apply_declared_extra_args, already shared by examples and entrypoints). Both this call site and the offline example consume it, and TestARGridMaxTokens pins the contract (formula + None-for-absent-grid semantics), so a grid-contract change can't desynchronize a copy. The third copy you flagged in #7312's bench_startup.py is outside this PR's tree — worth switching it over in #7312 if that harness sticks around post-#7134.
|
The body's Changes section doesn't cover the largest part of this diff: the serving-path work ( |
|
Thank you @zhongc1211 for the independent verification — the config sweep results and the latency comparison against the legacy path are exactly the evidence this PR needed. Glad #7199 is confirmed fixed end-to-end. (For the record: the review comments on the benchmark command and dashboard link were addressed in 7257574, along with rejecting |
7257574 to
55f8b12
Compare
|
Rebased onto the latest main (head |
|
Found the failing test — it was ours: the deploy yaml's |
|
Sorry for the extra ping @hsliuustc0106 — gentle reminder: the fix for the failing lane test is up ( |
9a17400 to
e9a1122
Compare
|
The GPU lane and all GitHub checks are green on |
|
@quan-yi-ai No worries. Since this is a collaborative project, the PRs need to be merged in order. Once it passes Buildkite, you can just wait for the merge. |
22bb5d8 to
1e2cb35
Compare
|
Rebased onto v0.30.0 / post- |
Omni ReviewBot: supersededThe CI failure noted on |
…T stage - Admit generation-LLM DiT stages (stage_type="llm" + final_output_type="image") to image serving via a shared is_image_generation_stage classifier; /v1/images/edits keeps the pre-existing diffusion-only gate (fixes vllm-project#7199 503). - Route the registry T2I prompt builder by resolved model class so the AR envelope (<|image start|>W*H<|image token|>), additional_information and grid-sized max_tokens survive the vllm-project#7134 topology migration. - Forward undeclared request extras into extra_body and apply declared extra_args (text_guidance_scale / cfg_range / num_inference_steps) to both AR and DiT stages; fall back to extract_images_from_outputs for LLM-DiT multimodal_output payloads. - Extract ar_grid_max_tokens into param_utils so online serving and the offline example share one grid contract; make parallel knobs (ulysses/ring/cfg/vae-patch) default to None so deploy YAML values stay in effect unless explicitly overridden. - Add MammothModa2 serving performance dashboard, deploy yaml knobs, recipe docs, and regression tests for the serving surface. Signed-off-by: quan-yi-ai <82917002+quan-yi-ai@users.noreply.github.com>
…time contract The migrated pipeline's config test requires mammoth_moda2.yaml to keep stage-level default_sampling_params unset so request-level sampling fields are never shadowed. Remove the block (plus the no-op profiler flag line) and pass the benchmark anchor sampling params (text_guidance_scale 9.0, cfg_range [0.0, 1.0], num_inference_steps 50) per request via --extra-body in the serving benchmark commands so the recorded numbers stay reproducible. Fixes the 'Simple - Other Test' failure on build #15723. Signed-off-by: quan-yi-ai <82917002+quan-yi-ai@users.noreply.github.com>
62f82a0 to
09095de
Compare
|
The previous lane failure looks infra-side: the only failed step was |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Both 09-12 comments are resolved and the body now covers the serving-path scope: the CLI-override P2 is fixed properly (argparse defaults moved to None so only explicit values forward, with --ulysses-degree 1 overriding the YAML as the docstring itself notes) and pinned by the new 140-line test_text_to_image_cli_forwarding.py; the AR grid-sizing triplication is lifted into the shared ar_grid_max_tokens() helper used by both the online path and the offline example. Mergeable and checks green. Good to merge.
Omni ReviewBot routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
Purpose
Fixes #7199 — serve MammothModa2-Preview (T2I) through
/v1/images/generations, plus the MammothModa2 text-to-image benchmark harness and profiler wiring for #7215 (Refs #7075).MammothModa2 is a two-stage AR→DiT pipeline whose DiT stage runs as a generation-LLM stage (
stage_type: llm,final_output_type: image) rather than adiffusion-typed stage. The image endpoints only looked fordiffusion-typed stages, so every/v1/images/generationsrequest returned HTTP 503 "No diffusion stage found" (#7199). This PR makes the serving path accept LLM-typed image stages end to end, and adds the profiler/benchmark tooling to measure it.Changes
A. Online serving fix (#7199) — the largest part of the diff:
entrypoints/openai/utils.py: newis_image_generation_stage()helper — accepts both classicaldiffusionstages and LLM-typed stages whosefinal_output_type == "image". Used by endpoint admission (app_state.py::_get_engine_and_model), stage lookup (api_server.py::edit_images), model-class resolution, and DiT param injection, so all four call sites share one semantic.serving_chat.py: multi-stage generation-LLM image serving —_build_multistage_generation_inputsroutes image requests through the T2I prompt builder (injects the|image start|>W*H|image token|>envelope),_resolve_multistage_model_class_name()(od_config first, LLM-wrapper fallback, cached) resolves the DiT model class when the pipeline has nodiffusionstage, and the AR stage getsextra_args/max_tokenswiring for the visual-token grid budget.api_server.py:/v1/images/generationsroute forwardsrequest.model_extraintoextra_bodyand hands multi-stage image requests to the chat handler;edit_imagesuses the shared stage-lookup semantic.protocol/images.py:ImageGenerationRequestisextra="allow"so per-model knobs reach the pipeline.B. Benchmark harness + profiler wiring (#7215, component-level baseline):
pipeline_mammothmoda2_dit.py:DiffusionPipelineProfilerMixinonMammothModa2DiTPipeline(same pattern as flux/bagel). This stage runs on the legacy LLM-generation path (built fromVllmConfig, noOmniDiffusionConfig), so the switch travels throughadditional_config, preserved for LLM stages per [Bugfix] Preserve additional_config for LLM stages #7272.mammoth_moda2.yaml: exposeadditional_config.enable_diffusion_pipeline_profiler(defaultfalse— zero overhead when off).text_to_image.pyexample: forward diffusion-only engine args only when explicitly set (_validate_global_stage_cli_ownershiprejects them for pipelines without a DIFFUSION stage); explicit CLI parallel-knob overrides equal to defaults are preserved (review feedback), and the AR gridmax_tokensbudget comes from the sharedar_grid_max_tokens()helper.diffusion/utils/param_utils.py:ar_grid_max_tokens(ar_width, ar_height)— single owner of thear_height * (ar_width + 1) + 1grid contract, shared by the online serving path and the offline example (review feedback; third copy lives in [Benchmark] Add MammothModa2 startup/loading breakdown #7312'sbench_startup.py).benchmarks/diffusion/performance_dashboard/mammoth_moda2_serving_performance.md+ raw JSON): serving latency/throughput sweep with stage breakdown. All numbers are legacy-topology baseline (DiT on the LLM path) and are expected to be rerun after [Model] Migrate MammothModa2 DiT to the shared diffusion runtime #7134 lands — see the callout in the dashboard.Coordination with #7134: #7134 owns the migration of this pipeline to the shared diffusion runtime (DiT as a standard
DIFFUSIONstage built fromOmniDiffusionConfig). Until it lands, the LLM-typed compatibility path added here is the only way to serve this model, so the serving fix lives in this PR and was verified against current main (7e520f9c). Once #7134 lands, the profiler switch moves to the first-classod_config.enable_diffusion_pipeline_profilerfield and the dashboard gets rerun — happy to follow up in a small PR as promised below.Test Plan
vLLM Version: 0.29.0
vLLM-Omni Commit: rebased on main
7e520f9cServing smoke (single A800-80G):
vllm-omni serve /path/to/MammothModa2-Preview --omni \ --deploy-config vllm_omni/deploy/mammoth_moda2.yaml --port 8000 curl /v1/images/generations # size 512x512Unit:
pytest tests/entrypoints/openai_api/test_image_server.py tests/examples/offline_inference/test_text_to_image_cli_forwarding.py -qTest Result
/v1/images/generations(512x512) returns HTTP 200 with a valid base64 image (28.5s end-to-end: AR + DiT + VAE). Before the fix: 503 on every request.7e520f9c: all configs 8/8 succeeded, no perf regression vs baseline — c1 mean 22.19s (baseline 21.8s), c2 24.72s (24.8s), c4 27.57s (28.5s). Raw JSON inmammoth_moda2_serving_results/.TestARGridMaxTokens(grid formula + None-edge), and CLI knob-forwarding regressions.cc @Levius-Fubuki (coordination on #7134 overlap), @hsliuustc0106 (entrypoints review + the
vllm bench serve --omnientry for PR2).