Skip to content

fix a bug in from_pretrained when load optional components #4745

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
merged 5 commits into from
Aug 25, 2023
Merged

fix a bug in from_pretrained when load optional components #4745

merged 5 commits into from
Aug 25, 2023

Conversation

yiyixuxu
Copy link
Collaborator

@yiyixuxu yiyixuxu commented Aug 23, 2023 •
edited
Loading

Below script currently fails but will be fixed with this PR (see more details about the error here #4686
)

import torch
from diffusers import StableDiffusionUpscalePipeline

model_id = "stabilityai/stable-diffusion-x4-upscaler"
cache_path = "/home/yiyi_huggingface_co/cache"

pipe = StableDiffusionUpscalePipeline.from_pretrained(
    model_id,
    variant="fp16",
    torch_dtype=torch.float16
)
pipe.save_pretrained(cache_path)

pipe = StableDiffusionUpscalePipeline.from_pretrained(
    pretrained_model_name_or_path=cache_path,
    variant="fp16",
    torch_dtype=torch.float16
)

currently, when we call from_pretrained(), optional components are treated same as optional kwargs - they are passed to the init() directly, but they should be handled as modules

https://github.com/huggingface/diffusers/blob/main/src/diffusers/pipelines/pipeline_utils.py#L1016

@yiyixuxu yiyixuxu changed the title fix a bug in from_pretrained when load none value optional components fix a bug in from_pretrained when load optional components Aug 23, 2023
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 23, 2023 •
edited
Loading

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

patrickvonplaten
patrickvonplaten approved these changes Aug 23, 2023
View reviewed changes
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.

Great find & fix!

You're totally right. The problem is that for newer pipelines, we started allowing optional components to be None, e.g. as done here:

diffusers/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion_upscale.py

Line 102 in 0a0fe69

feature_extractor: Optional[CLIPImageProcessor] = None,
but we didn't allow this in the beginning:

diffusers/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion.py

Line 113 in 0a0fe69

safety_checker: StableDiffusionSafetyChecker,

Your fix is very nice - could you please add a test for this? Maybe both for StableDiffusionUpscale and StableDiffusionXL pipelines (both have optional componens)

patrickvonplaten
patrickvonplaten reviewed Aug 23, 2023
View reviewed changes
patrickvonplaten
patrickvonplaten reviewed Aug 24, 2023
View reviewed changes
tests/pipelines/stable_diffusion_xl/test_stable_diffusion_xl.py Outdated
@@ -689,3 +690,26 @@ def test_stable_diffusion_xl_multi_prompts(self):

# ensure the results are not equal
assert np.abs(image_slice_1.flatten() - image_slice_3.flatten()).max() > 1e-4

@require_torch_gpu
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a GPU here - this should also work on CPU no?

patrickvonplaten
patrickvonplaten reviewed Aug 24, 2023
View reviewed changes
tests/pipelines/stable_diffusion_2/test_stable_diffusion_upscale.py Outdated
@@ -422,3 +423,42 @@ def test_stable_diffusion_pipeline_with_sequential_cpu_offloading(self):
mem_bytes = torch.cuda.max_memory_allocated()
# make sure that less than 2.9 GB is allocated
assert mem_bytes < 2.9 * 10**9

def test_stable_diffusion_upscale_pipeline_from_save_pretrained(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a dummy model here to not put too much burden on the tests (cc @DN6)

patrickvonplaten
patrickvonplaten approved these changes Aug 25, 2023
View reviewed changes
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.

Cool good to merge!
@sayakpaul can you maybe also give this a review?

sayakpaul
sayakpaul approved these changes Aug 25, 2023
View reviewed changes
Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Important one. Thank you.

@yiyixuxu yiyixuxu merged commit b3b2d30 into main Aug 25, 2023
@yiyixuxu yiyixuxu deleted the from_pretrained_optional_components branch August 25, 2023 16:25
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
@yiyixuxu @patrickvonplaten
fix a bug in from_pretrained when load optional components (hugging…
0ecac69 
…face#4745)

* fix
---------

Co-authored-by: yiyixuxu <yixu310@gmail,com>
Co-authored-by: Patrick von Platen <[email protected]>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
@yiyixuxu @patrickvonplaten
fix a bug in from_pretrained when load optional components (hugging…
14ef4c9 
…face#4745)

* fix
---------

Co-authored-by: yiyixuxu <yixu310@gmail,com>
Co-authored-by: Patrick von Platen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sayakpaul sayakpaul sayakpaul approved these changes

@patrickvonplaten patrickvonplaten patrickvonplaten approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yiyixuxu @HuggingFaceDocBuilderDev @sayakpaul @patrickvonplaten