fix(mag_cache): correct per-transformer step count for dual-transformer pipelines#14027
fix(mag_cache): correct per-transformer step count for dual-transformer pipelines#14027lcheng321 wants to merge 2 commits into
Conversation
|
@sayakpaul I saw your name in the git blame history for mag_cache.py and noticed you are also a maintainer on this file, would you have time to review this PR. It fixes the per-transformer step accounting bug reported in #14025, where num_inference_steps was set to the full pipeline count instead of the actual per-transformer execution count. I verified the fix locally with a CPU only repro and 20 passing unit tests covering asymmetric splits and edge cases. Happy to answer any questions about the approach. |
|
Cc: @AlanPonnachan |
|
@AlanPonnachan any chance you could take a look when free? |
|
Sorry for the late reply. I'll review it today |
| def update_mag_cache_num_steps(module: torch.nn.Module, num_steps: int) -> None: | ||
| config: MagCacheConfig = getattr(module, "_mag_cache_config", None) | ||
| if config is None: | ||
| return | ||
| original_ratios = getattr(config, "_original_mag_ratios", config.mag_ratios) | ||
| config.num_inference_steps = num_steps | ||
| if original_ratios is not None: | ||
| config.mag_ratios = nearest_interp(original_ratios, num_steps) |
There was a problem hiding this comment.
It's recommended to keep MagCacheConfig fixed during pipeline execution rather than modifying it dynamically. If a user passes the same MagCacheConfig object to both transformers, or reuses the pipeline for a new request with a different step count, mutating config.num_inference_steps and config.mag_ratios in place will corrupt the configuration for subsequent runs?
There was a problem hiding this comment.
Fixed in 6d19597.
Config was stored by reference and read live every step. Same config on both transformers under an asymmetric split meant the second update_mag_cache_num_steps call clobbered the first.
Now config is never mutated. Each transformer reads its own step count from a plain attribute on the root module (_mag_cache_expected_steps), falling back to config.num_inference_steps when unset. update_mag_cache_num_steps is gone, nothing left to mutate.
def test_shared_config_object_untouched_by_apply(self):
shared = _make_config(self.RATIOS, self.TOTAL)
t1, t2 = _make_tiny_transformer(), _make_tiny_transformer()
apply_mag_cache(t1, shared)
apply_mag_cache(t2, shared)
assert shared.num_inference_steps == self.TOTAL
assert len(shared.mag_ratios) == self.TOTALAlso covered asymmetric splits (3,2), (7,3), (1,9) on a shared config. 19/19 tests pass.
| if self.transformer is not None: | ||
| update_mag_cache_num_steps(self.transformer, n_steps_t1) | ||
| if self.transformer_2 is not None: | ||
| update_mag_cache_num_steps(self.transformer_2, n_steps_t2) |
There was a problem hiding this comment.
we should avoid tightly coupling the core pipelines to specific hook implementations (like importing update_mag_cache_num_steps directly). I think a better solution would be
can we just have the pipeline generically broadcast the expected step counts to the modules? For example:
n_steps_t1 = sum(1 for t in timesteps if t >= boundary_timestep) if self.transformer is not None: self.transformer._expected_steps = n_steps_t1 if self.transformer_2 is not None: self.transformer_2._expected_steps = len(timesteps) - n_steps_t1
Then, MagCacheBlockHook can simply read getattr(module, "_expected_steps", default) during initialization. This keeps the pipeline cleanly decoupled from hook internals."
There was a problem hiding this comment.
Fixed in 6d19597, close to your suggestion.
Pipeline just sets a plain attribute now:
if self.transformer is not None:
self.transformer._mag_cache_expected_steps = n_steps_t1
if self.transformer_2 is not None:
self.transformer_2._mag_cache_expected_steps = n_steps_t2One tweak: reading it during initialize_hook doesn't work, that runs before the pipeline computes boundary_timestep, so the attribute isn't set yet. Hooks also live on individual blocks, not the root transformer. So hooks hold a root_module reference and read the attribute lazily at forward/reset time instead.
…er step count via plain attribute MagCacheConfig is no longer mutated at runtime. Instead, pipelines broadcast each transformer's actual step count via a plain _mag_cache_expected_steps attribute on the root module, which MagCacheHeadHook/MagCacheBlockHook read live through _get_effective_num_steps/_get_effective_mag_ratios (falling back to config.num_inference_steps when unset, so single-transformer pipelines are unaffected). Addresses both review comments on huggingface#14027: - Sharing one MagCacheConfig across transformer/transformer_2 under an asymmetric step split can no longer corrupt it, since it's never mutated. - Pipelines no longer import hook internals (update_mag_cache_num_steps is removed); they only set a plain attribute, keeping pipeline code decoupled from hook implementation details.
Fixes #14025
Root cause
MagCacheConfigwas applied to bothtransformerandtransformer_2withnum_inference_stepsset to the full pipeline step count. In dual transformer pipelines such as Wan 2.2,transformeronly runs the high noise stage andtransformer_2only runs the low noise stage. Each of them executes a subset of the total steps, never the full count.This causes three downstream problems:
_advance_step: the checkstate.step_index >= config.num_inference_stepsnever fires for either transformer, since neither ever reaches the full step count. State never resets and calibration never completes.int(retention_ratio * num_inference_steps)is computed against the full step count instead of the actual per transformer step count, so it protects more steps from skipping than intended.mag_ratiosindexing: sincemag_ratiosis sized to the full step count,step_indexno longer maps to the correct entry, so each transformer reads ratios that do not correspond to its own denoising timestep.First attempt and why it changed
The initial fix added
update_mag_cache_num_steps(module, actual_steps), which mutatedconfig.num_inference_stepsandconfig.mag_ratiosin place on the attachedMagCacheConfig.Review feedback identified two problems with that approach:
MagCacheConfigobject is passed to bothtransformerandtransformer_2, calling this function for one transformer corrupts the config for the other, since they share the same object.importa hook internal function directly, coupling pipeline code to hook implementation details.Fix
MagCacheConfigis now never mutated after construction. Instead:transformer._mag_cache_expected_steps = n_steps, no hook internals imported.apply_mag_cachetime) and read_mag_cache_expected_stepsoff it live, at forward and reset time, falling back toconfig.num_inference_stepsif the attribute was never set.mag_ratiosare computed on demand from_original_mag_ratiosat the effective step count, never written back toconfig.mag_ratios.This means a shared config object across both transformers can no longer be corrupted, since nothing ever writes to it after construction.
Files changed:
src/diffusers/hooks/mag_cache.py_get_effective_num_stepsand_get_effective_mag_ratios, both pure functions that read from a root module attribute and a config, no mutation. Hooks now take aroot_modulereference. Removeupdate_mag_cache_num_steps.src/diffusers/hooks/__init__.pyupdate_mag_cache_num_steps.src/diffusers/pipelines/wan/pipeline_wan.pytransformer._mag_cache_expected_steps/transformer_2._mag_cache_expected_stepsinstead of calling into hook internals.src/diffusers/pipelines/wan/pipeline_wan_i2v.pysrc/diffusers/pipelines/wan/pipeline_wan_vace.pyAll pipeline changes are no-ops when MagCache is not applied.
Evidence
Run locally, CPU only, no GPU or model weights required. Uses tiny
WanTransformerBlockinstances (dim=32) just to satisfy the hook registry.1. The bug this PR fixes: shared config corruption under asymmetric split
Before this PR, the mutating approach corrupted a config object shared across two transformers. Reproduced directly against a plain
MagCacheConfig, no pipeline needed:Output:
after setting for transformer: 7
after setting for transformer_2: 3
what transformer now reads: 3
transformerwas supposed to run 7 steps but the shared config now reports 3, becausetransformer_2's update overwrote it. This is the exact corruption the reviewer flagged.2. After the fix, the same shared config is never touched
Output:
t1 effective steps: 7
t2 effective steps: 3
shared.num_inference_steps, unchanged: 10
Each transformer sees its own correct step count and the shared config object is never written to.
3. Full unit test suite
$ python -m pytest tests/hooks/test_mag_cache_wan22_steps.py -v
19 passed in 0.30s
Covers, beyond the case above:
mag_ratiosre-interpolated per transformer without mutatingconfig.mag_ratios.(3,2),(7,3),(1,9)on a single shared config object.config.num_inference_stepsunchanged when_mag_cache_expected_stepswas never set, confirming no behavior change there.4. Double interpolation is still avoided
Unchanged from the original fix, still verified by the test suite:
mag_ratiosare always re-computed from_original_mag_ratios, never from an already interpolatedmag_ratios, so calling this multiple times at different step counts does not compound rounding error.Scope note
This PR only fixes the step accounting and config mutation issues described above. The separately reported quality degradation on heavily step distilled models (50 step calibration collapsed into a 4 step schedule) is a distinct issue tracked in #14025 and not addressed here.