Add SeaCache support for Cosmos3 pipelines - #14663
Conversation
|
Claude-Opus 4.8 Self-review:
|
| location | status | reason |
|---|---|---|
sea_cache.py:555 _prepare_cosmos3_vision_metadata + first_block branch in _build_indicator |
Likely-dead under default | only runs when indicator_source="first_block"; default is raw_vision_latents |
SeaCacheConfig.metadata_callback / raw_vision_callback |
Likely-dead | no caller passes them; Cosmos3 uses native adapters |
_helpers.py hidden_states_norm_module_name |
Used only by first_block | tied to the non-default indicator |
Process reminders (AI-contribution guidelines)
- Uncommitted/untracked files in your worktree are NOT in the pushed diff but shouldn't get added:
check_cosmos3_transfer_workflow.py,run_cosmos3_modular_transfer.sh,assets/,examples/cosmos3/inference_cosmos3_modular_distilled.py, and an unstagedencoders.pyedit. The dev scripts/assets are ephemeral (per review-rules "Ephemeral context") — keep them out of the PR. - Add a coordination link (issue where a maintainer acknowledged this work) to the PR description.
- Add test commands + pasted results to the PR. Suggested:
pytest tests/hooks/test_sea_cache.py tests/models/transformers/test_models_transformer_cosmos3.py tests/pipelines/cosmos/test_cosmos3.py tests/modular_pipelines/cosmos/ -q.
Verdict: NEEDS CHANGES
Primary asks: split the three features (#1), trim defensive/unused paths (#2, #3). Tests are strong — real Cosmos3 model at tiny config, per-context cache isolation, fail-open, and dtype coverage all exercised, consistent with testing.md
sayakpaul
left a comment
There was a problem hiding this comment.
Thanks! The design looks mostly good. My major comment is that we're adding utilities to enable caching at the pipeline-level which is not something we typically do in the library.
|
Hi @yzhautouskay, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. Please note that PRs without a linked issue are likely to be automatically closed 10 days after this notice. Once the PR links an issue (or gets the |
|
@yiyixuxu @sayakpaul Thanks for the review! I addressed moving caching to the transformer level instead of pipeline, and refactored tests Few thing I will address next:
|
sayakpaul
left a comment
There was a problem hiding this comment.
Thanks for the updates! This looks much better!
| current_step_callback=lambda: pipe.current_step_index, | ||
| current_sigma_callback=lambda: pipe.current_sigma, | ||
| num_inference_steps_callback=lambda: pipe.num_timesteps, |
There was a problem hiding this comment.
Would it make sense to provide actual values here? Or maybe even just specify what pipe is supposed to be?
There was a problem hiding this comment.
These must remain callbacks because the step index and sigma change during every denoising iteration, while num_timesteps is determined per pipeline call. I updated the example to define pipe explicitly
| This model-level API works with [`Cosmos3OmniPipeline`], [`Cosmos3OmniModularPipeline`], and | ||
| [`Cosmos3DistilledModularPipeline`]. SeaCache is an approximate optimization and may change generated outputs. Call | ||
| `pipe.transformer.disable_cache()` when you need every denoising step to execute the full transformer. |
There was a problem hiding this comment.
Nice, thanks for the note! From a quick skim of the paper, it doesn't look like it needs to be Cosmos3 specific no?
| raise ValueError(f"Cache config {type(config)} is not supported.") | ||
|
|
||
| self._cache_config = config | ||
| HookRegistry.check_if_exists_or_initialize(self)._child_registries_cache = None |
There was a problem hiding this comment.
This fix is only needed for the specific case where a baseline inference runs first without caching enabled, while still entering cache_context(), and caching is enabled on the same transformer instance afterward
Before the fix: Cosmos3OmniPipeline.__call__() → CacheMixin.cache_context() → HookRegistry._set_context() → _get_child_registries() scans the model and saves [], because no cache hooks exist yet. After enable_cache() installs child hooks, the next context call reuses that old empty list, so "cond"/"uncond" does not reach them.
With the fix:* enable_cache() sets _child_registries_cache = None after installing hooks. The next context call rescans the model, finds the new child registries, and propagates the context correctly
|
/diffusers-bot pytest tests/models -k "seacache" |
|
✅ |
What does this PR do?
0.25, and at most2consecutive cached steps. This provides 2-3x speedup depending on inference params with on-par quality.Before submitting
self-reviewskill on the diff?documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.