-
Notifications
You must be signed in to change notification settings - Fork 5.9k
[Feature] Implement tiled VAE encoding/decoding for Wan model. #11414
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
base: main
Are you sure you want to change the base?
Conversation
Hi @a-r-r-o-w @yiyixuxu, could you please help reviewing this patch? Thanks! |
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 hope the inline comment makes sense to you~
@@ -677,42 +677,7 @@ def __init__( | |||
attn_scales: List[float] = [], | |||
temperal_downsample: List[bool] = [False, True, True], | |||
dropout: float = 0.0, | |||
latents_mean: List[float] = [ |
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.
These function parameters are not being used. They have been removed in this patch but can be added back at any time if needed.
batch_size = 2 | ||
num_frames = 9 | ||
num_channels = 3 | ||
sizes = (640, 480) |
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.
Add another input because the (16, 16) tensor is too small for tiling operations.
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.
Let's try to reduce the size as much as possible because these tests should not cause unexpected slowdowns in the CI. While enabling tiling, you can set different tile width/height and stride, than the default 256 and 192.
(128, 128) would be good, with tile size being 96, 96
and stride being 64, 64
, or something similar/lower.
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.
That's a good idea! Done.
|
||
self.assertLess( | ||
(output_without_tiling.detach().cpu().numpy() - output_with_tiling.detach().cpu().numpy()).max(), | ||
0.5, |
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.
On my machine, this value is approximately 0.404, and IIRC the average absolute value of these arrays is less than 0.01, which makes me confident that the implementation is correct at some point.
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 for working on this @c8ef! Most of the changes look good to me. Will have to verify the visual quality once before we merge -- I can look into that after some of the reviews here are addressed 🤗
@@ -677,42 +677,7 @@ def __init__( | |||
attn_scales: List[float] = [], | |||
temperal_downsample: List[bool] = [False, True, True], | |||
dropout: float = 0.0, | |||
latents_mean: List[float] = [ |
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.
Indeed it might not be used in the diffusers codebase at the moment, but it is being used downstream in a few repositories (example). Removing this will break downstream so we should keep this anyway.
What you could instead do here to reduce LOC is wrap these two parameters in a non-format block and condense the list into a single line:
# fmt: off
latents_mean: List[float] = ...
latents_std: List[float] = ...
# fmt: on
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 add the parameter back~
2.8251, | ||
1.9160, | ||
], | ||
spatial_compression_ratio: int = 8, |
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.
Let's not add new parameter now because it will lead to unnecessary config warning
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.
Done.
@@ -730,6 +695,58 @@ def __init__( | |||
base_dim, z_dim, dim_mult, num_res_blocks, attn_scales, self.temperal_upsample, dropout | |||
) | |||
|
|||
self.spatial_compression_ratio = spatial_compression_ratio |
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.
Instead, let's set this attribute based on the init parameters. The same logic as used in the pipeline can be applied here.
Let's also add temporal_compression_ratio as 2 raised to power of (the number of true values in temporal downsample)
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.
Done.
# When decoding spatially large video latents, the memory requirement is very high. By breaking the video latent | ||
# frames spatially into smaller tiles and performing multiple forward passes for decoding, and then blending the | ||
# intermediate tiles together, the memory requirement can be lowered. | ||
self.use_tiling = False |
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.
Let's also add use_slicing
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.
Done.
batch_size = 2 | ||
num_frames = 9 | ||
num_channels = 3 | ||
sizes = (640, 480) |
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.
Let's try to reduce the size as much as possible because these tests should not cause unexpected slowdowns in the CI. While enabling tiling, you can set different tile width/height and stride, than the default 256 and 192.
(128, 128) would be good, with tile size being 96, 96
and stride being 64, 64
, or something similar/lower.
@@ -785,7 +802,11 @@ def encode( | |||
The latent representations of the encoded videos. If `return_dict` is True, a | |||
[`~models.autoencoder_kl.AutoencoderKLOutput`] is returned, otherwise a plain `tuple` is returned. | |||
""" | |||
h = self._encode(x) | |||
_, _, _, height, width = x.shape |
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.
Let's make sure to support use_slicing here as well
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.
Done. Test case added.
What does this PR do?
Implement tiled VAE encoding/decoding for Wan model.
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.