[Refactor] spliting`ResnetBlock2D` into multiple blocks #6166

yiyixuxu · 2023-12-14T01:42:22Z

This PR spin out ResnetBlockCondNorm2D from ResnetBlock2D: ResnetBlockCondNorm2D is a resnet block with a normalization layer that incorporates conditional information, e.g. AdaGroupNorm, SpatialNorm. This resnet block is mainly used by Kandinsky decoder movq and latent upscaler

yiyixuxu · 2023-12-14T01:43:27Z

still wip, cc @DN6 for awareness

DN6 · 2023-12-15T10:46:55Z

src/diffusers/models/unet_2d_blocks.py

        # there is always at least one resnet
        resnets = [
-            ResnetBlock2D(
+            get_resnet_block(


I think it might be better to just append the appropriate Resnet block here than introduce the block fetching function. Is the purpose here to ensure backwards compatibility? Or do these blocks use a mix of Resnet classes?

I'm aware that some of the blocks use a mix, i.e. the ones Kandinsky used for MOVQ: DownEncoderBlock2D, AttnDownEncoderBlock2D, UpDecoderBlock2D, AttnUpDecoderBlock2D.

We could use get_resnet_block for these blocks instead - I would prefer that way too, because it is more explicit and readable. I used the function everywhere for now because:

Technically all these blocks can be configured to use both types, I don't have the complete knowledge to know if it would break existing models. I think pretty unlikely for "spatial norm" though it is very specific to MOVQ

use this as an example to show how we can ensure backward compatibility. I only split ResnetBlock2D into 2 blocks here in the draft and as far as I'm aware, ResnetBlockCondNorm2D is only used by latent upscaler and Kandinsky; but maybe we want to have more blocks with more specific configurations. It can become unmanageable without a block-fetching function

I agree with you it is better just to use the appropriate Resnet blocks, and very happy to refactor later. I will wait to do that after hearing more about:

feedbacks on how many blocks we want to split into

what's the best strategy to ensure backward compatibility, given that we might not have complete data on how these blocks are used

feedbacks on how many blocks we want to split into

I think we'd want to split into the most common block types. I think this number is still quite unclear.

We could use get_resnet_block for these blocks instead - I would prefer that way too, because it is more explicit and readable.

How about we use the fetcher function but also add a comment above to denote which type of ResNet block is being used?

sayakpaul

Very nice start here. @yiyixuxu

What do we think about keeping resnet a dedicated module instead like we did in #6129? This way specific ResNet blocks can have their own script. This way resnet.py becomes quite clean IMO.

patrickvonplaten

Very nice first attempt here! I agree with @DN6 that creating a get_resnet_block(...) is not really making the code much more readable. Instead it would be better if we can directly add the correct Resnet class to the blocks.

I think most blocks only use one of the two ResNets - in this case can we add the correct ResNet right away? In the exceptional case where the block can use both ResNets, let's add an if-else statement?

HuggingFaceDocBuilderDev · 2023-12-28T05:14:53Z

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.

yiyixuxu · 2023-12-28T05:54:37Z

@sayakpaul

What do we think about keeping resnet a dedicated module instead like we did in #6129? This way specific ResNet blocks can have their own script. This way resnet.py becomes quite clean IMO.

can we do these in future PRs? I would rather save these types of tasks for the last, i.e split one files into multiple, move them around etc

yiyixuxu · 2023-12-28T06:23:11Z

@patrickvonplaten @DN6
I removed get_resnet_block, let me know if you have any other feedbacks!

src/diffusers/models/resnet.py

patrickvonplaten

Nice looking good - got only one comment regarding the deprecation warning

Co-authored-by: Patrick von Platen <[email protected]>

DN6 · 2024-01-03T10:50:28Z

src/diffusers/models/resnet.py

+            deprecate(
+                "ada_group",
+                "1.0.0",
+                "Passing `ada_group` as `time_embedding_norm` is deprecated, please create `ResnetBlockCondNorm2D` instead",
+            )
+        if time_embedding_norm == "spatial":
+            raise ValueError(
+                "spatial",
+                "1.0.0",
+                "Passing `spatial` as `time_embedding_norm` is deprecated, please create `ResnetBlockCondNorm2D` instead",
+            )
+


Why use deprecate for one and ValueError for the other?

oops 😅 fixed it

DN6

It's looking really good! 👍🏽

sayakpaul · 2024-01-04T04:18:06Z

src/diffusers/models/resnet.py

+                "Passing `ada_group` as `time_embedding_norm` is deprecated, please create `ResnetBlockCondNorm2D` instead",
+            )


I am not sure if saying something is deprecated is a good idea in a "ValueError". Have we ever done that?

Yeah, let's indeed rename the message here to something like "This class cannot be used with "type==ada_group", please use XXX instead"

sayakpaul

Very nice. Just one comment.

patrickvonplaten

Nice!

…et2d

…6166) --------- Co-authored-by: yiyixuxu <yixu310@gmail,com> Co-authored-by: Patrick von Platen <[email protected]> Co-authored-by: Sayak Paul <[email protected]>

yiyixuxu added 6 commits December 13, 2023 09:20

first draft - seperate out ada_group and spatial norm

1173013

add a test line print

c79dc8e

more prints

b75c8e6

fi

b89788e

remove testing lines

ef8b900

get_resnet_block()

282bce4

yiyixuxu added 2 commits December 14, 2023 07:42

fix ada_group norm

3c7c361

update doc string

d6e5881

yiyixuxu added the refactor label Dec 14, 2023

DN6 reviewed Dec 15, 2023

View reviewed changes

sayakpaul reviewed Dec 20, 2023

View reviewed changes

patrickvonplaten reviewed Dec 26, 2023

View reviewed changes

remove get_resnet_block function

8e0c9c9

yiyixuxu added 4 commits December 28, 2023 05:38

fix

d5f2e82

Fix copies

763ac1d

merge main

e69a56d

style

34ddd7e

patrickvonplaten reviewed Jan 2, 2024

View reviewed changes

src/diffusers/models/resnet.py Outdated Show resolved Hide resolved

patrickvonplaten reviewed Jan 2, 2024

View reviewed changes

Update src/diffusers/models/resnet.py

20196a2

Co-authored-by: Patrick von Platen <[email protected]>

DN6 reviewed Jan 3, 2024

View reviewed changes

yiyixuxu and others added 2 commits January 3, 2024 17:07

deprecate -> valueerror

d69b577

Merge branch 'main' into resnet2d

b0af51e

sayakpaul reviewed Jan 4, 2024

View reviewed changes

sayakpaul approved these changes Jan 4, 2024

View reviewed changes

patrickvonplaten approved these changes Jan 5, 2024

View reviewed changes

sayakpaul mentioned this pull request Jan 8, 2024

refactor: extract init/forward function in UNet2DConditionModel #6478

Merged

17 tasks

yiyixuxu added 2 commits January 8, 2024 10:05

update message

7f4f169

Merge branch 'resnet2d' of github.com:huggingface/diffusers into resn…

10f7098

…et2d

yiyixuxu merged commit dd4459a into main Jan 10, 2024

yiyixuxu deleted the resnet2d branch January 10, 2024 07:38

		"Passing `ada_group` as `time_embedding_norm` is deprecated, please create `ResnetBlockCondNorm2D` instead",
		)

[Refactor] splitingResnetBlock2D into multiple blocks #6166

[Refactor] splitingResnetBlock2D into multiple blocks #6166

Uh oh!

Conversation

yiyixuxu commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yiyixuxu commented Dec 14, 2023

Uh oh!

DN6 Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiyixuxu Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sayakpaul Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

sayakpaul left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickvonplaten left a comment

Choose a reason for hiding this comment

Uh oh!

HuggingFaceDocBuilderDev commented Dec 28, 2023

Uh oh!

yiyixuxu commented Dec 28, 2023

Uh oh!

yiyixuxu commented Dec 28, 2023

Uh oh!

Uh oh!

patrickvonplaten left a comment

Choose a reason for hiding this comment

Uh oh!

DN6 Jan 3, 2024

Choose a reason for hiding this comment

Uh oh!

yiyixuxu Jan 3, 2024

Choose a reason for hiding this comment

Uh oh!

DN6 left a comment

Choose a reason for hiding this comment

Uh oh!

sayakpaul Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

patrickvonplaten Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

yiyixuxu Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

sayakpaul left a comment

Choose a reason for hiding this comment

Uh oh!

patrickvonplaten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[Refactor] spliting`ResnetBlock2D` into multiple blocks #6166

[Refactor] spliting`ResnetBlock2D` into multiple blocks #6166

yiyixuxu commented Dec 14, 2023 •

edited

Loading

DN6 Dec 15, 2023 •

edited

Loading

yiyixuxu Dec 15, 2023 •

edited

Loading

sayakpaul left a comment •

edited

Loading