-
Notifications
You must be signed in to change notification settings - Fork 6.6k
Fix StableDiffusionXLPAGInpaintPipeline #9128
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
Conversation
2d7ff3b to
12bf313
Compare
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Hi @gumgood PR looks good. Could you please run |
9d7ef91 to
ff5fd9d
Compare
|
@DN6 Thank you for the review. I've applied the style changes. Before applying the style, I ran the tests and fixed a few additional issues:
|
yiyixuxu
left a comment
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.
thanks for the PR! I left a comment
| return image_latents | ||
|
|
||
| # Copied from diffusers.pipelines.stable_diffusion_xl.pipeline_stable_diffusion_xl_inpaint.StableDiffusionXLInpaintPipeline.prepare_mask_latents | ||
| # Modified from diffusers.pipelines.stable_diffusion_xl.pipeline_stable_diffusion_xl_inpaint.StableDiffusionXLInpaintPipeline.prepare_mask_latents |
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.
thanks! is it possible to keep this method Copied from but modify the output instead? e.g. this is how we modify ip_adapter_image_embeds for PAG https://github.com/huggingface/diffusers/blob/f848febacdc54c351ed0ed23fcc4c9349828021e/src/diffusers/pipelines/pag/pipeline_pag_sd_xl_inpaint.py#L1558C1-L1559C1
it's a little bit weird and requires more code but this way it will be easier for us to maintain all the PAG pipelines
in the future, we should make sure these methods that prepare conditions for the denoiser model always return negative condition and condition instead of a combined output
| enable_pag = kwargs.pop("enable_pag") | ||
| if enable_pag: | ||
| orig_class_name = config["_class_name"].replace("Pipeline", "PAGPipeline") | ||
| orig_class_name = ( |
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.
if we do this, the orig_class_name for StableDiffusionXLPipeline would be StableDiffusionXLPAGInpaintPipeline too - in this case will get the correct result regardless but logic is incorrect
the orig_class_name for StableDiffusionXLPipeline should be StableDiffusionXLPAGPipeline and then we map it to the corresponding to the inpainting class through _get_task_class(...) and get StableDiffusionXLPAGInpaintPipeline
we can maybe do something like
to_replace = "InpaintPipeline" if "Inpaint` in config["_class_name"] else "Pipeline"
orig_class_name = config["_class_name"].replace(to_replace, "PAG" + to_replace)
gumgood
left a comment
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.
@yiyixuxu thank you! I have made updates according to the suggestion.
| .replace("Pipeline", "PAGInpaintPipeline") | ||
| ) | ||
| to_replace = "InpaintPipeline" if "Inpaint" in config["_class_name"] else "Pipeline" | ||
| orig_class_name = config["_class_name"].replace(to_replace, "PAG" + to_replace) |
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.
if we do this, the
orig_class_nameforStableDiffusionXLPipelinewould beStableDiffusionXLPAGInpaintPipelinetoo - in this case will get the correct result regardless but logic is incorrectthe
orig_class_nameforStableDiffusionXLPipelineshould beStableDiffusionXLPAGPipelineand then we map it to the corresponding to the inpainting class through_get_task_class(...)and getStableDiffusionXLPAGInpaintPipelinewe can maybe do something like
to_replace = "InpaintPipeline" if "Inpaint` in config["_class_name"] else "Pipeline" orig_class_name = config["_class_name"].replace(to_replace, "PAG" + to_replace)
Nice code. I have appied it and verified that it works well
| mask = self._prepare_perturbed_attention_guidance(mask, mask, self.do_classifier_free_guidance) | ||
| masked_image_latents = self._prepare_perturbed_attention_guidance( | ||
| masked_image_latents, masked_image_latents, self.do_classifier_free_guidance | ||
| ) |
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.
Updated to handle batch size without modifying the prepare_mask_latents function.
yiyixuxu
left a comment
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.
thanks!
What does this PR do?
Fixed the issue preventing model and inference with
StableDiffusionXLInpaintPipelineclass in UNet wherein_channels=9.1. When loading models of the
StableDiffusionXLInpaintPipelineclass likediffusers/stable-diffusion-xl-1.0-inpainting-0.1, the load fails ifenable_pag=True.error:
Fixed to find the correct class
StableDiffusionXLPAGInpaintPipeline.2. When calling
__call__with a model wherein_channels=9in the UNet,latent_model_inputis not created correctlyerror:
The batch size of
maskandmasked_image_latentsdiffers from that oflatent_model_input. This has been fixed.Before submitting
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@sayakpaul @yiyixuxu @DN6