-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Check correct model type is passed to from_pretrained
#10189
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
2fd4508
78c6e68
185a78f
679c18c
6aad7a7
c1db3bd
50b740a
b8fa81a
44f24a4
5af8c7f
baea141
c5e1e2d
dba12b6
99b0f92
3a43c8a
803e33f
c81415b
13a824e
5679067
24d79a3
3f841d5
f18687f
87f8f03
56ac8b4
87dcf54
e193563
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 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1802,6 +1802,16 @@ def test_pipe_same_device_id_offload(self): | |||||||||||
sd.maybe_free_model_hooks() | ||||||||||||
assert sd._offload_gpu_id == 5 | ||||||||||||
|
||||||||||||
def test_wrong_model(self): | ||||||||||||
tokenizer = CLIPTokenizer.from_pretrained("hf-internal-testing/tiny-random-clip") | ||||||||||||
with self.assertRaises(ValueError) as error_context: | ||||||||||||
_ = StableDiffusionPipeline.from_pretrained( | ||||||||||||
"hf-internal-testing/diffusers-stable-diffusion-tiny-all", text_encoder=tokenizer | ||||||||||||
) | ||||||||||||
|
||||||||||||
assert "is of type" in str(error_context.exception) | ||||||||||||
assert "but should be" in str(error_context.exception) | ||||||||||||
Comment on lines
+1806
to
+1813
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. We're now using warning, but for this case, diffusers/src/diffusers/pipelines/pipeline_utils.py Lines 893 to 897 in 6324340
So it's a little inconsistent and needs further testing to determine which other cases this already applies to. |
||||||||||||
|
||||||||||||
|
||||||||||||
@slow | ||||||||||||
@require_torch_gpu | ||||||||||||
|
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 if it's not a scheduler and the types don't match it's okay to raise an error. I think it would break in the model loading step anyway in this case. wdyt @yiyixuxu?
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 prefer a warning because:
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.
we just added
use_flow_sigma
to a few non-flow match schedulers with the SANA pr, and also we plan to refactor them but don't have a design finalized yetgiven that, I think maybe we can skip checking for scheduler altogether for now, and revisit later. let me know what you guys think!
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've removed scheduler related changes for now, I think we can revisit that later, as @yiyixuxu mentioned above type hints haven't been strictly enforced there are probably some missing/wrong, especially for schedulers. Warning is better because of that too, if there is some wrong type hint that makes its way into a release we'd have to issue a hotfix release to fix it, that just creates headaches and issue reports.