Skip to content

Conversation

GreggHelt2
Copy link
Contributor

@GreggHelt2 GreggHelt2 commented Oct 10, 2023

What does this PR do?

Adds ability to mix usage of T2I-Adapter(s) and ControlNet(s). Previously, UNet2DConditional implementation only allowed use of one or the other. Adds new forward() arg down_intrablock_additional_residuals specifically for T2I-Adapters. If down_intrablock_addtional_residuals is not used, maintains backward compatibility with prior usage of only T2I-Adapter or ControlNet but not both, via down_block_additional_residuals and mid_block_additional_residual args.

Here's an example of mixing T2I-Adapter and ControNets with this fix, (in an InvokeAI dev branch). From left to right: original image, then inference with prompt "old man" and same seed -- ControlNet Canny, T2I-Adapter Color, both applied together.
Screenshot from 2023-10-10 08-02-10

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline?
  • Did you read our philosophy doc (important for complex PRs)?
  • Was this discussed/approved via a Github issue or the forum? Please add a link to it if that's the case.
    The problem with combined use of T2I-Adapter and ControlNet was mentioned during initial discussions of diffusers T2I-Adapter implementation
  • Did you make sure to update the documentation with your changes?
    I added documentation for UNet2DConditional.forward() existing args
    down_block_additional_residuals and mid_block_additional_residual and new arg down_intrablock_additional_residuals
  • Did you write any new necessary tests?

Who can review?

@patrickvonplaten
@sayakpaul
@williamberman

Previously, UNet2DConditional implemnetation onloy allowed use of one or the other.
Adds new forward() arg down_intrablock_additional_residuals specifically for T2I-Adapters. If down_intrablock_addtional_residuals is not used, maintains backward compatibility with prior usage of only T2I-Adapter or ControlNet but not both
GreggHelt2 added a commit to invoke-ai/InvokeAI that referenced this pull request Oct 10, 2023
…ss_attention_controlled_conditioning() to reflect changes to T2I-Adapter implementation to allow usage of T2I-Adapter and ControlNet at the same time.

Also, the PREVIOUS commit (@8d3885d, which was already pushed to github repo) was wrongly commented, but too late to fix without a force push or other mucking that I'm reluctant to do. That commit is actually the one that has all the changes to diffusers_pipeline.py to use additional arg down_intrablock_additional_residuals (introduced in diffusers PR huggingface/diffusers#5362) to detangle T2I-Adapter from ControlNet inputs to main UNet.
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.

The change makes sense to me - we should try to keep UNet2D as general as possible to allow for such use cases.

Before merging it would be good to:

  • 1.) Adapt all T2I pipelines to pass down_intrablock_additional_residuals instead of down_block_additional_residuals
  • 2.) Add a test that makes sure that the legacy format is still supported.

Also I'd very much like to get a review from @williamberman here

@@ -778,6 +778,7 @@ def forward(
added_cond_kwargs: Optional[Dict[str, torch.Tensor]] = None,
down_block_additional_residuals: Optional[Tuple[torch.Tensor]] = None,
mid_block_additional_residual: Optional[torch.Tensor] = None,
down_intrablock_additional_residuals: Optional[Tuple[torch.Tensor]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok for me! Think it's indeed a good idea to cleanly separate ControlNet and T2I here

Copy link
Contributor

@williamberman williamberman left a comment

Choose a reason for hiding this comment

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

Love this, down_intrablock_additional_residuals is a bit of a wonky name so if there is a better name we can come up with (I can't think of one), that would be great but if not lgtm

@HuggingFaceDocBuilderDev

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

@patrickvonplaten
Copy link
Contributor

Tests are passing, PR is a nice refactor that disentangles two concepts that used the same function input. Will fix quality test directly on main.

@patrickvonplaten patrickvonplaten merged commit de12776 into huggingface:main Oct 16, 2023
@GreggHelt2
Copy link
Contributor Author

Thanks all for the quick reviews and feedback!

GreggHelt2 added a commit to invoke-ai/InvokeAI that referenced this pull request Oct 16, 2023
…ss_attention_controlled_conditioning() to reflect changes to T2I-Adapter implementation to allow usage of T2I-Adapter and ControlNet at the same time.

Also, the PREVIOUS commit (@8d3885d, which was already pushed to github repo) was wrongly commented, but too late to fix without a force push or other mucking that I'm reluctant to do. That commit is actually the one that has all the changes to diffusers_pipeline.py to use additional arg down_intrablock_additional_residuals (introduced in diffusers PR huggingface/diffusers#5362) to detangle T2I-Adapter from ControlNet inputs to main UNet.
psychedelicious pushed a commit to invoke-ai/InvokeAI that referenced this pull request Oct 17, 2023
…ss_attention_controlled_conditioning() to reflect changes to T2I-Adapter implementation to allow usage of T2I-Adapter and ControlNet at the same time.

Also, the PREVIOUS commit (@8d3885d, which was already pushed to github repo) was wrongly commented, but too late to fix without a force push or other mucking that I'm reluctant to do. That commit is actually the one that has all the changes to diffusers_pipeline.py to use additional arg down_intrablock_additional_residuals (introduced in diffusers PR huggingface/diffusers#5362) to detangle T2I-Adapter from ControlNet inputs to main UNet.
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
…gface#5362)

* Add ability to mix usage of T2I-Adapter(s) and ControlNet(s).
Previously, UNet2DConditional implemnetation onloy allowed use of one or the other.
Adds new forward() arg down_intrablock_additional_residuals specifically for T2I-Adapters. If down_intrablock_addtional_residuals is not used, maintains backward compatibility with prior usage of only T2I-Adapter or ControlNet but not both

* Improving forward() arg docs in src/diffusers/models/unet_2d_condition.py

Co-authored-by: psychedelicious <[email protected]>

* Add deprecation warning if down_block_additional_residues is used for T2I-Adapter (intrablock residuals)

Co-authored-by: Patrick von Platen <[email protected]>

* Oops my bad, fixing last commit.

* Added import of diffusers utils.deprecate

* Conform to max line length

* Modifying T2I-Adapter pipelines to reflect change to UNet forward() arg for T2I-Adapter residuals.

---------

Co-authored-by: psychedelicious <[email protected]>
Co-authored-by: Patrick von Platen <[email protected]>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
…gface#5362)

* Add ability to mix usage of T2I-Adapter(s) and ControlNet(s).
Previously, UNet2DConditional implemnetation onloy allowed use of one or the other.
Adds new forward() arg down_intrablock_additional_residuals specifically for T2I-Adapters. If down_intrablock_addtional_residuals is not used, maintains backward compatibility with prior usage of only T2I-Adapter or ControlNet but not both

* Improving forward() arg docs in src/diffusers/models/unet_2d_condition.py

Co-authored-by: psychedelicious <[email protected]>

* Add deprecation warning if down_block_additional_residues is used for T2I-Adapter (intrablock residuals)

Co-authored-by: Patrick von Platen <[email protected]>

* Oops my bad, fixing last commit.

* Added import of diffusers utils.deprecate

* Conform to max line length

* Modifying T2I-Adapter pipelines to reflect change to UNet forward() arg for T2I-Adapter residuals.

---------

Co-authored-by: psychedelicious <[email protected]>
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants