-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Fix config prints and save, load of pipelines #2849
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
Changes from all commits
de50ca2
527ebf5
0e634c6
ed0d30c
cdb58ef
0b3a75d
5e79e09
e909088
f1a92ca
2584856
8b1a696
22db823
4edbfc4
ee359f8
f7f5781
5da8e03
bbc1e52
6fe0479
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| import torch.nn as nn | ||
|
|
||
| from ..configuration_utils import ConfigMixin, register_to_config | ||
| from ..utils import BaseOutput | ||
| from ..utils import BaseOutput, deprecate | ||
| from .embeddings import GaussianFourierProjection, TimestepEmbedding, Timesteps | ||
| from .modeling_utils import ModelMixin | ||
| from .unet_1d_blocks import get_down_block, get_mid_block, get_out_block, get_up_block | ||
|
|
@@ -190,6 +190,16 @@ def __init__( | |
| fc_dim=block_out_channels[-1] // 4, | ||
| ) | ||
|
|
||
| @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 | ||
|
|
||
|
Comment on lines
+193
to
+202
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing this via
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah good idea
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok played around with it a bit and don't feel super comfortable overwriting Added deprecation properties for all the important direct accesses, think that should be good enough. |
||
| def forward( | ||
| self, | ||
| sample: torch.FloatTensor, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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 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
ConfigMixinwhich gives some nice config utils:.configproperty`The API is designed to allow the following
This currently does two things:
a) it automatically saves
attr1andattr2asself.config.attr1andself.config.attr2b) However, it also saves
attr1andattr2asself.attr1andself.attr2I'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 aspipe.unetwith 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.
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.
+100 to your reasoning. I am okay with this.
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 way, we also encourage our users to access configuration variables from
.configwhich reduces the cognitive burdens and helps establish a clean and uniform design philosophy.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 don't remember why this was added in the first place, but I'm all for simplifying the API. Thanks for revisiting!
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.
yep!
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 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?
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 fair. I think it should be nice. Cc @patrickvonplaten
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.
Too many breaking changes because of this PR, will try to fix and make a patch release
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 PR should work for the patch release: #3129
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.
@remorses sorry about the breaking change, we just made a patch release for it