Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Core] fix variant-identification. #9253
[Core] fix variant-identification. #9253
Changes from 7 commits
6b379a9
f155ec7
3f36e59
91253e8
dd5941e
564b8b4
fdd0435
d5cad9e
c0b1ceb
247dd93
b024a6d
fdfdc5f
dcf1852
3a71ad9
ab91852
aa631c5
453bfa5
11e4b71
dbdf0f9
671038a
57382f2
ea5ecdb
a510a9b
f583dad
dc0255a
f2ab3de
10baa9d
25ac01f
bac62ac
b6794ed
fcb4e39
4c0c5d2
0b1c2a6
8ad6b23
1190f7d
59cfefb
d72f5c1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 maybe we shoud just update the
_identify_model_variants
function usingvariant_compatible_siblings
and it is still not able to load variants with shared checkpoints from pipeline level
i.e. we should be able to load the fp16 variant in the transformer folder too but it is currently not
you get
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.
cc @DN6 @a-r-r-o-w here too
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 was moved up to raise error earlier in code.
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 remove this error in
download
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.
It's not an error, though. It's a deprecation. Do we exactly want to keep it that way? If so, we will have to remove it anyway because the deprecation is supposed to expire after "0.24.0" version.
Instead, we are erroring out now from
from_pretrained()
:diffusers/src/diffusers/pipelines/pipeline_utils.py
Line 757 in f583dad
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.
Ah got it. I think this should be resolved now.
WDYT about catching these errors without having to download the actual files and leveraging
model_info()
(in case we're querying the Hub) or regular string matching (in case it's local)? Currently, we're still callingdownload()
in case we don't have the model files cached. I think many errors can be caught and warnings can be thrown without having to do that.This could live in a future 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.
LMK if someone has a better idea to test it out.
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 should have been serialized with
variant
andsafe_serialization
otherwise the test seems wrong to me.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 would have failed with the fixes from this PR rightfully complaining:
We didn't have it because we never tested it. But we should be all good now.