Repository navigation
[Bugfix][Diffusion] Keep TeaCache CFG branches separate in the shared CFG helper - #8475
Conversation
… CFG helper TeaCache keeps one cache state per CFG branch and decides which branch a transformer call belongs to from transformer.do_true_cfg. Only Flux, Flux Kontext and Qwen-Image set that flag; every other pipeline that runs sequential CFG through CFGParallelMixin.predict_noise_maybe_with_cfg left it unset. For TeaCache-enabled models among them (Flux2Klein, Flux2 with a negative prompt, LongCat-Image) the positive and negative calls of a step therefore shared one state. Both calls see the same latents and timestep, so the second call's modulated input matched the first exactly and it could reuse the other branch's residual, collapsing the guidance. Set transformer.do_true_cfg in predict_noise_maybe_with_cfg, where the branch layout is known, so every pipeline using the helper gets per-branch TeaCache states. The flag is reset for calls without CFG. Pipelines without a `transformer` attribute are untouched, and multi-branch CFG uses a separate method. TeaCache is the only reader of the flag. On FLUX.2-klein-base-4B (50 steps, guidance 4.0, rel_l1_thresh 0.2, 1024^2) mean SSIM against the uncached image goes from 0.59 to 0.90 at a similar speed-up; uncached output is byte-identical. Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
Self-review
|
|
This PR appears to belong to: docs/design/module/diffusion/parallelism.md, docs/design/module/diffusion/index.md. Module owners: @princepride @xuechendi @david6666666 Routing: @princepride via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @xuechendi via module of the changed files, module named in the PR description, semantic router; @david6666666 via module of the changed files, module named in the PR description @fusheng-ji, 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 routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Verdict: Approve. Reviewed at diff level against merge-base ee8fdab1, plus a logic-level replay of the branch-selection code (no GPU runs from my side). The change is minimal, correctly scoped, and the evidence chain in the description (SSIM/PSNR A/B, byte-identical uncached outputs, flag-only A/B tree) is unusually strong.
What I verified:
transformer.do_true_cfghas exactly one reader in production code (vllm_omni/diffusion/cache/teacache/hook.py:148). Flux / Flux-Kontext / Qwen-Image already set the same value themselves, so their behavior is unchanged; every other mixin user with sequential CFG left it unset, so pos/neg calls both landed inteacache_positive— confirming the reported guidance collapse for the TeaCache-enabled ones (Flux2Klein, Flux2, LongCat-Image).- The CFG-parallel path is safe: the hook selects the branch from
cfg_rank(not the counter) whencfg_parallel_size > 1, so setting the flag on all ranks gives each rank a consistent branch. - The multi-branch path (
predict_noise_with_multi_branch_cfg, Bagel) is untouched, and pipelines without a.transformerattribute fall through thegetattrguard. - A pure-Python replay of
predict_noise_maybe_with_cfg+ the hook's branch selection confirms: onmainboth calls of a CFG step share one state; with this PR they separate. The two new CPU regressions cover exactly this and the no-transformerpath.
Two non-blocking notes:
- The sequential-path branch assignment relies on
_forward_cnt % 2parity. An odd number of non-CFG forwards interleaved between CFG steps (e.g. separate CFG/non-CFG batches alternating on the same transformer under continuous scheduling, since_prepare_cfg_scalarsforbids mixing within a batch) flips the parity: the two branches of a step still land in different states, but their histories swap, so a branch can continue from the other branch's residual/distance state. This is pre-existing for Flux/Qwen-Image; this PR widens exposure to the other mixin users. Worth a follow-up that makes the mixin stamp an explicit branch id per call instead of relying on parity. - The SPDX copyright-line change rides along; fine if the hook enforces it, just noting it's unrelated churn.
Nit on the notes: none blocking — happy to see this merged as-is.
|
Thanks for the thorough review and the logic-level replay, @hsliuustc0106!
|
Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
|
Latest head:
The shared CFG helper supports direct pipeline calls without a CFG process group, but enabling the real TeaCache hook could still trigger an assertion when the hook queried the group's world size. The hook now treats an uninitialized CFG group as world size 1 and follows sequential CFG, keeping positive and negative cache states separate. Initialized groups continue to select the branch by rank, and errors from an initialized group still propagate. The commit adds five regressions that actually install TeaCache through
Validation after
The PR description reflects these results. The full diffusion CPU sweep and the original image-quality measurements were not repeated after the fallback or cleanup commits; the focused and cache CPU/GPU suites above were rerun where noted. |
4058696 to
978fd4a
Compare
Centralize transformer.do_true_cfg on CFGParallelMixin (including PP>1), drop redundant Flux/Qwen writes, tighten tests, and revert unrelated SPDX churn. Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
978fd4a to
52df1b0
Compare
|
Following up on the earlier update: commit 52df1b0 adds the following changes on top of f041cb2:
The uninitialized-CFG-group fallback introduced in f041cb2 remains unchanged. Sequential TeaCache branch selection still uses counter parity; explicit per-call branch identification is tracked separately in #8483. Validation clarification: the earlier comment’s “Validation after The previously reported 30 CPU passes, 10 two-B200 CFG passes, 8 GPU cache passes, and the 205 passed / 8 failed / 7 errors CPU cache result were obtained on f041cb2, before this cleanup. At the time of that update, GPU suites, the full diffusion CPU sweep, and image-quality measurements had not been rerun on 52df1b0. Subsequent GPU validation is recorded below. Subsequent GPU validation on
|
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
|
Four-GPU follow-up on 7e88fc3 (the current head of this PR):
This closes the previously stated four-GPU topology test gap for #8475. It does not establish full pretrained-model PP + TeaCache + CFG generation or image/video quality. Wan model adaptation, calibration and full-model validation are being developed separately in draft #8731, which also incorporates the explicit-branch dependency from #8483. |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 120s (try 2 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; retrying strict/cursor/cursor-grok-4.6-high in 600s (try 3 of 3)). |
Omni ReviewBot attempt recordReview attempt ended as failed (failed; falling back to direct/cursor/auto). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; retrying direct/cursor). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; retrying direct/cursor). |
Omni ReviewBot attempt recordReview attempt ended as failed (Cursor exited with 1: ActionRequiredError: You've hit your usage limit You've saved $3501 on API model usage this month with Ultra. Switch to a different model or set a Spend Limit to continue with Auto. Your usage limits will reset when your monthly cycle ends on 10/20/2026.; no fallback: the fallb). |
|
Hi @Isotr0py @princepride @SamitHuang, could you take a look at this TeaCache/CFG fix when you have a chance? The PR centralizes On commit Explicit per-call branch identification is handled separately in #8483. Feedback on the shared-helper placement and TeaCache fallback would be especially helpful. Thanks! |
vllm-project#8475 makes predict_noise_maybe_with_cfg set transformer.do_true_cfg before every call, so the SD3 pipeline no longer needs to set it in its denoise loop. Drop that line and leave pipeline_sd3.py as on main. The SD3 diffuse test now runs the real CFG helper and a real scheduler and checks the flag the transformer sees on every call, instead of faking the pipeline with SimpleNamespace. The SD3 extractor test builds its tiny transformer from OmniDiffusionConfig and TransformerConfig. Signed-off-by: blonde <164845202+blondeCS@users.noreply.github.com>
vllm-project#8475 makes predict_noise_maybe_with_cfg set transformer.do_true_cfg before every call, so the SD3 pipeline no longer needs to set it in its denoise loop. Drop that line and leave pipeline_sd3.py as on main. The SD3 diffuse test now runs the real CFG helper and a real scheduler and checks the flag the transformer sees on every call, instead of faking the pipeline with SimpleNamespace. The SD3 extractor test builds its tiny transformer from OmniDiffusionConfig and TransformerConfig. Signed-off-by: blonde <164845202+blondeCS@users.noreply.github.com>
Purpose
Part of #8411 (item 2,
Flux2Klein).TeaCacheHookkeeps one cache state per CFG branch. It decides which branch a transformer call belongs to fromtransformer.do_true_cfg: when the flag is set and CFG is sequential, calls alternate positive/negative on a counter; otherwise every call goes to thepositivestate.Only Flux, Flux Kontext and Qwen-Image set that flag. Every other pipeline that runs sequential CFG through
CFGParallelMixin.predict_noise_maybe_with_cfgleaves it unset. For the TeaCache-enabled ones among them (Flux2Klein, Flux2 with a negative prompt, LongCat-Image), the positive and negative calls of each step therefore share one TeaCache state. Both calls see the same latents and timestep, so the negative call's modulated input equals the positive call's exactly; its distance is driven only by the polynomial's constant term, and the two branches reuse each other's residuals. Guidance collapses and the output drifts to a different composition.Change:
CFGParallelMixin._set_transformer_do_true_cfgsetsself.transformer.do_true_cfgfrompredict_noise_maybe_with_cfg(and fromPipelineParallelMixinwhen PP world size > 1) before predicting, where the branch layout is known. The flag is cleared for calls without CFG. Pipelines without atransformerattribute are untouched, multi-branch CFG goes through the separatepredict_noise_with_multi_branch_cfg, and TeaCache is the only reader of the flag, so uncached generation is unchanged. Redundant per-pipeline writes in Flux, Flux Kontext, and Qwen-Image were removed so the mixin is the single source of truth.Follow-up: sequential CFG still alternates TeaCache branches via
_forward_cntparity when non-CFG forwards interleave; explicit per-call branch stamping is tracked in #8483 (not in this PR).For direct pipeline calls without an initialized CFG process group,
TeaCacheHooknow treats the CFG world size as 1 and uses sequential positive/negative cache states. Initialized groups still select the branch by rank, and errors from an initialized group are propagated. This matches the shared CFG helper's fallback.No coefficient change. On FLUX.2-klein-base-4B the current (FLUX.1-borrowed) coefficients over-predict the output change by about 1.9x, which keeps TeaCache conservative; once the branches are separated they give quality comparable to the calibrated Z-Image result in #8408. A 4th-order refit on 10,220 within-branch pairs (70 Parti prompts x {25, 50} steps) reached only R² = 0.15, so I am not proposing new coefficients; details in #8411.
Test Plan and Results
Branch head:
52df1b0aa72016d5cd35a3be71f7ac19b9e51d44(fix/teacache-cfg-branch-state).Validation environment: Python 3.12.13, Torch 2.13.0, vLLM 0.31.0 loaded from a matching wheel, and 2 x B200. Tests use
VLLM_WORKER_MULTIPROC_METHOD=spawn; CPU runs hide CUDA devices. Three-card cases are excluded.After the uninitialized-group fallback fix (and the review cleanup that centralizes the flag and adds a PP>1 CPU regression):
core_model and cpufull_model and cuda and B200 and cards_2core_model and cudaTeaCacheHookthroughapply_teacache_hook. They verify separate positive/negative residuals and cache hits on the next step with an absent or single-rank CFG group and initialized rank selection. A low-value broken-CFG-group probe was dropped in review cleanup.PYTHONPATH=/tmp/vllm-8474-wheel+ worktree, vLLM 0.31.0):test_teacache_cfg_group.py+test_cfg_parallel.py(core_model and diffusion and cpu) → 23 passed; newtest_pp_predict_noise_sets_transformer_do_true_cfg→ 1 passed. GPU suites were not rerun for the cleanup commit.transformerattribute. GPU CFG coverage includes sequential/parallel CFG, normalization, and multi-branch CFG. GPU cache coverage includes TeaCache image generation and Flux2/Klein extractor checks.539c5337, including failure messages; no new failures were found.core_model and cpusweep onbba2b29produced 7,190 passed, 89 failed, 183 errors, and 79 skipped. All 272 failing/error cases were rerun on baseline539c5337with matching messages. The full diffusion sweep was not repeated after the fallback fix; the focused CPU, cache CPU, and GPU suites above were rerun.git diff --checkpass. All applicable pre-commit checks pass exceptmypy-3.10: two pre-existingextractor_fn/Nonetyping errors inhook.pywere independently reproduced on the baseline. The new test file passes mypy.Full logs, JUnit reports, and the standalone real-hook probe are retained outside the repository under
/tmp/pr8475-validation-20261005/on the validation host.Original Image Quality Measurements
These measurements predate the final uninitialized-group fallback fix and were not rerun for that fix. Environment: 1 x B200, frozen
vllm/vllm-openai:v0.30.0container, baselineee8fdab1, offlineOmni, 1024x1024, the #8408 protocol (6 prompts x 3 seeds; each TeaCache image against its uncached twin; SSIM with a Gaussian window, sigma 1.5, population covariance; warm-up excluded; speed-up from mean request latency).TeaCache at the default
rel_l1_thresh=0.2, 50 steps,guidance_scale=4.0(both model cards' defaults), each TeaCache image against its uncached twin:mainee8fdab1: mean SSIM (min) / PSNR / speed-upblack-forest-labs/FLUX.2-klein-base-4Bmeituan-longcat/LongCat-Imagemainvs this PR, and FLUX.2-klein-base-4Bmainvs the flag-only A/B tree.FLUX.2-klein-4B(distilled, 4 steps, no CFG) is unaffected: per-step input change is 0.33-0.53, so TeaCache never skips at 0.2 and images match the uncached ones exactly, before and after.mainthe damaged images are different compositions, not a blur (e.g. an isometric library turns into an incoherent scene).An A/B tree that only adds the flag inside the Klein pipeline gives the same SSIM as this PR to the last digit. Scripts, images and logs are kept outside the repository and can be shared on request.
AI assistance: Codex and Claude Code were used to trace the cache path, design measurements, and draft changes, tests, and this description. Codex executed the latest CPU/GPU validation and baseline comparisons.