Repository navigation
[Bugfix][Wan2.2] Cache-DiT: support low-noise-only (single transformer_2) runs and discover cache targets - #8800
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/cache_management.md, docs/design/module/diffusion/index.md. Module owners: @Isotr0py @princepride @SamitHuang Routing: @Isotr0py via module of the changed files, module named in the PR description, CODEOWNERS; @princepride via module of the changed files, module named in the PR description, CODEOWNERS; @SamitHuang via module of the changed files, module named in the PR description, CODEOWNERS @xuechendi, 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 noteResolved as of |
| pipeline.scheduler.set_timesteps(num_inference_steps, device=device) | ||
| # The Wan UniPC scheduler is built with shift=1.0 and takes the flow shift | ||
| # here; the Euler scheduler ignores it and keeps its built-in shift. | ||
| pipeline.scheduler.set_timesteps(num_inference_steps, device=device, shift=pipeline._flow_shift) |
There was a problem hiding this comment.
The runner refreshes the cache before forward() reads the request's flow_shift, so this still uses the previous value. A request that overrides it (e.g. flow_shift=12.0 as in the 480p I2V example) on a server started with 5.0 still gets the 17/23 split, but denoises with a different one.
There was a problem hiding this comment.
Good catch, thanks. The refresh API only gets (pipeline, num_inference_steps), so the split couldn't see the request. Fixed in 3fee614:
- The runner now calls an optional pipeline.prepare_cache_refresh(first_req) right before cache_backend.refresh(...).
- Wan22Pipeline (skipped for DMD) and Wan22I2VPipeline use it to apply the request's sample_solver / flow_shift to the scheduler. They go through a new apply_wan_request_scheduler() helper, which forward() now reuses instead of duplicating the logic. Calling it again in forward() for the same request is a no-op.
- VACE overrides the hook as a no-op. Its forward() ignores per-request scheduler settings, so its split already matches.
New tests:
- test_wan22_step_split_follows_request_flow_shift_override: a server started with 5.0 gets a request with flow_shift=12.0, and the split matches the 12.0 schedule instead of 17/23.
- test_refresh_cache_prepares_pipeline_before_backend_refresh: the hook runs before the backend refresh.
| if transformer is None and transformer_2 is None: | ||
| raise ValueError(f"{type(pipeline).__name__} has no loaded transformer to enable cache-dit on") | ||
|
|
||
| if transformer is None or transformer_2 is None: |
There was a problem hiding this comment.
Could we add a test for the low noise only case (transformer is None, transformer_2 loaded)? The existing taylorseer test only covers transformer_2=None, and the new test only covers the step split.
There was a problem hiding this comment.
Added in 3fee614:
- test_wan22_custom_enabler_low_noise_expert_only: with transformer=None and transformer_2 loaded, the cache is installed on transformer_2 and the returned refresh refreshes transformer_2.
- test_wan22_custom_enabler_without_any_transformer_raises: covers the ValueError when neither expert is loaded.
0523857 to
3fee614
Compare
Signed-off-by: Chendi Xue <chendi.xue@intel.com> (cherry picked from commit b284339) (cherry picked from commit 92815d11e1ed587b657bd9cf35a9af9f0892b878)
…split (vllm-project#32) vllm-project#7576 builds the Wan UniPC scheduler with shift=1.0 and passes the flow shift to set_timesteps instead. _split_wan22_inference_steps still called set_timesteps without a shift, so it counted the unshifted schedule and refreshed the high/low-noise Cache-DiT contexts with the wrong step budgets: 40 steps at flow_shift=5 split N5/N35 instead of the N17/N23 the experts actually run (and that v0.28.0 reported). Pass the pipeline's flow shift for the UniPC solver so the split mirrors the denoise schedule. Signed-off-by: Chendi Xue <chendi.xue@intel.com> (cherry picked from commit 1077cc435755a4464f5fb16865cb17504ad2342f)
The runner refreshes Cache-DiT before forward() resolves the request's sample_solver / flow_shift, so the dual-expert step split was planned with the previous request's scheduler. A request overriding flow_shift (e.g. 12.0 on a server started with 5.0) got the 17/23 split but denoised with a different one. Add an optional prepare_cache_refresh(req) pipeline hook that the runner calls right before cache_backend.refresh(). Wan22Pipeline and Wan22I2VPipeline use it to apply the request's scheduler settings through a shared apply_wan_request_scheduler() helper, which forward() now reuses. VACE ignores per-request scheduler settings and keeps a no-op hook. Also cover the low-noise-expert-only case (transformer=None, transformer_2 loaded) and the no-transformer error in the Cache-DiT tests. Signed-off-by: Chendi Xue <chendi.xue@intel.com>
|
@LinzeShi @hsliuustc0106 , PTAL Setup: Wan2.2-T2V-A14B-Diffusers , server started with --boundary-ratio 0.875 --flow-shift 5.0. Expected split, from build_wan_scheduler("unipc", fs).set_timesteps(40, shift=fs) with boundary 875:
With this PR (3fee614): request 1: flow_shift=5.0 (same as server default) -> HTTP 200, 78 s(APIServer pid=689230) INFO 10-15 03:52:09 [serving_video.py:470] Boundary ratio parse: request=0.875 gen_params=0.875 request 2: flow_shift=12.0 (per-request override) -> HTTP 200, 67 s(APIServer pid=689230) INFO 10-15 03:53:27 [serving_video.py:470] Boundary ratio parse: request=0.875 gen_params=0.875 The high-noise (W4…M0) and low-noise (W2…M20) contexts get N17/N23 at flow_shift=5.0 and N26/N14 at the 12.0 override. Both match the schedule forward() denoises with. |
|
@hsliuustc0106 PTAL |
|
Reviewed head One pre-existing correctness case remains in the same cache-split path: when the engine is started without Please share resolution of the effective boundary (engine override → request → 0.875) between cache preparation and denoising, and cover both an omitted boundary and a request override. Keep the startup value separate from request-effective state so successive requests can choose different boundaries. This is an existing gap, not a regression introduced by this PR. Validation: source/call-chain review and static parsing; no model execution or independent hardware measurements. |
…xpert split Without --boundary-ratio the Cache-DiT split treated every step as high-noise (40/0) while forward() defaults to 0.875 or uses the request's boundary_ratio. Resolve the boundary in one helper (engine -> request -> 0.875) shared by forward() and prepare_cache_refresh(), which stores it as request-scoped state so the startup boundary_ratio is not overwritten. Signed-off-by: Chendi Xue <chendi.xue@intel.com>
|
@hsliuustc0106 thanks, fixed in c7a6424.
Tests (
B60 (XPU): I ran every non-e2e test file that imports the changed modules (37 files). Result: 1590 passed, 80 failed. The same 80 tests also fail on 3fee614 (1580 passed there), e.g. MammothModa2 model path missing on the node and |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Reviewed current head 7ae90faaa6c4f5c9b3249d468c737c6bc77f8ebb.
The request-specific scheduler/flow-shift ordering, effective-boundary precedence and successive-request isolation address the earlier review concerns. I also inspected low-noise-only target selection and cleanup of both experts, the runner integration, and the corresponding regression tests. The named scheduler/boundary regressions and author-provided before/after serving logs support the correction; I did not independently execute hardware tests.
Current-head GitHub Actions and docs checks pass. The ready-triggered CUDA pipeline build #17778 reports success for this exact SHA; its entry/bootstrap and ready routing were checked against the trusted repository configuration. This is pipeline-level CI evidence, not a claim that I inspected individual job logs or independently validated XPU/AMD behavior. AMD and Intel CI report failures whose causes I have not verified; CUDA does not establish their correctness.
No remaining actionable correctness finding in the reviewed changes. Final squash-and-merge remains with the maintainer.
|
fix conflicts |
Resolve conflicts with vllm-project#7335, which refreshes the Wan2.2 Cache-DiT contexts from forward() with the resolved schedule. That supersedes this branch's runner-side prepare_cache_refresh / request boundary plumbing, so those are dropped in favour of main. Kept on top of main: the single-expert (low-noise-only transformer_2) cache target and refresh getter, the no-transformer error, backend target discovery, and the shared resolve_wan_boundary_ratio helper. Signed-off-by: Chendi Xue <chendi.xue@intel.com>
|
@hsliuustc0106 conflicts fixed in bd20d0c (merge of main). #7335 landed on main with the same flow-shift / effective-boundary fix, done from
The title and description are updated. The diff is smaller than the one you approved, so please take another look. CI: the AMD failure was B60 at bd20d0c: |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Reviewed current head bd20d0cad1745380aa2f05134cf0fce8a952f066 after the main merge and narrower scope.
Revalidated the remaining six-file diff: low-noise-only cache enable/refresh targets transformer_2, backend target discovery also covers cleanup, and the shared boundary resolver preserves engine override → request → default precedence. The request-specific schedule refresh now comes from merged #7335; the old runner hook is no longer duplicated. The earlier flow-shift and low-noise-only review concerns are addressed in this implementation and the named regression tests.
Evidence: inspected the production paths and test_wan22_custom_enabler_low_noise_expert_only, missing-expert, boundary-precedence and shifted-schedule tests; reused the earlier Wan request-level evidence and read the author's current-head 27-test receipt. I did not execute tests or benchmarks locally.
Current-head GitHub checks and CUDA ready pipeline #17845 pass. Intel pipeline #10608 failed; its job-level cause is unavailable here and is not classified as infrastructure or treated as a passing platform test. This review makes no new Intel correctness or performance claim.
|
Re-checked after #7335 landed: this PR is still dirty / conflicted against Please rebase onto current |
Purpose
Fix Cache-DiT on Wan2.2 when only one expert is loaded, and make the backend discover its cache targets instead of assuming
pipeline.transformer.The flow-shift / effective-boundary part of the expert split that this PR originally carried is now on main via #7335 (refresh from
forward()with the resolved schedule), so after merging main this PR keeps only what #7335 does not cover.Changes
cachedit/model_specific.pyenable_cache_for_wan22: in single-expert mode, cache and refresh whichever expert is actually loaded. With a Wan2.2 MoE checkpoint andboundary_ratio=1.0,Wan22Pipelineloads onlytransformer_2andpipeline.transformerisNone, so the old code crashed onpipeline.transformer.blocksand its refresh readpipeline.transformer. Both the runner refresh and [Bugfix][Diffusion] Make Cache-DiT effective when serving Wan2.2 T2V/I2V/S2V without num_inference_steps #7335's per-schedule refresh now use a getter for the active expert. Raise a clearValueErrorwhen neither expert is loaded.cachedit/backend.py: an enabler that only returns a refresh func now gets its cache targets discovered from the pipeline's DiT modules (filtered byBlockAdapter.is_cached). Before, the target waspipeline.transformer, which isNonefor the low-noise-only run and also left outtransformer_2in the dual-expert case, so the second expert's hooks stayed installed afterdisable().wan2_2:resolve_wan_boundary_ratio()(engine override -> request -> 0.875) replaces the three copies in T2V / I2V / VACEforward()and the batch-settings check.Test Plan
New tests in
tests/diffusion/cache/test_cache_dit.py:test_wan22_custom_enabler_low_noise_expert_only(enable, runner refresh and per-schedule refresh all targettransformer_2),test_wan22_custom_enabler_without_any_transformer_raises,test_resolve_wan_boundary_ratio.Test Result
On Intel Arc Pro B70 (XPU), head
bd20d0cad:tests/diffusion/cache/test_cache_dit.py: 27 passed.origin/main(98c204144) on this node (offline model downloads / environment), none in the touched code.(anything written below this line will be removed by GitHub Actions)