Skip to content

[Dummy imports] Better error message #795

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.

Sign up for GitHub

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

Jump to bottom
Merged
patrickvonplaten merged 3 commits into from
Oct 12, 2022
Merged

[Dummy imports] Better error message #795

patrickvonplaten merged 3 commits into from
Oct 12, 2022

Conversation

@patrickvonplaten
Copy link
Contributor

@patrickvonplaten patrickvonplaten commented Oct 10, 2022 •
edited
Loading

This PR adds from_config and from_pretrained dummy class methods to all dummy classes.

Running:

pytest -sv tests/test_pipelines.py::PipelineFastTests::test_stable_diffusion_k_lms

without scipy installed now yields:

E           ImportError: 
E           LMSDiscreteScheduler requires the scipy library but it was not found in your environment. You can install it with pip: `pip install
E           scipy`

as expected. Thanks for flagging @pcuenca

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 10, 2022 •
edited
Loading

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

@patrickvonplaten patrickvonplaten linked an issue Oct 10, 2022 that may be closed by this pull request
Better error message for unsupported classes #794
Closed
patrickvonplaten
patrickvonplaten commented Oct 10, 2022
View reviewed changes
utils/check_dummies.py
requires_backends(self, {1})
@classmethod
def from_config(cls, *args, **kwargs):
Copy link
Contributor Author

@patrickvonplaten patrickvonplaten Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding from_config and from_pretrained is quite simple here

@pcuenca
Copy link
Member

pcuenca commented Oct 10, 2022

Very cool!

Loading the scheduler shows the nice error, but loading the pretrained pipeline still gives the cryptic message to me:

        pipe = StableDiffusionPipeline.from_pretrained("hakurei/waifu-diffusion")

@pcuenca
Test: load pipeline with LMS scheduler.
6524e8d 
Fails with a cryptic message if scipy is not installed.
@pcuenca
Copy link
Member

pcuenca commented Oct 11, 2022

@patrickvonplaten I created a test to load a pipeline that uses the LMS Discrete scheduler. It currently fails with a cryptic error message if scipy is not installed. I'll try to debug it later.

@pcuenca
Copy link
Member

pcuenca commented Oct 11, 2022

If dummy_torch_and_scipy_objects.LMSDiscreteScheduler inherited from SchedulerMixin then it would be a load candidate and the nice error message would be presented. But I'm not sure how to achieve that with the current system.

Another option is to detect None here and show an error with the class name that we are attempting and failing to load.

What do you think?

@patrickvonplaten
Copy link
Contributor Author

patrickvonplaten commented Oct 12, 2022

@patrickvonplaten I created a test to load a pipeline that uses the LMS Discrete scheduler. It currently fails with a cryptic error message if scipy is not installed. I'll try to debug it later.

Thanks, checking it out now!

patrickvonplaten
patrickvonplaten commented Oct 12, 2022
View reviewed changes
tests/test_pipelines.py
assert np.abs(image_slice.flatten() - expected_slice).max() < 1e-2
assert np.abs(image_from_tuple_slice.flatten() - expected_slice).max() < 1e-2

def test_from_pretrained_error_message_uninstalled_packages(self):
Copy link
Contributor Author

@patrickvonplaten patrickvonplaten Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcuenca - I can't think of a good way to test that we have a nice error message going forward. Suggest to just merge without a good test for now. Let me know if you have other ideas!

Copy link
Member

@pcuenca pcuenca Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is not as important as the real use in the wild, like what happened with that Space that broke. If users see something reasonable instead of the previous cryptic message, then I'm all for it!

Copy link
Contributor Author

@patrickvonplaten patrickvonplaten Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the test as it's now doesn't really help at all 😅 I was just wondering if there is a way to "pretend" scipy is not installed and then verify that the error message is nice

@patrickvonplaten patrickvonplaten merged commit db47b1e into main Oct 12, 2022
@patrickvonplaten patrickvonplaten deleted the better_error_message branch October 12, 2022 12:41
prathikr pushed a commit to prathikr/diffusers that referenced this pull request Oct 26, 2022
@patrickvonplaten @pcuenca
[Dummy imports] Better error message (huggingface#795)
430954a 
* [Dummy imports] Better error message

* Test: load pipeline with LMS scheduler.

Fails with a cryptic message if scipy is not installed.

* Correct

Co-authored-by: Pedro Cuenca <[email protected]>
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
@patrickvonplaten @pcuenca
[Dummy imports] Better error message (huggingface#795)
fdc250f 
* [Dummy imports] Better error message

* Test: load pipeline with LMS scheduler.

Fails with a cryptic message if scipy is not installed.

* Correct

Co-authored-by: Pedro Cuenca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcuenca pcuenca Awaiting requested review from pcuenca

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Better error message for unsupported classes

4 participants

@patrickvonplaten @HuggingFaceDocBuilderDev @pcuenca