Fix config prints and save, load of pipelines #2849

patrickvonplaten · 2023-03-27T19:19:29Z

As noticed in ##2806 the pipeline currently does not take changes of its components into account. This PR make sure that when changing the scheduler or vae the changes are correctly reflected when saving the pipeline afterwards.

🚨🚨 Also, after this PR config attributes are not changed as instance attributes anymore. This was incorrectly implemented before and is removed as explained here: #2849 (comment) . This can for some edge cases be a breaking change. In this case please make sure to access directly the config variable. The most prominent examples are added as deprecated properties to limit the breaking changes to a minimum. 🚨🚨

HuggingFaceDocBuilderDev · 2023-03-27T19:24:43Z

The documentation is not available anymore as the PR was closed or merged.

patrickvonplaten · 2023-03-28T12:28:20Z

src/diffusers/configuration_utils.py

@@ -109,13 +109,6 @@ def register_to_config(self, **kwargs):
        # TODO: remove this when we remove the deprecation warning, and the `kwargs` argument,
        # or solve in a more general way.
        kwargs.pop("kwargs", None)
-        for key, value in kwargs.items():
-            try:
-                setattr(self, key, value)


I think we should delete this, I don't know why I added it back then, but it's not a good design looking at it now.

In short currently, we have the following logic:

All diffusers models and pipelines shall inherit from ConfigMixin which gives some nice config utils:

the .config property`

automatic saving of arguments pass to the class contstructor

load / save config

from_config

The API is designed to allow the following

class MyModel(ConfigMixin): @register_to_config def __init__(self, attr1, attr2): super().__init__()

This currently does two things:

a) it automatically saves attr1 and attr2 as self.config.attr1 and self.config.attr2
b) However, it also saves attr1 and attr2 as self.attr1 and self.attr2

I'm not sure why I thought b) is a good idea back then, but I think it's a pretty bad design choice now as it's not intuitive and also prone to errors (see raise error statement here). Also it doesn't really help as every attribute can be accessed with self.config . Also it badly entangles object members such as pipe.unet with configs which are not models.

=> Long story short, I think we should remove option b). This will clearly be a breaking change, but I don't think anybody really used b) much before as it's quite weird to assume that config attributes are direct members of the class instance.

@pcuenca @sayakpaul @patil-suraj @williamberman @yiyixuxu ok for you if I remove this with a big 🚨 headear?

As you can see many tests are failing, but they are all easy to fix.

+100 to your reasoning. I am okay with this.

This way, we also encourage our users to access configuration variables from .config which reduces the cognitive burdens and helps establish a clean and uniform design philosophy.

I don't remember why this was added in the first place, but I'm all for simplifying the API. Thanks for revisiting!

This changes broke this pipeline

I think it could break many more, instead of not setting attributes anymore we should set getters that tell the user these properties are deprecated, should i open a PR?

That's fair. I think it should be nice. Cc @patrickvonplaten

Too many breaking changes because of this PR, will try to fix and make a patch release

This PR should work for the patch release: #3129

@remorses sorry about the breaking change, we just made a patch release for it

pcuenca · 2023-03-29T08:58:43Z

src/diffusers/configuration_utils.py

@@ -109,13 +109,6 @@ def register_to_config(self, **kwargs):
        # TODO: remove this when we remove the deprecation warning, and the `kwargs` argument,
        # or solve in a more general way.
        kwargs.pop("kwargs", None)
-        for key, value in kwargs.items():
-            try:
-                setattr(self, key, value)


I don't remember why this was added in the first place, but I'm all for simplifying the API. Thanks for revisiting!

src/diffusers/pipelines/pipeline_utils.py

pcuenca · 2023-04-11T09:37:31Z

src/diffusers/configuration_utils.py

@@ -109,13 +109,6 @@ def register_to_config(self, **kwargs):
        # TODO: remove this when we remove the deprecation warning, and the `kwargs` argument,
        # or solve in a more general way.
        kwargs.pop("kwargs", None)
-        for key, value in kwargs.items():
-            try:
-                setattr(self, key, value)


An alternative to minimize breaking changes would be to implement __getattr__ and return the property if it's in the config. It's not so strange to think that, i.e., in_channels could be an attribute of unet.

pcuenca · 2023-04-11T09:40:19Z

src/diffusers/models/unet_1d.py

+    @property
+    def in_channels(self):
+        deprecate(
+            "in_channels",
+            "1.0.0",
+            "Accessing `in_channels` directly via unet.in_channels is deprecated. Please use `unet.config.in_channels` instead",
+            standard_warn=False,
+        )
+        return self.config.in_channels
+


Doing this via __getattr__ we could deprecate all properties, not just in_channels.

Ah yeah good idea

Ok played around with it a bit and don't feel super comfortable overwriting __getattr__ actually. Many Python methods such as hasattr(...) rely on __getattr__ and I don't manage to escape infinite recursions and have nice standard error messages here.

Added deprecation properties for all the important direct accesses, think that should be good enough.

pcuenca · 2023-04-11T09:41:37Z

src/diffusers/pipelines/ddpm/pipeline_ddpm.py

@@ -68,9 +68,9 @@ def __call__(
        """
        # Sample gaussian noise to begin loop
        if isinstance(self.unet.sample_size, int):
-            image_shape = (batch_size, self.unet.in_channels, self.unet.sample_size, self.unet.sample_size)
+            image_shape = (batch_size, self.unet.config.in_channels, self.unet.sample_size, self.unet.sample_size)


Why is sample_size not accessed through the config too?

Yeah sample_size is defined separately by the unet so that's fine, but probably better to use config anyways.

adhikjoshi · 2023-04-12T05:53:48Z

Using lpw custom pipeline, now running this error after this PR

File "/workspace/server/tasks.py", line 325, in txt2img
2023-04-12T05:48:19.164871592Z result = self.pipe.text2img(**args_dict)
2023-04-12T05:48:19.164875432Z File "/root/.cache/huggingface/modules/diffusers_modules/git/lpw_stable_diffusion.py", line 942, in text2img
2023-04-12T05:48:19.164879552Z return self.call(
2023-04-12T05:48:19.164883222Z File "/opt/conda/envs/ldm/lib/python3.8/site-packages/torch/utils/_contextlib.py", line 115, in decorate_context
2023-04-12T05:48:19.164887812Z return func(*args, **kwargs)
2023-04-12T05:48:19.164891452Z File "/root/.cache/huggingface/modules/diffusers_modules/git/lpw_stable_diffusion.py", line 855, in call
2023-04-12T05:48:19.164895462Z image, has_nsfw_concept = self.run_safety_checker(image, device, text_embeddings.dtype)
2023-04-12T05:48:19.164899382Z File "/root/.cache/huggingface/modules/diffusers_modules/git/lpw_stable_diffusion.py", line 593, in run_safety_checker
2023-04-12T05:48:19.164903442Z safety_checker_input = self.feature_extractor(self.numpy_to_pil(image), return_tensors="pt").to(device)
2023-04-12T05:48:19.164907432Z TypeError: 'NoneType' object is not callable

patrickvonplaten · 2023-04-12T13:12:23Z

Hey @adhikjoshi,

Could you maybe open a new issue for this with a reproducible code snippet? :-)

* [Config] Fix config prints and save, load * Only use potential nn.Modules for dtype and device * Correct vae image processor * make sure in_channels is not accessed directly * make sure in channels is only accessed via config * Make sure schedulers only access config attributes * Make sure to access config in SAG * Fix vae processor and make style * add tests * uP * make style * Fix more naming issues * Final fix with vae config * change more

Link to PR in diffusers repository: huggingface/diffusers#2691 Imports: `diffusers.models.cross_attention -> diffusers.models.attention_processor` Unions: `AttnProcessor -> AttentionProcessor` Classes: | Old name | New name | | --- | --- | | CrossAttention | Attention | | CrossAttnProcessor | AttnProcessor | | XFormersCrossAttnProcessor | XFormersAttnProcessor | | CrossAttnAddedKVProcessor | AttnAddedKVProcessor | | LoRACrossAttnProcessor | LoRAAttnProcessor | | LoRAXFormersCrossAttnProcessor | LoRAXFormersAttnProcessor | | FlaxCrossAttention | FlaxAttention | | AttendExciteCrossAttnProcessor | AttendExciteAttnProcessor | | Pix2PixZeroCrossAttnProcessor | Pix2PixZeroAttnProcessor | Also config values no longer sets as attributes of object: huggingface/diffusers#2849

* [Config] Fix config prints and save, load * Only use potential nn.Modules for dtype and device * Correct vae image processor * make sure in_channels is not accessed directly * make sure in channels is only accessed via config * Make sure schedulers only access config attributes * Make sure to access config in SAG * Fix vae processor and make style * add tests * uP * make style * Fix more naming issues * Final fix with vae config * change more

[Config] Fix config prints and save, load

de50ca2

patrickvonplaten changed the title ~~[Config] Fix config prints and save, load~~ [WIP][Config] Fix config prints and save, load Mar 27, 2023

patrickvonplaten mentioned this pull request Mar 27, 2023

printing pipeline doesn't take customized ones into account #2806

Closed

patrickvonplaten commented Mar 28, 2023

View reviewed changes

pcuenca reviewed Mar 29, 2023

View reviewed changes

patrickvonplaten added 4 commits April 9, 2023 20:03

Only use potential nn.Modules for dtype and device

527ebf5

Merge branch 'main' into fix_config_print_and_save

0e634c6

Correct vae image processor

ed0d30c

make sure in_channels is not accessed directly

cdb58ef

patrickvonplaten force-pushed the fix_config_print_and_save branch from 4075111 to cdb58ef Compare April 9, 2023 19:15

patrickvonplaten added 9 commits April 9, 2023 21:17

make sure in channels is only accessed via config

0b3a75d

Make sure schedulers only access config attributes

5e79e09

Merge branch 'main' into fix_config_print_and_save

e909088

Merge branch 'main' into fix_config_print_and_save

f1a92ca

Make sure to access config in SAG

2584856

Fix vae processor and make style

8b1a696

add tests

22db823

uP

4edbfc4

make style

ee359f8

patrickvonplaten changed the title ~~[WIP][Config] Fix config prints and save, load~~ Fix config prints and save, load Apr 10, 2023

patrickvonplaten changed the title ~~Fix config prints and save, load~~ Fix config prints and save, load of pipelines Apr 10, 2023

patrickvonplaten added 2 commits April 11, 2023 00:31

Fix more naming issues

f7f5781

Final fix with vae config

5da8e03

pcuenca approved these changes Apr 11, 2023

View reviewed changes

patrickvonplaten added 2 commits April 11, 2023 12:45

change more

bbc1e52

Merge branch 'main' into fix_config_print_and_save

6fe0479

patrickvonplaten merged commit 8b451eb into main Apr 11, 2023

patrickvonplaten deleted the fix_config_print_and_save branch April 11, 2023 11:35

StAlKeR7779 mentioned this pull request Apr 12, 2023

[Bugfix] Renames in 0.15.0 diffusers invoke-ai/InvokeAI#3184

Merged

patrickvonplaten mentioned this pull request Apr 12, 2023

[Pipelines] Make sure that None functions are correctly not saved #3080

Merged

sayakpaul mentioned this pull request Apr 17, 2023

Suggestion: Bump the major release number when releasing a compatibility-breaking version #3096

Closed

patrickvonplaten mentioned this pull request Apr 17, 2023

Fix config deprecation #3129

Merged

Fix config prints and save, load of pipelines #2849

Fix config prints and save, load of pipelines #2849

Uh oh!

Conversation

patrickvonplaten commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HuggingFaceDocBuilderDev commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickvonplaten Mar 28, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickvonplaten Apr 11, 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!

adhikjoshi commented Apr 12, 2023

Uh oh!

patrickvonplaten commented Apr 12, 2023

Uh oh!

Uh oh!

patrickvonplaten commented Mar 27, 2023 •

edited

Loading

HuggingFaceDocBuilderDev commented Mar 27, 2023 •

edited

Loading

patrickvonplaten Mar 28, 2023 •

edited

Loading

patrickvonplaten Apr 11, 2023 •

edited

Loading