Skip to content
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

Allow passing a checkpoint state_dict to convert_from_ckpt (instead of just a string path) #4653

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

cmdr2
Copy link
Contributor

@cmdr2 cmdr2 commented Aug 17, 2023

This allows passing either a string path or a state_dict to the download_from_original_stable_diffusion_ckpt() method.

This makes it a bit more consistent with other model loading functions in diffusers, as well as allows external management and inspection of the state_dict (for e.g. Easy Diffusion analyzes the state dict for a few more things to determine the correct config file).

In general, it just provides a lot more flexibility to the users of this function, without any real downside.

Thanks!

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.

Ok for me once tests are all green. @sayakpaul wdyt?

@cmdr2
Copy link
Contributor Author

cmdr2 commented Aug 24, 2023

@patrickvonplaten I don't think the failing tests are from this PR.

Looking at the code of these tests, they don't use anything from the ckpt loader.

FAILED tests/models/test_lora_layers.py::SDXLLoraLoaderMixinTests::test_load_lora_locally - AssertionError: False is not true
FAILED tests/pipelines/paint_by_example/test_paint_by_example.py::PaintByExamplePipelineFastTests::test_save_load_local - ValueError: Trying to set a tensor of shape torch.Size([1, 1, 32]) in "uncond_vector" (which has shape torch.Size([1, 1, 768])), this look incorrect.
FAILED tests/pipelines/paint_by_example/test_paint_by_example.py::PaintByExamplePipelineFastTests::test_save_load_optional_components - ValueError: Trying to set a tensor of shape torch.Size([1, 1, 32]) in "uncond_vector" (which has shape torch.Size([1, 1, 768])), this look incorrect.

@patrickvonplaten
Copy link
Contributor

You're right @cmdr2 :-) Updating this PR to "main"

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@patrickvonplaten patrickvonplaten merged commit cb432c4 into huggingface:main Aug 25, 2023
@cmdr2
Copy link
Contributor Author

cmdr2 commented Aug 30, 2023

Thanks @patrickvonplaten ! :)

@xhinker
Copy link
Contributor

xhinker commented Oct 21, 2023

@cmdr2 Man, this parameter change from "checkpoint_path" to "checkpoint_path_or_dict" break everything, why not add a new parameter?

@patrickvonplaten I believe this PR break the fundamental open-close development rules, and Diffusers's guidelines too

@cmdr2
Copy link
Contributor Author

cmdr2 commented Oct 22, 2023

@xhinker I'm sorry, yes this was a mistake on my part. I assumed that the first argument wouldn't work with named parameters, since it was a mandatory argument (i.e. no default value). Clearly that was a mistake on my part.

Unfortunately, I see a few other scripts have started using the new variable name now (after it was checked-in on Aug 26). So reverting this change would break someone else again.

Again, my apologies for not checking all my assumptions.

@xhinker
Copy link
Contributor

xhinker commented Oct 23, 2023

@xhinker I'm sorry, yes this was a mistake on my part. I assumed that the first argument wouldn't work with named parameters, since it was a mandatory argument (i.e. no default value). Clearly that was a mistake on my part.

Unfortunately, I see a few other scripts have started using the new variable name now (after it was checked-in on Aug 26). So reverting this change would break someone else again.

Again, my apologies for not checking all my assumptions.

Hi, @cmdr2 thank you for the response, agree we should keep it as it is now. I will need to update all code in my part to reflect the change. but please, add new changes, instead of changing the existing input output, or dependance logic. Thank you.

@patrickvonplaten
Copy link
Contributor

Yeah going forward now that from_single_file is used much more we should be more careful and make sure we cleanly deprecate arguments that change names.

yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
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.

4 participants