-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[CI] Refactor Wan Model Tests #13082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
yiyixuxu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really nice thanks!
should we start to add a guide for contributor some where, maybe https://huggingface.co/docs/diffusers/main/en/conceptual/contribution
dg845
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I see there are two Wan model related failures from the CI:
tests/models/transformers/test_models_transformer_wan_animate.py::TestWanAnimateTransformer3DAttention::test_fuse_unfuse_qkv_projectionstests/models/transformers/test_models_transformer_wan_vace.py::TestWanVACETransformer3DAttention::test_fuse_unfuse_qkv_projections
If I try to run the new Wan tests locally, for example with
pytest tests/models/transformers/test_models_transformer_wan.pyI get some more test failures:
tests/models/transformers/test_models_transformer_wan.py::TestWanTransformer3Dtest_keep_in_fp32_modulestest_from_save_pretrained_dtype_inference[fp16,bf16]
tests/models/transformers/test_models_transformer_wan.py::TestWanTransformer3DGGUFtest_gguf_quantization_inferencetest_gguf_keep_modules_in_fp32test_gguf_quantization_dtype_assignmenttest_gguf_quantization_lora_inferencetest_gguf_dequantizetest_gguf_quantized_layers
tests/models/transformers/test_models_transformer_wan.py::TestWanTransformer3DGGUFCompiletest_gguf_torch_compiletest_gguf_torch_compile_with_group_offload
Are these test failures expected?
|
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. |
|
Thanks for flagging @dg845. I've fixed the test issues. There are some GGUF related fixes that should probably go in a different PR (will handle that later) |
dg845
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
sayakpaul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I left some comments!
| self.inner_dim = dim_head * heads | ||
| self.heads = heads | ||
| self.cross_attention_head_dim = cross_attention_dim_head | ||
| self.cross_attention_dim_head = cross_attention_dim_head |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Would be nice if you could explain these changes? Were these flagged by the newly written test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just to keep the naming convention consistent
| # Get model dtype from first parameter | ||
| model_dtype = next(model_quantized.parameters()).dtype | ||
|
|
||
| inputs = self.get_dummy_inputs() | ||
| # Cast inputs to model dtype | ||
| inputs = { | ||
| k: v.to(model_dtype) if isinstance(v, torch.Tensor) and v.is_floating_point() else v | ||
| for k, v in inputs.items() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting here is brittle because it's based on model_dtype which we get from model_dtype = next(model_quantized.parameters()).dtype. This can lead to different dtypes across different models and different quantization schemes. e.g With Flux + GGUF the test passes because the parameter dtype is the same the input dtype (bfloat16). However with Wan it fails because the parameter dtype is int8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. But does it affect the existing Flux tests?
Casting here is brittle because it's based on model_dtype which we get from model_dtype = next(model_quantized.parameters()).dtype
I wonder if using .dtype on a model subclassed from ModelMixin would alleviate this problem because dtype implementation is quite elaborate:
| def get_parameter_dtype(parameter: torch.nn.Module) -> torch.dtype: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a torch_dtype property to the quantization tests and we cast the inputs directly in get_dummy_inputs. Think it's more clear this way
Flux TorchAO and BnB tests will fail with this change, but I'll update the Flux2 PR to include fixes to address the change in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Thanks!
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import unittest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing the changes under tests/models/transformers/ were all auto-generated?
| class TestWanVACETransformer3DCompile(WanVACETransformer3DTesterConfig, TorchCompileTesterMixin): | ||
| """Torch compile tests for Wan VACE Transformer 3D.""" | ||
|
|
||
| def test_torch_compile_repeated_blocks(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can further simplify this test by letting users pass a recompile_limit. I will open a PR.
What does this PR do?
Update Wan tests with new format
Fixes # (issue)
Before submitting
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.