Repository navigation
[Bugfix][Diffusion] Stamp the CFG branch for TeaCache instead of inferring it from parity - #8483
Conversation
b7f8e84 to
be1b987
Compare
Self-review
Known limits: request boundaries under step execution (the second half of #8482) are not addressed here. There was no GPU run, because request-mode decisions don't change and the step-execution path is covered by the CPU test. |
|
This PR touches vllm_omni/diffusion/, tests/diffusion/ (5 files). Based on CODEOWNERS coverage of the changed files, the most-related reviewers appear to be: Could one of you take a look when you get a chance? Thanks! |
… 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>
…rring it from parity TeaCacheHook chose the per-branch cache state for sequential CFG from _forward_cnt % 2. Request mode refreshes the hook before every request, so the parity always restarted at zero. Step execution never refreshes it, though: a CFG wave followed by a non-CFG wave with an odd number of forwards flipped the parity, and the next CFG step's positive call landed in the negative state, continuing from the other branch's residual (issue vllm-project#8482). CFGParallelMixin now stamps transformer.cfg_branch ("positive" / "negative") around each predict_noise call on all three paths (sequential CFG, CFG-parallel ranks, no CFG) and restores the previous value afterwards, also when predict_noise raises. TeaCacheHook prefers explicit sources and keeps the old rank/parity inference only for callers that provide none. A missing negative kwargs dict with do_true_cfg=True now raises a ValueError naming the branch. In request mode the stamp equals the old parity, so behaviour is unchanged there. Request boundaries under step execution (the second half of the issue) are not addressed here. This composes with the per-call extractor hint added for MammothModa2 (ctx.extra_states["teacache_branch"], PR 5357): the extractor hint wins, then the mixin stamp, then inference. Both explicit sources go through the same validation, keeping that PR's error message for invalid hints. Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
be1b987 to
461418b
Compare
|
This PR appears to belong to: docs/design/module/diffusion/index.md, docs/design/module/cache_management.md. Module owners: @princepride @Isotr0py @SamitHuang Routing: @princepride via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @Isotr0py via module of the changed files, module named in the PR description, semantic router; @SamitHuang via module of the changed files, module named in the PR description, semantic router @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 zcode (GLM-5.3-Flash) under experiment |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 54a7abc3-b66e-4b7f-93ec-67500d6f0d71) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 7bf61e2b-fd49-441c-8cff-7c5af1d3998f) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 55b9456f-8250-4867-906f-1c304896e39e) — check |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
0 actionable finding(s).
CI at
0faff74962e7(2026-10-10T07:26:15.033528+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Note: The assigned review arm
strict/zcode/GLM-5.3-Flashcould not complete this review, so it was produced by the fallback armdirect/cursor/auto. It is excluded from the routing experiment.
Full review analysis
PR description
CFGParallelMixin.predict_noise_maybe_with_cfg sets transformer.do_true_cfg for the call, then _predict_noise_for_cfg_branch writes transformer.cfg_branch to "positive" or "negative" around every predict_noise on the sequential, CFG-parallel, and no-CFG paths and restores the previous value in a finally. TeaCacheHook chooses the cache context from the extractor hint teacache_branch when that hint is present, otherwise from this stamp, and only then from the previous rank or forward-count parity. Under step execution a CFG wave, an odd non-CFG wave, and another CFG wave therefore stay on positive, negative, positive, positive, negative instead of swapping the last wave. Request-mode two-call CFG still matches the old parity because the hook counter restarts at zero each request.
Change flow
flowchart LR
waves["[EXISTING] CFG wave, non-CFG wave, CFG wave"]:::existing
mixin["[CHANGED] CFGParallelMixin.predict_noise_maybe_with_cfg"]:::changed
stamp["[NEW] _predict_noise_for_cfg_branch"]:::new
hook["[CHANGED] TeaCacheHook hint, then stamp, then parity"]:::changed
states["[EXISTING] teacache_positive and teacache_negative"]:::existing
waves --> mixin --> stamp --> hook --> states
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
No actionable findings.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
Omni ReviewBot: no human activity for 12 days@fusheng-ji this pull request has had no human commit, comment or review since 2026-10-04. 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. |
Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
|
This PR is still active. I've merged the latest main (b164fea) and rerun the CFG/TeaCache CPU checks: 43 passed, with no new mypy diagnostics. Seven MammothModa2 cases encounter the same no-GPU setup errors on main. This remains stacked on #8475, so please focus on the explicit branch-stamping change. @princepride @Isotr0py, could you review the CFG → non-CFG → CFG behavior and extractor-hint/stamp/fallback precedence when available? Request-boundary resets under step execution remain outside this PR. |
Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com>
#8483 #9272 #8802 #9248 #9282 #9264 (#9295) Signed-off-by: psv666 <2693925048@qq.com> Signed-off-by: haic0 <149741444+haic0@users.noreply.github.com> Signed-off-by: andyluo7 <andy.luo@amd.com> Signed-off-by: Joshna Medisetty <joshna.medisetty@intel.com> Signed-off-by: Chendi Xue <chendi.xue@intel.com> Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com> Signed-off-by: NumberWan <wantszkin2003@gmail.com> Signed-off-by: Sylvan Liu <Sylvan.Liu@amd.com> Signed-off-by: wangqiankun13 <wangqiankun13@huawei.com> Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Signed-off-by: Weiming Liao <liaowm5@gmail.com> Co-authored-by: psv666 <150513104+psv666@users.noreply.github.com> Co-authored-by: haic0 <149741444+haic0@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: andyluo7 <andy.luo@amd.com> Co-authored-by: Joshna-Medisetty <joshna.medisetty@intel.com> Co-authored-by: Chendi Xue <chendi.xue@intel.com> Co-authored-by: Wenbo Ji 嵇文博 <36562829+fusheng-ji@users.noreply.github.com> Co-authored-by: dengyunyang <584797741@qq.com> Co-authored-by: NumberWan <wantszkin2003@gmail.com> Co-authored-by: Sylvan Liu <syliu@amd.com> Co-authored-by: wangqiankun13 <wangqiankun13@huawei.com> Co-authored-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Weiming Liao <liaowm5@gmail.com>
Purpose
Part of #8482 (the explicit-branch half). Stacked on #8475: the first commit here is #8475's change rebased onto current
main(same patch); please review only the second commit. I'll rebase ontomainas soon as #8475 merges.TeaCacheHookpicks the per-branch cache state for sequential CFG from_forward_cnt % 2. In request mode the runner refreshes the hook before every request, so the parity restarts at zero and stays aligned. Under step execution the hook is never refreshed, so a CFG wave followed by a non-CFG wave with an odd number of forwards flips the parity. The next CFG step's positive call then lands in the negative state and continues from the other branch's residual and distance. This was raised in the #8475 review and is pre-existing for Flux / Qwen-Image; #8475 widens it to everyCFGParallelMixinuser.Change:
CFGParallelMixinstampstransformer.cfg_branch("positive"/"negative") around eachpredict_noisecall on all three paths: sequential CFG, each CFG-parallel rank, and no CFG. A new_predict_noise_for_cfg_branchhelper restores the previous value afterwards, also whenpredict_noiseraises, so the stamp never leaks into calls made outside the helper.TeaCacheHookprefers explicit sources, most specific first: the extractor's per-call hintctx.extra_states["teacache_branch"]added for MammothModa2 in [Perf] Support TeaCache for MammothModa2 #5357, then the mixin stamp. Both go through the same validation, which keeps [Perf] Support TeaCache for MammothModa2 #5357's error message for invalid hints. The old inference (CFG rank, else forward-count parity) moves unchanged into_infer_cfg_branchand is used only when neither is present. MammothModa2 does not use the mixin, so in practice the two explicit sources never meet.CachedTransformerdeclarescfg_branch = Nonenext todo_true_cfg.do_true_cfg=Trueandnegative_kwargs=None, the helper now raises aValueErrorthat names the branch, instead of a bareTypeErrorfrom**None.In request mode the stamp equals the old parity, so TeaCache decisions there are unchanged. Every TeaCache model that runs CFG as two calls (Flux, Flux Kontext, Qwen-Image, Flux2, Flux2Klein, LongCat-Image) goes through the mixin. Z-Image batches both branches into one call and is unaffected.
Not in this PR: request boundaries under step execution (the second half of #8482, which needs a maintainer decision).
Test Plan
test_interleaved_non_cfg_wave_keeps_branch_states_aligned: a realTeaCacheHookon a probe module, driven throughCFGParallelMixinas CFG wave -> non-CFG wave -> CFG wave. It asserts the hook selectspositive, negative, positive, positive, negative. Before this change it selectednegative, positivefor the last wave.test_unstamped_callers_keep_the_parity_fallback: direct calls without a stamp keep the old parity behaviour.test_extractor_hint_takes_precedence_over_stamp,test_invalid_stamp_is_rejected: the [Perf] Support TeaCache for MammothModa2 #5357 hint wins over the stamp, and an invalid stamp raises.test_cfg_helper_stamps_branch_per_call_and_restores_it,test_cfg_parallel_rank_stamps_its_branch[0/1],test_cfg_branch_stamp_is_restored_when_predict_noise_raises: the mixin stamps the right branch on each path and restores it.The CFG-parallel test also patches
cfg_parallel.is_cfg_group_initialized, because_get_cfg_world_size_or_one()reports 1 without an initialized group.vLLM Version: 0.30.0
vLLM-Omni Commit: main
c1e84ce9(includes #5357) + #8475 (rebased; the CPU results below were re-run there)Test Result
tests/diffusion/cache/,test_cfg_parallel.py,test_diffusion_step_pipeline.py, the Flux, Flux2, Qwen-Image and LongCat-Image model suites, and MammothModa2's TeaCache tests: 350 passed with this PR vs 340 onmain. Both trees show the same environment-dependent failures/errors (ProcessGroupNCCL is only supported with GPUs, andReplicatedLinearhas nocpu_linearwhen building the real MammothModa2 module on CPU); none are new. MammothModa2's TeaCache tests pass on both (9/9).hook.pyandbase.py).mypy-3.10on the touched files: 3 errors vs 5 onmain, none new.AI assistance: Codex and Claude Code were used to trace the cache path, draft the change, tests and this description. All changes were reviewed and the tests run by the author.