[tests] refactor o and p series pipeline tests - #14646
Merged
Merged
Conversation
Member
Author
|
/diffusers-bot pytest tests/pipelines/pag |
Contributor
|
|
sayakpaul
commented
Aug 29, 2026
| - `torch.nn.MultiheadAttention` is the common instance: it passes `self.out_proj.weight` straight to `torch.nn.functional.multi_head_attention_forward` instead of calling `self.out_proj`, so the hook on `out_proj` never fires. `SiglipVisionModel`'s attention pooling head wraps one — see `tests/pipelines/hunyuan_video/test_hunyuan_video_framepack.py`, whose `image_encoder` is excluded for this reason. | ||
| - `HunyuanDiTAttentionPool` (`src/diffusers/models/embeddings.py`) shows the same failure without an MHA module: a plain `nn.Module` that hands its `q_proj` / `k_proj` / `v_proj` / `c_proj` weights to `torch.nn.functional.multi_head_attention_forward`, so all four projections stay offloaded rather than just one. `HunyuanDiT2DModel` opts out of group offloading entirely with `_supports_group_offloading = False`. | ||
| - Before adding a skip or an exclusion, confirm the failure still reproduces — several existing skips are stale, having outlived the upstream cause. | ||
| - **A migration that surfaces a `src/` gap marks the test `xfail`, it does not patch the pipeline.** Give the marker a module-level name and a `reason` naming the exact gap (`PNDM_*` in `tests/pipelines/pndm/test_pndm.py` is the worked example), and prefer `strict=True` so the marker reports XPASS — and gets deleted — the day the pipeline is fixed. Use `strict=False` only when one mark covers a group whose members do not all fail. Marking a whole test class keeps the mixin's own marks (`@is_memory`, `@require_accelerator`) intact; overriding individual inherited tests drops the decorators they were declared with, so re-declare those too. |
Member
Author
There was a problem hiding this comment.
I think it's better to xfail things where we need core changes while doing test PRs. xfail is better than skipping the tests.
Collaborator
There was a problem hiding this comment.
Fine to add. But could we do it in a separate PR please.
Member
Author
There was a problem hiding this comment.
Didn't add src level changes in this PR. Not sure if that is what you meant.
sayakpaul
commented
Aug 29, 2026
| AutoencoderKL, | ||
| DDIMScheduler, | ||
| MotionAdapter, | ||
| StableDiffusionPipeline, |
Member
Author
There was a problem hiding this comment.
Just import level changes in the animdatediff series because we moved the from_pipe tests in a dedicated mixin.
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Member
Author
|
/diffusers-bot pytest tests/pipelines/pag |
Contributor
|
❌ |
Member
Author
|
/diffusers-bot pytest tests/pipelines/pag |
Contributor
|
✅ |
DN6
approved these changes
Aug 31, 2026
| - `torch.nn.MultiheadAttention` is the common instance: it passes `self.out_proj.weight` straight to `torch.nn.functional.multi_head_attention_forward` instead of calling `self.out_proj`, so the hook on `out_proj` never fires. `SiglipVisionModel`'s attention pooling head wraps one — see `tests/pipelines/hunyuan_video/test_hunyuan_video_framepack.py`, whose `image_encoder` is excluded for this reason. | ||
| - `HunyuanDiTAttentionPool` (`src/diffusers/models/embeddings.py`) shows the same failure without an MHA module: a plain `nn.Module` that hands its `q_proj` / `k_proj` / `v_proj` / `c_proj` weights to `torch.nn.functional.multi_head_attention_forward`, so all four projections stay offloaded rather than just one. `HunyuanDiT2DModel` opts out of group offloading entirely with `_supports_group_offloading = False`. | ||
| - Before adding a skip or an exclusion, confirm the failure still reproduces — several existing skips are stale, having outlived the upstream cause. | ||
| - **A migration that surfaces a `src/` gap marks the test `xfail`, it does not patch the pipeline.** Give the marker a module-level name and a `reason` naming the exact gap (`PNDM_*` in `tests/pipelines/pndm/test_pndm.py` is the worked example), and prefer `strict=True` so the marker reports XPASS — and gets deleted — the day the pipeline is fixed. Use `strict=False` only when one mark covers a group whose members do not all fail. Marking a whole test class keeps the mixin's own marks (`@is_memory`, `@require_accelerator`) intact; overriding individual inherited tests drops the decorators they were declared with, so re-declare those too. |
Collaborator
There was a problem hiding this comment.
Fine to add. But could we do it in a separate PR please.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.