[Refactor] How attention is set in 3D UNet blocks #6893
Conversation
|
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. |
| # If `num_attention_heads` is not defined (which is the case for most models) | ||
| # it will default to `attention_head_dim`. This looks weird upon first reading it and it is. | ||
| # The reason for this behavior is to correct for incorrectly named variables that were introduced | ||
| # when this library was created. The incorrect naming was only discovered much later in https://github.com/huggingface/diffusers/issues/2011#issuecomment-1547958131 | ||
| # Changing `attention_head_dim` to `num_attention_heads` for 40,000+ configurations is too backwards breaking | ||
| # which is why we correct for the naming here. | ||
| num_attention_heads = num_attention_heads or attention_head_dim |
There was a problem hiding this comment.
Also remove or update the comment above, assuming num_attention_heads is replicated in the hub configs. Do we know how many models like https://huggingface.co/ali-vilab/i2vgen-xl/blob/6c4e9e70bdcd36eb59d98d2b583adea0813ea8de/unet/config.json#L21 do we need to update?
Will we need to live forever with duplicated property names in the hub?
There was a problem hiding this comment.
just this one model and it was just out a few days ago
IMO it's an edge case we don't mind breaking - it will only affect people who want to use the local copy, no?
| num_attention_heads, | ||
| out_channels // num_attention_heads, |
There was a problem hiding this comment.
CrossAttnDownBlock3D is part of our public API and this is a breaking change, no?
There was a problem hiding this comment.
Sorry, why isn't it backwards compatible? None of the args in the class init are being changed right?
The 3D blocks and the 3D UNet are only used with the Text to Video Synth and I2VGenXL model in the library.
There was a problem hiding this comment.
Same with get_up_block and get_down_block. We're only changing the number being passed in to the num_attention_heads argument.
There was a problem hiding this comment.
change the meaning of an argument is breaking, no?
for example,
CrossAttnDownBlock3D(... num_attention_heads = 64) is currently expected to create attentions with head_dim=64; with this code change, it will create attentions with 64 heads instead
There was a problem hiding this comment.
The 3D blocks and the 3D UNet are only used with the Text to Video Synth and I2VGenXL model in the library.
yes but it is our public API and we have to assume it's been used outside of the library
There was a problem hiding this comment.
I think this might be a relatively safe change. Searching Github public repos for an import of these blocks from the public API doesn't return any results. I actually don't think you can import it directly from diffusers
https://github.com/search?q=%22from+diffusers.models+import+CrossAttnDownBlock3D%22+language:Python+&type=code
https://github.com/search?q=%22from+diffusers+import+CrossAttnUpBlock3D%22+language:Python+&type=code
https://github.com/search?q=%22from+diffusers+import+UNetMidBlock3DCrossAttn%22+language:Python+&type=code
It looks like more often than not people redefine the blocks themselves
https://github.com/search?q=%22CrossAttnDownBlock3D%22+language:Python+&type=code&p=5
There was a problem hiding this comment.
Even getting specific with the imports doesn't return a lot of results
https://github.com/search?q=%22from+diffusers.models.unet_3d_blocks+import+CrossAttnDownBlock3D%22+language:Python+&type=code
There was a problem hiding this comment.
cc @pcuenca and @patrickvonplaten here.
I would like to hear your thoughts about when we can make breaking changes (other than v1.0.0).
Personally, I think we should only make breaking changes when we don't have another choice, or we know super confidently it is an edge case (e.g., if we just added these blocks yesterday, I would think it's ok to break here).
I think in this case,
(1) we do not have to make these changes: we want to make these changes to make our code more readable and easier for contributors to contribute, but it is not a must and this is not the only way to go
(2) we don't really have a way to find out about its usage outside github
also, I think a break change is somewhat more acceptable if we are able to throw an error. In this case, it will just be breaking silently so IMO it is worse
But I'm curious about your thoughts on this and I'm cool with it if you all feel strongly about making this change here :)
There was a problem hiding this comment.
python -c "from diffusers import CrossAttnDownBlock3D"doesn't work, so strictly speaking CrossAttnDownBlock3D is not considered part of the public API. Also I don't think it's used that much so IMO it's ok to change it here (while making sure though that this might lead to breaking changes depending on how CrossAttnDownBlock3D is imported.
| @@ -132,13 +132,19 @@ def __init__( | |||
| "At the moment it is not possible to define the number of attention heads via `num_attention_heads` because of a naming issue as described in https://github.com/huggingface/diffusers/issues/2011#issuecomment-1547958131. Passing `num_attention_heads` will only be supported in diffusers v0.19." | |||
| ) | |||
|
|
|||
| if isinstance(attention_head_dim, int): | |||
| num_attention_heads = [out_channels // attention_head_dim for out_channels in block_out_channels] | |||
There was a problem hiding this comment.
That's a good idea.
Are we sure though that this is always correct? Does out_channels always represent the hidden_dim of the attention layer?
There was a problem hiding this comment.
For the 3D UNets this is safe. There are a limited number of blocks used with this model CrossAttnDownBlock3D, CrossAttnUpBlock3DandUNetMidBlock3DCrossAttnand they all configurenum_attention_heads` based on the out_channels.
| @@ -132,13 +132,19 @@ def __init__( | |||
| "At the moment it is not possible to define the number of attention heads via `num_attention_heads` because of a naming issue as described in https://github.com/huggingface/diffusers/issues/2011#issuecomment-1547958131. Passing `num_attention_heads` will only be supported in diffusers v0.19." | |||
There was a problem hiding this comment.
I would prefer to try to remove this statement
| out_channels // num_attention_heads, | ||
| num_attention_heads, | ||
| out_channels // num_attention_heads, |
There was a problem hiding this comment.
Let's use key word arguments here when correcting it.
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
@DN6 let's finish up this soon:) |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
@yiyixuxu Anything specific you wanted to address in this current PR? |
|
Oh wait. Nvm I haven't addressed those initial comments. Will wrap this up. |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
What does this PR do?
There's some confusion around how
num_attention_headsis being used in the 3D UNet + Block. This PR attempts to fix the issue by correcting the values used fornum_attention_headsRelated:
#6872
#6873
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.