Repository navigation
Conversation
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/diffusion/index.md, docs/design/module/diffusion/offloader.md, docs/design/module/diffusion/diffusion_model_integration.md. Module owners: @david6666666 @Isotr0py @princepride Routing: @david6666666 via module of the changed files, module named in the PR description; @Isotr0py via module of the changed files, module named in the PR description; @princepride via module of the changed files, module named in the PR description @blondeCS, 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: three questions on the performance claim@blondeCS this PR reads as a performance or value claim:
Before the full evidence checklist, three short questions:
When you answer, the evidence that settles it is: base and head SHA, hardware, model, workload, warm-up and repeat count, mean or percentiles with their spread, and a correctness/quality-equivalence signal; an end-to-end claim also needs stage attribution. |
|
Self-review (head 2b31e55, 4 files, +197/−26) What I checked:
Not checked:
AI assistance: Claude Code helped draft this comment and ran the precheck. I reran the unit tests and pre-commit myself and reviewed the diff. |
|
Answers to the three questions. All numbers come from the runs in the description. Two GPUs: 2× RTX PRO 6000 Blackwell Server Edition, driver 580.82.09, torch 2.13.0+cu130, vLLM 0.31.0, 1. Bottleneck. With CFG on, each of the 50 steps runs the transformer twice, one call after the other, on one GPU. The timings show the transformer calls dominate: guidance 5 (100 calls) takes 21.65 ± 0.01 s, guidance 1 (50 calls) 11.41 ± 0.03 s. The difference, 10.24 s for 50 calls, is about 0.20 s per call, which leaves about 1.2 s of the guidance-1 run outside the transformer calls. This is arithmetic on end-to-end timings, not a profile; I have no profiler trace. 2. Value. Latency of one CFG image for users with two GPUs: 21.65 s → 12.07 ± 0.10 s (1.79×) at 2048², 50 steps, guidance 5, with a pixel-identical image. Before this PR, 3. A/B. There is one change, so one comparison: one GPU against two GPUs with
AI assistance: Claude Code helped draft this reply from my benchmark logs. I checked the numbers against the logs. |
Route the HiDream-O1-Image denoising loop through CFGParallelMixin so that cfg_parallel_size=2 runs the conditional and unconditional branches on two ranks. The sequential path keeps the same operations in the same order. Signed-off-by: blonde <164845202+blondeCS@users.noreply.github.com>
Share the baseline comparison between the existing TP=2 case and a new cfg_parallel_size=2 case. Signed-off-by: blonde <164845202+blondeCS@users.noreply.github.com>
…llel test Signed-off-by: blonde <164845202+blondeCS@users.noreply.github.com>
2b31e55 to
05a39a2
Compare
|
|
||
| def predict_noise( | ||
| self, | ||
| sample: HiDreamO1TextSample, |
There was a problem hiding this comment.
double check this sample data type, thanks
There was a problem hiding this comment.
Thanks, changed in 80c7855. predict_noise no longer takes the model-specific HiDreamO1TextSample. It takes the sample's fields as keyword arguments (input_ids, position_ids, attention_mask, full_attn_spans, vinput_mask) plus z, t_pixeldit and sigma, and the loop unpacks each sample into the kwargs it hands to predict_noise_maybe_with_cfg. full_attn_spans stays a list of index spans because that is what the model takes. _forward_once had no other caller, so its body moved into predict_noise unchanged.
@hsliuustc0106 this is also the point you raised in today's meeting.
Checked at 80c7855:
- CPU unit tests: 24 passed, 12 skipped. The CFG-Parallel test now stubs the model call instead of
_forward_once, so thevinput_maskselection and the velocity conversion run in the test. - pre-commit on the two changed files passes.
- RTX 5090, 2048×2048, 50 steps, seed 42: the images at guidance 5 and 1 are pixel-identical to 05a39a2.
I did not rerun the two-GPU benchmark. The description is updated.
AI assistance: Claude Code helped make this change, run the checks and draft this reply. I reviewed the change and the results.
predict_noise took the model-specific HiDreamO1TextSample. It now takes the sample's tensors as keyword arguments, and the denoising loop unpacks each sample into the branch kwargs it hands to the CFG mixin. _forward_once had no caller left besides predict_noise, so its body moved into predict_noise unchanged. The CFG-Parallel test now stubs the model call instead of _forward_once, so the vinput_mask selection and the velocity conversion run for real in the test. Signed-off-by: blonde <164845202+blondeCS@users.noreply.github.com>
Omni ReviewBot: no human activity for 7 days@blondeCS this pull request has had no human commit, comment or review since 2026-10-27. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
|
Nothing pending on my side: the only review comment was addressed in 80c7855 on Oct 21, and a re-review is requested. |
Contributor intent (required human answers)
These are my own answers. I wrote them in Chinese and translated them with Claude Code.
1. What outcome do you want to achieve?
Let HiDream-O1-Image support CFG parallel flexibly, depending on the case it runs in.
2. What is the highest-leverage variable for that outcome?
How many times each GPU calls the model per denoising step. That is decided at
v_guided = self.predict_noise_maybe_with_cfg(...): the loop hands all the arguments to this function in the mixin, and the function decides how many passes to run. With CFG on one GPU it is two calls per step; with CFG on two GPUs, one.3. How will this work move that variable?
Covered in my answer to question 2.
4. Whose customer or user needs does this address, and what are those needs?
Users who need to run HiDream-O1-Image with CFG parallel on multiple GPUs.
Purpose
Before:
HiDreamO1ImagePipelineraisesNotImplementedErrorwhencfg_parallel_size > 1, and the feature table lists CFG-Parallel as unsupported for HiDream-O1-Image.After: the denoising loop goes through
CFGParallelMixin. With--cfg-parallel-size 2, one GPU runs the conditional branch and the other the unconditional one, and the two results are exchanged every step. On 2× RTX PRO 6000 Blackwell, a 2048×2048, 50-step image withguidance_scale=5goes from 21.65 s to 12.07 s (1.79×), and the output is pixel-identical to the single-GPU image. On one GPU, with or without CFG, the loop runs the same operations in the same order as before.Changes:
HiDreamO1ImagePipelineinheritsCFGParallelMixin, and thecfg_parallel_size > 1guard is removed.predict_noise: takes the text sample's fields (input_ids,position_ids,attention_mask,full_attn_spans,vinput_mask) andz,t_pixeldit,sigmaas keyword arguments, runs the model once, and converts the x-prediction to a velocity as the loop did inline. It takes over the body of_forward_once, whose only caller was that loop.predict_noise_maybe_with_cfg(withcfg_normalize=False) andscheduler_step_maybe_with_cfg(passing the request's scheduler). The mixin's default combine,n + scale * (p - n), is the expression the loop already used.cfg_parallel_size=2e2e accuracy case next to the existing TP=2 one (they now share a helper), and the feature table cell.Docs: the feature contract
docs/design/feature/cfg_parallel.mdand the user guidedocs/user_guide/diffusion/parallelism/cfg_parallel.mdneed no change. This PR uses the mixin as the contract documents, and the guide's supported-model list points to the feature table updated here.Related:
predict_noise) in its own pipeline. This PR does it for the pipeline that is on main.Not in this PR
Cache-DiT together with CFG parallel.
HiDreamO1TextModeldeclareshas_separate_cfg=True, which does not match one forward per GPU per step under CFG parallel. The open #8998 would resolve that flag toFalsewhen the CFG-parallel group has more than one rank, and it covers model-declared adapters like this one, so this PR leaves the flag alone. I have not run Cache-DiT with CFG parallel on HiDream-O1-Image.Test Plan
vLLM Version: 0.31.0
vLLM-Omni Commit: this branch, rebased on Oct 19 onto 71f0fa4 to resolve a conflict in the feature table. The GPU runs used 45df6b3 on top of 5b95f27. The pipeline file is byte-identical after the rebase, and the commits after it up to 05a39a2 change tests only. Main now includes #8475, which makes the mixin set
do_true_cfgonself.transformerwhen the pipeline has one; this pipeline keeps its model inself.model, so that step does nothing here. I did not rerun the GPU benchmarks after the rebase. The unit tests and pre-commit below were rerun at 05a39a2, the rebased head. On Oct 21, after review, 80c7855 changedpredict_noiseto take plain tensors instead ofHiDreamO1TextSample; at 80c7855 I reran the unit tests, pre-commit and the single-GPU pixel check, but not the two-GPU runs.Each image run is one fresh process:
The reported time is the script's "Total generation time": the
generatecall after the engine is up. Model loading is excluded, and there is no warm-up request.pytest tests/diffusion/models/hidream_o1_image tests/diffusion/distributed/test_cfg_parallel.py. The new file stubs the model call (self.model) and keeps the rest offorwardreal. One test runs two threads as two CFG ranks, each through its ownforward, and checks that rank 0 only runs the conditional branch, rank 1 only the unconditional one, and both end bit-identical to the loop as it was written before.--cfg-parallel-size 2.pytest tests/e2e/accuracy/test_hidream_o1_image.py -m "slow and diffusion and cuda and H100 and cards_2" --run-level full_model.pre-commit runon the changed files.Test Result
Two GPUs (2× RTX PRO 6000 Blackwell, 3 runs each, mean ± std):
cfg_parallel_size=2Single GPU, main against this PR (RTX 5090, one run each):
Single GPU, 05a39a2 against 80c7855 (the
predict_noisechange; RTX 5090, one run each, Oct 21):Tests
test_cfg_parallel.py. The new test file on unmodified main (71f0fa4): 2 failed (test_static_config_accepts_cfg_parallel,test_two_cfg_ranks_run_one_branch_each_and_agree), 2 passed (the single-GPU cases, which run main's own loop against the same stubbed model).tp2andcfg_parallel2) in 84 s. TheH100mark only selects the tests; they ran on the RTX PRO 6000 pair.The numbers from the two machines should not be compared with each other: different GPUs and different torch builds.
Not run
cfg_parallel_sizeabove 2.AI assistance: Claude Code helped write the change and the tests, run the benchmarks on my RTX 5090 and on two rented GPUs, translate my answers above, and draft the Purpose, Test Plan and Test Result sections. I reviewed the changes and the results.