-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add PAG support to StableDiffusionControlNetPAGInpaintPipeline #8875
Add PAG support to StableDiffusionControlNetPAGInpaintPipeline #8875
Conversation
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. |
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 working on this! The PR looks absolutely perfect with all required PAG changes. I think you need to run make style
for the CI to go green. @yiyixuxu WDYT?
control_image = control_images if isinstance(control_image, list) else control_images[0] | ||
controlnet_prompt_embeds = prompt_embeds | ||
|
||
# 7.2 Create tensor stating which controlnets to keep |
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.
The numbering here onwards seems incorrect
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.
Oh you're right! Changing it...
mask_image, | ||
height, | ||
width, | ||
callback_steps, |
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.
I think you can remove this parameter completely instead of passing None
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.
Oh sure! I just removed it
Hi @a-r-r-o-w , Thank you very much for your feedback 🚀 ! I just updated my PR trying to address all the issues. Thanks again. |
Sorry I had a small mistake that made some tests failed. It should be solved now! |
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! super cool PR! I left one comment,
also, can we make sure it work with from_pipe()
?
@@ -944,7 +946,10 @@ def from_pretrained(cls, pretrained_model_or_path, **kwargs): | |||
if "enable_pag" in kwargs: | |||
enable_pag = kwargs.pop("enable_pag") | |||
if enable_pag: | |||
orig_class_name = config["_class_name"].replace("Pipeline", "PAGPipeline") | |||
if "controlnet" in kwargs: |
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.
can you provide an example where this code addition is needed? I think it should already be addressed in line 944 no?
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.
Hi @yiyixuxu,
Thank you very much for your comments. Sorry if I am missing something here, but my idea is as follows:
There are 4 cases that need to be handled:
-
No ControlNet and no Pag -> Keep Pipeline
-
With ControlNet and no Pag -> Replace Pipeline with ControlNetPipeline
-
No ControlNet and with Pag -> Replace Pipeline with PAGPipeline
-
With ControlNet and with Pag -> Replace Pipeline with ControlNetPAGPipeline, which will override the first case.
That's why I added the nested conditions, but maybe I am missing something 🤔
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.
i see! thank for explaining it, you were absolute ly right!
can we do this instead (to be consistent with AutoPipelineForText2Image and AutoPipelineForImage2Image)
if "controlnet" in kwargs:
orig_class_name = config["_class_name"].replace("Pipeline", "ControlNetPipeline")
if "enable_pag" in kwargs:
enable_pag = kwargs.pop("enable_pag")
if enable_pag:
orig_class_name = orig_class_name.replace("Pipeline", "PAGPipeline")
orig_class_name = orig_class_name.replace("Pipeline", "PAGPipeline") |
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.
Oh sure, done! This way is better!
Hi @yiyixuxu, Thanks for your support and feedback. I updated my PR with a new test. I run it with:
and it seems to be working fine. How do you like it? Would be enough for testing that it works with Please let me know 😃 |
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.
very nice! I left some comments for the test, since all the from_pipe tests are based on SDXL, if you want to skip it, it is ok too! Just let us know!
Let's merge this soon
@@ -283,6 +283,26 @@ def test_from_pipe_pag_inpaint(self): | |||
pipe = AutoPipelineForInpainting.from_pipe(pipe_pag, enable_pag=False) | |||
assert pipe.__class__.__name__ == "StableDiffusionXLInpaintPipeline" | |||
|
|||
def test_from_pipe_pag_controlnet_inpaint(self): |
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.
nice! we can combine this test with test_from_pipe_pag_inpaint
and make sure it covers all the scenarios similarly as in https://github.com/huggingface/diffusers/blob/main/tests/pipelines/test_pipelines_auto.py#L141
test_from_pipe_pag_inpaint
test for the inpainting XL pipeline, we can update to test SD pipeline instead because we now have a controlnet + inpaint + PAG for SD but not for SDXL- make sure :
- we test use
from_pipe
on inpainting pipeline, inpainting controlnet pipeline and pag inpainting controlnet - for each, make sure we test the combination of
enable_pag
and controlnet`
- we test use
but if you want to skip the test, it's ok too! we can update the test when we have the sdxl pag inpainting pipeline as well
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 @yiyixuxu! I'm short on time right now, so I removed it. Would this work for now? I can come back later to finish it properly, but I wanted to get the PR merged sooner. Please let me know what you think! 😄
…ag are present based on yiyixuxu feedback
Thanks a lot @yiyixuxu for your feedback. I addressed your comment in src/diffusers/pipelines/auto_pipeline.py, and remove for now the test I added so the PR can be merged. I can come back later to finish it properly or if someone want to take it, please let me know! |
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
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 so much for the PR
and very sorry I forgot about it and only merge it now
No worries @yiyixuxu! Thank you very much for the support. I am very happy that now is merged 🚀 |
…ngface#8875) * Add pag to controlnet inpainting pipeline --------- Co-authored-by: YiYi Xu <yixu310@gmail.com>
* Add pag to controlnet inpainting pipeline --------- Co-authored-by: YiYi Xu <yixu310@gmail.com>
What does this PR do?
Adds PAG (Perturbed-Attention Guidance) support for SD 1.5 with controlnet and inpainting models (StableDiffusionControlNetPAGInpaintPipeline )
Continuation of #8710. It was not mentioned on #8710 but I think this pipeline is pretty cool also 😄
Before submitting
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@a-r-r-o-w @yiyixuxu
Anyone in the community is free to review the PR once the tests have passed.
Code example