Skip to content

Fix _is_cached class/instance mismatch for multi-transformer pipelines - #1102

Merged
DefTruth merged 2 commits into
vipshop:mainfrom
jiangyon-amd:fix/is-cached-class-vs-instance
Sep 2, 2026
Merged

DefTruth merged 2 commits into
vipshop:mainfrom
jiangyon-amd:fix/is-cached-class-vs-instance

Conversation

@jiangyon-amd

Copy link
Copy Markdown
Contributor

Summary

CachedAdapter.create_context() sets _is_cached on block_adapter.pipe.__class__,
but sets _context_manager on the instance:

block_adapter.pipe._context_manager = context_manager       # instance level
...
block_adapter.pipe.__class__._is_cached = True               # class level

For pipelines with more than one transformer that don't have a real pipe object
(a BlockAdapter(transformer=..., ...) call per transformer, as one would do for a
dual/multi-DiT model), each transformer gets wrapped in its own fresh
FakeDiffusionPipeline() instance -- but all of those instances share the same
class. Once the first transformer's setup flips the class-level _is_cached flag,
BlockAdapter.is_cached() -- ultimately a plain getattr(pipe, "_is_cached", False),
which can't distinguish an instance attribute from an inherited class attribute --
reports every sibling instance as already cached too, even though it never got its
own _context_manager. create_context() then returns early, and the caller's
assert hasattr(block_adapter.pipe, "_context_manager") fails for the second
transformer.

Fix: only take the early-return/skip path when this pipe instance actually already
carries its own _context_manager.

Repro / verification

Added tests/api/test_multi_transformer_is_cached.py, which calls enable_cache on
two independent transformer-only BlockAdapters (mirroring what a dual-DiT model
does) and asserts the second one gets its own context manager.

  • Against current main: fails with
    AssertionError
    ...cache_adapter.py:439: in collect_unified_blocks
        assert hasattr(block_adapter.pipe, "_context_manager")
    
    preceded by the log line Pipeline has been already cached, skip creating cache context again. for the second transformer -- confirming the class-level flag leaked from the first.
  • With this fix applied: passes, and the second transformer has its own distinct _context_manager.

Notes

Found while integrating a dual-DiT pipeline (video + audio branches, each wrapped
via the transformer-only API) with cache-dit.

CachedAdapter.create_context() sets `_is_cached` on `block_adapter.pipe.__class__`
while `_context_manager` is set on the instance. Pipelines that wrap more than one
transformer without a real pipe (e.g. a dual-DiT model calling enable_cache once per
transformer) get a fresh FakeDiffusionPipeline() instance per transformer, but every
instance shares the same class. Once the first instance's cache setup flips the
class-level `_is_cached` flag, `BlockAdapter.is_cached()` (a plain getattr, so it
can't distinguish instance vs. inherited class attributes) reports every sibling
instance as already cached too -- even though they never got their own
`_context_manager` -- so create_context() returns early and the caller's
`assert hasattr(block_adapter.pipe, "_context_manager")` fails.

Only skip context creation when this specific pipe instance actually already has a
context manager.

Added a regression test (tests/api/test_multi_transformer_is_cached.py) that enables
cache on two independent transformer-only BlockAdapters and verifies the second one
gets its own context manager instead of crashing.
@DefTruth
DefTruth requested review from DefTruth and a lite review from Copilot August 21, 2026 10:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a caching-state leak that occurs when multiple transformer-only BlockAdapters each create their own FakeDiffusionPipeline() instance but share the same pipeline class, causing class-level _is_cached to incorrectly short-circuit cache setup for sibling instances.

Changes:

  • Tighten CachedAdapter.create_context()’s early-return condition so it only skips when the current pipe instance already has a context manager.
  • Add a regression test covering two independent transformer-only adapters to ensure both get distinct context managers.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/api/test_multi_transformer_is_cached.py Adds regression coverage for multi-transformer transformer-only caching with shared FakeDiffusionPipeline class.
src/cache_dit/caching/cache_adapters/cache_adapter.py Prevents premature early-return in create_context() when _is_cached is inherited from the pipe class but the instance lacks _context_manager.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/cache_dit/caching/cache_adapters/cache_adapter.py Outdated
@DefTruth

Copy link
Copy Markdown
Member

The pre-commit ci failed, please init and run pre-commit

Co-authored-by: Copilot Autofix powered by AI <[email protected]>
@DefTruth
DefTruth merged commit 0a59673 into vipshop:main Sep 2, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants