Skip to content

Conversation

yiyixuxu
Copy link
Collaborator

@yiyixuxu yiyixuxu commented Aug 26, 2023

This PR fix #4727

We now pass relevant kwargs to load_config() when we retrieve the config file

I only updated for Text2Image here, will update other auto pipelines too if it's ok

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 26, 2023

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

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Nice job - that looks correct to me!
Can we apply this change also to all other autopipelines to make sure they are also fixed?

yiyixuxu and others added 2 commits August 26, 2023 08:22
Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
@patrickvonplaten
Copy link
Contributor

patrickvonplaten commented Aug 26, 2023

Super! Sorry, one last thing: Let's maybe add a small test to check that everything works as expected would be nice - e.g. maybe just passing the cache_dir to a dummy model and make sure that both config and model are saved in the same cache dir

@yiyixuxu
Copy link
Collaborator Author

@patrickvonplaten

I think it will be pretty tricky to test, load_config() is called twice here:

first time to find out the original class name for the pipeline, and then it will be called again once we dispatch to the pipeline_class.from_pretrained() - so the config and model will always be saved in the cache_dir the second time we call load_config.

@patrickvonplaten
Copy link
Contributor

Hmm yeah sorry this:

Super! Sorry, one last thing: Let's maybe add a small test to check that everything works as expected would be nice - e.g. maybe just passing the cache_dir to a dummy model and make sure that both config and model are saved in the same cache dir

wasn't very clear / didn't make too much sense 😅

Then let's maybe more or less use the reproducible code snippet here: #4727 (comment) as a way of testing no? Using only_local_files as a way of testing should be easy to set up :-)

Maybe you can write a test similar to this one:

def test_local_files_only_are_used_when_no_internet(self):
(you don't need to add the mock internet part though)

@yiyixuxu
Copy link
Collaborator Author

@patrickvonplaten

So the code snippet here #4727 (comment) isn't reproducible as it is: I think they happened to have an outdated copy of the cached model.

I made a test to try to recreate this edge case by modifying the commit ID. Let me know if this is ok, or if there is an easier/better way to test.

@patrickvonplaten
Copy link
Contributor

Looks great - thanks!

@yiyixuxu yiyixuxu merged commit a971c59 into main Aug 28, 2023
@yiyixuxu yiyixuxu deleted the auto_pipe_fix branch August 28, 2023 17:42
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* fix

---------

Co-authored-by: yiyixuxu <yixu310@gmail,com>
Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* fix

---------

Co-authored-by: yiyixuxu <yixu310@gmail,com>
Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto Pipelines May Break When Using local_files_only
3 participants