diffusers icon indicating copy to clipboard operation
diffusers copied to clipboard

Published 20 hours ago • verified publisher icon huggingface

Fixing implementation of ControlNet-XS

Open UmerHA opened this issue 1 year ago • 4 comments

What does this PR do?

Implements ControlNet-XS with the feedback from PR #5827 incorporated.

See original PR for details.

Who can review?

Core library:

  • Pipelines: @patrickvonplaten and @sayakpaul

UmerHA avatar Jan 30 '24 16:01 UmerHA

Hi @patrickvonplaten,

I've updated PR #5827 with your review comments.

Re Do we need the class_labels parameter? (https://github.com/huggingface/diffusers/pull/5827#discussion_r1437019546 and https://github.com/huggingface/diffusers/pull/5827#discussion_r1437019775): Iiuc, class_labels are not only used for upsampling, but also for class-conditioned generation. So I'd include it.

Re https://github.com/huggingface/diffusers/pull/5827#discussion_r1437020210: I'm almost certain using the add_embedding of the base model is correct. That's also how the paper implementation does is. Why would you not do it?

All other comments are incorporated!

UmerHA avatar Feb 01 '24 14:02 UmerHA

@yiyixuxu can you review here?

patrickvonplaten avatar Feb 09 '24 16:02 patrickvonplaten

@yiyixuxu Pinging you as the last comment was 2 weeks ago. Let me know if I can help in any way!

UmerHA avatar Feb 25 '24 21:02 UmerHA

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.

HuggingFaceDocBuilderDev avatar Feb 28 '24 18:02 HuggingFaceDocBuilderDev

cc @DN6 for a final review

yiyixuxu avatar Apr 03 '24 19:04 yiyixuxu 

out_channels = out_channels or in_channels
self.in_channels = in_channels

oh sorry I meant do we need to assign it as an attribute to the model?

We don't, but out_channels is also assigned as an attribute to the model. For consistency, I'd also assign in_channels. wdyt @yiyixuxu ?

UmerHA avatar Apr 03 '24 20:04 UmerHA

Looking good overall 👍🏽 Left some comments.

Regarding the naming, we could perhaps call the module ControlNetXSAdapter? Since its behaviour is similar to our other adapters? cc: @yiyixuxu

DN6 avatar Apr 08 '24 05:04 DN6

Regarding the naming, we could perhaps call the module ControlNetXSAdapter? Since its behaviour is similar to our other adapters?

ok!

yiyixuxu avatar Apr 08 '24 17:04 yiyixuxu

Regarding the naming, we could perhaps call the module ControlNetXSAdapter? Since its behaviour is similar to our other adapters?

ok!

Sounds good. Have renamed

  • ControlNetXSAddon -> ControlNetXSAdapter
  • ControlNetXSAddonDownBlockComponents -> DownBlockControlNetXSAdapter, and similarly for mid/up blocks
  • get_down_block_addon -> get_down_block_adapter, and similarly for mid/up

UmerHA avatar Apr 09 '24 11:04 UmerHA

@UmerHA the failing tests look relevant here - can you look into and fix?

yiyixuxu avatar Apr 11 '24 17:04 yiyixuxu

@yiyixuxu you were right - should be fixed now. can you rerun the tests?

UmerHA avatar Apr 11 '24 20:04 UmerHA

The only failing test seems very unrelated to this PR:

CleanShot 2024-04-15 at 08 43 29@2x

cc @yiyixuxu

UmerHA avatar Apr 15 '24 06:04 UmerHA

@DN6 some of your comments aren't marked as resolved. do you have any more questions/feedback, or is it fine as is?

UmerHA avatar Apr 15 '24 16:04 UmerHA

@UmerHA All good. We can break up the tests in a follow up PR.

DN6 avatar Apr 16 '24 16:04 DN6

@UmerHA We have failing slow tests because the checkpoint doesn't seem to be updated. Can you take care of this please Raised an issue: #7694

DN6 avatar Apr 17 '24 06:04 DN6

Have updated the models & ran the slow test on a T4 - works now.

UmerHA avatar Apr 17 '24 11:04 UmerHA