Skip to content

Implement rest of the test cases (LoRA tests) #2824

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

Merged
merged 8 commits into from
Apr 12, 2023

Conversation

aandyw
Copy link
Contributor

@aandyw aandyw commented Mar 25, 2023

PR for issue #2789

@aandyw aandyw marked this pull request as ready for review March 25, 2023 06:20
@aandyw aandyw marked this pull request as draft March 25, 2023 06:20
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 25, 2023

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

@aandyw
Copy link
Contributor Author

aandyw commented Mar 31, 2023

@patrickvonplaten Haven't pushed the updated code but atm I'm thinking that we add another conditional statement to:

if name.startswith("mid_block"):
      hidden_size = model.config.block_out_channels[-1]
  elif name.startswith("up_blocks"):
      block_id = int(name[len("up_blocks.")])
      hidden_size = list(reversed(model.config.block_out_channels))[block_id]
  elif name.startswith("down_blocks"):
      block_id = int(name[len("down_blocks.")])
      hidden_size = model.config.block_out_channels[block_id]

for name.startswith("transformer_in"). However, I'm not quite sure how to deal with these initial layers. I've tried ignoring them but it seems like model.set_attn_processor(lora_attn_procs) requires lora_attn_procs to contain something for the transformer_in layers.

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.

Looks very cool! Think your added tests are great! Went into the PR to help a bit. test_lora_processors now passes and I think you can use its logic to make the other tests as well - see: https://github.com/huggingface/diffusers/pull/2824/files#r1157208349

Let me know if you need any more help :-)

cc @sayakpaul could you also take a look here?

@aandyw
Copy link
Contributor Author

aandyw commented Apr 4, 2023

Looks very cool! Think your added tests are great! Went into the PR to help a bit. test_lora_processors now passes and I think you can use its logic to make the other tests as well - see: https://github.com/huggingface/diffusers/pull/2824/files#r1157208349

Let me know if you need any more help :-)

cc @sayakpaul could you also take a look here?

Thank you for the help! I really appreciate it. Would you be able to recommend some resources that I could look into to learn more about this structure and what the transformer_in is doing specifically? I'm amazed by how fast you were able to figure out the PR.

@aandyw aandyw marked this pull request as ready for review April 4, 2023 20:19
@@ -38,7 +40,10 @@
def create_lora_layers(model):
lora_attn_procs = {}
for name in model.attn_processors.keys():
cross_attention_dim = None if name.endswith("attn1.processor") else model.config.cross_attention_dim
has_cross_attention = name.endswith("attn2.processor") and not (
Copy link
Member

Choose a reason for hiding this comment

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

Love this!

with torch.no_grad():
sample1 = model(**inputs_dict).sample

lora_attn_procs = {}
Copy link
Member

@sayakpaul sayakpaul Apr 5, 2023

Choose a reason for hiding this comment

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

We can leverage the create_lora_layers() here and elsewhere, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree - @pie31415 could we maybe as a final todo factor out this code:

        for name in model.attn_processors.keys():
            has_cross_attention = name.endswith("attn2.processor") and not (
                name.startswith("transformer_in") or "temp_attentions" in name.split(".")
            )
            cross_attention_dim = model.config.cross_attention_dim if has_cross_attention else None

            if name.startswith("mid_block"):
                hidden_size = model.config.block_out_channels[-1]
            elif name.startswith("up_block"):
                block_id = int(name[len("up_blocks.")])
                hidden_size = list(reversed(model.config.block_out_channels))[block_id]
            elif name.startswith("down_blocks"):
                block_id = int(name[len("down_blocks.")])
                hidden_size = model.config.block_out_channels[block_id]
            elif name.startswith("transformer_in"):
                # Note that the `8 * ...` comes from: https://github.com/huggingface/diffusers/blob/7139f0e874f10b2463caa8cbd585762a309d12d6/src/diffusers/models/unet_3d_condition.py#L148
                hidden_size = 8 * model.config.attention_head_dim

            lora_attn_procs[name] = LoRAAttnProcessor(hidden_size=hidden_size, cross_attention_dim=cross_attention_dim)

            with torch.no_grad():
                lora_attn_procs[name].to_q_lora.up.weight += 1
                lora_attn_procs[name].to_k_lora.up.weight += 1
                lora_attn_procs[name].to_v_lora.up.weight += 1
                lora_attn_procs[name].to_out_lora.up.weight += 1

into a create_lora_layers() as it's used three times further below? :-)

Think after this we're good for merge ❤️

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.

Thanks for taking on this one! Great work 🔥

@patrickvonplaten
Copy link
Contributor

Looks very cool! Think your added tests are great! Went into the PR to help a bit. test_lora_processors now passes and I think you can use its logic to make the other tests as well - see: https://github.com/huggingface/diffusers/pull/2824/files#r1157208349
Let me know if you need any more help :-)
cc @sayakpaul could you also take a look here?

Thank you for the help! I really appreciate it. Would you be able to recommend some resources that I could look into to learn more about this structure and what the transformer_in is doing specifically? I'm amazed by how fast you were able to figure out the PR.

To be honest the Unet3DConditionModel is quite specific and I don't really know the reason why the authors decided to add a transformer in block 🤷

To understand what's going on, I don't have too many tips besides reading the code:

and trying to understand how they are connected. Note that we use the unet_3d_blocks.py as a file to collect many different blocks depending on the architecture while we try to have the other three files as clean as possible.

Very well done in the PR though - we're almost there!

@aandyw aandyw changed the title [WIP] implement rest of the test cases (LoRA, Slow tests) [WIP] implement rest of the test cases (LoRA tests) Apr 11, 2023
@aandyw
Copy link
Contributor Author

aandyw commented Apr 11, 2023

@patrickvonplaten @sayakpaul PR should be good to merge!

lora_attn_procs[name] = LoRAAttnProcessor(hidden_size=hidden_size, cross_attention_dim=cross_attention_dim)
lora_attn_procs[name] = lora_attn_procs[name].to(model.device)

lora_attn_procs = create_lora_layers(model, mock_weights=False)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not mocking weights here?

lora_attn_procs[name] = LoRAAttnProcessor(hidden_size=hidden_size, cross_attention_dim=cross_attention_dim)
lora_attn_procs[name] = lora_attn_procs[name].to(model.device)

lora_attn_procs = create_lora_layers(model, mock_weights=False)
Copy link
Member

Choose a reason for hiding this comment

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

Same question.

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.

I truly appreciate your hard work! I think this was super important.

I just have a single doubt after which we should be good to merge 🚀

@sayakpaul sayakpaul merged commit 9d7c08f into huggingface:main Apr 12, 2023
@patrickvonplaten
Copy link
Contributor

Great job @pie31415 !

w4ffl35 pushed a commit to w4ffl35/diffusers that referenced this pull request Apr 14, 2023
* inital commit for lora test cases

* help a bit with lora for 3d

* fixed lora tests

* replaced redundant code

---------

Co-authored-by: Patrick von Platen <[email protected]>
Co-authored-by: Sayak Paul <[email protected]>
dg845 pushed a commit to dg845/diffusers that referenced this pull request May 6, 2023
* inital commit for lora test cases

* help a bit with lora for 3d

* fixed lora tests

* replaced redundant code

---------

Co-authored-by: Patrick von Platen <[email protected]>
Co-authored-by: Sayak Paul <[email protected]>
@aandyw aandyw changed the title [WIP] implement rest of the test cases (LoRA tests) Implement rest of the test cases (LoRA tests) Jun 20, 2023
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* inital commit for lora test cases

* help a bit with lora for 3d

* fixed lora tests

* replaced redundant code

---------

Co-authored-by: Patrick von Platen <[email protected]>
Co-authored-by: Sayak Paul <[email protected]>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* inital commit for lora test cases

* help a bit with lora for 3d

* fixed lora tests

* replaced redundant code

---------

Co-authored-by: Patrick von Platen <[email protected]>
Co-authored-by: Sayak Paul <[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.

4 participants