[Discussion] allow new merging methods from peft

@pacman100

We rely on peft to supercharge our support doing all things LoRA: https://huggingface.co/docs/diffusers/main/en/tutorials/using_peft_for_inference.

Currently, it's possible to do multi-LoRA inference seamlessly using peft:
https://huggingface.co/docs/diffusers/main/en/tutorials/using_peft_for_inference#combine-multiple-adapters

However, the limitation of this is the following.

It currently supports only one merging method for combining different LoRAs.
It doesn't benefit from newer and more advanced merging methods from the community, having the potential to improve generation quality even further.

G.O.A.T @pacman100 is adding new merging methods to peft in huggingface/peft#1364, supporting TIES, DARE_LINEAR, etc.

Inspired by that, I quickly cooked up a Colab Notebook, that:

Converts diffusers LoRA into PeftModel. Otherwise, we won't be able to do what's described in the next point.
Loads multiple such PeftModel LoRAs and combines them using add_weighted_adapter() method from peft. The advantage of doing this that add_weighted_adapter() comes supported with all the new merging methods right off the bat.

Check #6892 (comment) for more results.

Limitations

The notebook shows a PoC where the LoRAs were only targeted for the UNet component of the diffusion pipeline.
The entire process of getting the PeftModels out seems like a big one.

Questions to discuss

Are we okay having support for new merging methods like this way?
Or should we expose a new argument in set_adapters() (merge_method) and add support for them within diffusers natively, possibly leveraging some utilities from peft?

Tagging everyone that might be interested to say their bits from the HF team:
@younesbelkada, @apolinario, @linoytsaban, @BenjaminBossan.

Folks from the community:
@okotaku, @asomoza, @fabiorigano.

Cc: @yiyixuxu for awareness.

Just a thought: One potential alternative could be that we try to push down the merging logic to the level of the LoRA layer. That way, diffusers could just provide a high level API for merging and delegate the complex logic to PEFT's LoraLayer.

I haven't checked in detail if this would work, but at first glance I think such a refactor could be possible. It would be a little more limited than having the logic on LoraModel (as is the case right now) -- e.g. it assumes that each layer can be merged independently of another, but if we ever introduce a merging method that violates the assumption, it would be hard to implement it.

e.g. it assumes that each layer can be merged independently of another, but if we ever introduce a merging method that violates the assumption, it would be hard to implement it.

I think that is okay provided we catch it early and throw appropriate error messages.

Just a thought: One potential alternative could be that we try to push down the merging logic to the level of the LoRA layer. That way, diffusers could just provide a high level API for merging and delegate the complex logic to PEFT's LoraLayer.

I feel like this would limit the merging methods to lora and would need separate implementation for each peft method. Keeping merging operation separate from peft modules classes might be better.

it assumes that each layer can be merged independently of another, but if we ever introduce a merging method that violates the assumption, it would be hard to implement it.

Moreover, there might be some merging methods where, for, eg, even in ties-merging you need to prune based on all peft values as opposed to layer level parameters. And I guess this would be true or many other merging methods.

Thanks, Prateek! Really appreciate your continual support in our repositories.

Keeping merging operation separate from peft modules classes might be better.

Do you have some kind of pseudo API design in mind?

Moreover, there might be some merging methods where, for, eg, even in ties-merging you need to prune based on all peft values as opposed to layer level parameters. And I guess this would be true or many other merging methods.

Hello Prateek, currently, the combining methods in PEFT are done layer by layer. This seems to work well in practice while allowing for the creation of new weighted adapters at runtime. We will be adding this information in the documentation

I feel like this would limit the merging methods to lora and would need separate implementation for each peft method. Keeping merging operation separate from peft modules classes might be better.

My expectation would be that different adapter methods will require different merging methods no matter what. In PEFT, we have several methods and it's not possible to share the same merging code between these methods. Maybe that will change in the future, but I don't think it's very likely.

@pacman100

I am posting some results that @pacman100 gathered after experimenting with different settings for the text-to-image generation case. The purpose is to give the community more evidence for consideration.

Pixel Art + Toy Face (with TIES merging)

Sticker + Toy Face (with DARE Linear merging)

Pixel Art + Toy Face (with DARE merging)

Wow, this is supercool. Is there a way to create a gradio demo of it? I would be down to make one but I am not sure how that is done. This is so helpful, if people can see what merging can do then they will start using it more often.

The Colab Notebook could be useful here: https://colab.research.google.com/drive/1EG1tb5qiioOvLF9UdDowNuEDa69RoXuG?usp=sharing :-)

It's a PoC, so expect rough edges there.

FWIW I really like to give people the freedom to do weird stuff, so this proposed method is ideal in that case:

model.add_weighted_adapter(
    adapters=[lora_one, lora_two],
    weights=[1.0, 1.0],
    combination_type="ties",
    adapter_name=merged_name_one,
    density=0.5,
)
pipe.set_adapters(merged_name)

Because it allows us to do this:

model.add_weighted_adapter(
    adapters=[lora_one, lora_two],
    weights=[1.0, 1.0],
    combination_type="ties",
    adapter_name=merged_name_one,
    density=0.5,
)
model.add_weighted_adapter(
    adapters=[lora_three, lora_four],
    weights=[1.0, 1.2],
    combination_type="ties",
    adapter_name=merged_name_two,
    density=0.5,
)
pipe.set_adapters([merged_name_one, merged_name_two], adapter_weights=[1.0, 1.0])

These are the results:

base	merge 1	merge 2	merge 1 + 2

This would be more complex to do if the combination type was an argument of set_adapters also if I try to merge the four loras at the same time it doesn't give me the same result.

There's one concern about this though, this requires that the loras have the same rank and most people don't know that, especially if you download a lot of them from sites like civit.ai, so half of the time it would fail.

I would really like to have this in diffusers though, especially if I make it possible to enable them with attention masks.

@pacman100

This would be more complex to do if the combination type was an argument of set_adapters

Could you explain this a bit?

also if I try to merge the four loras at the same time it doesn't give me the same result.

I think that is expected since merging 4 LoRAs at once with some hyperparameters doesn't equal the underlying formulation of merging them with different steps, such as the one you described. @pacman100 am I correct?

There's one concern about this though, this requires that the loras have the same rank and most people don't know that, especially if you download a lot of them from sites like civit.ai, so half of the time it would fail.

Yeah, this is currently a limitation of the API, which primarily stems from the underlying methods that it implements. Is that correct @pacman100 @BenjaminBossan? In that case, we could potentially catch that check early and error out. Documenting this behavior will also be useful.

I was referring to this question:

Or should we expose a new argument in set_adapters() (merge_method) and add support for them within diffusers natively, possibly leveraging some utilities from peft?

as I see it, there's two solutions to do the merging one is like what we have been doing:

model.add_weighted_adapter(
    adapters=[lora_one, lora_two],
    weights=[1.0, 1.0],
    combination_type="ties",
    adapter_name=merged_name_one,
    density=0.5,
)
pipe.set_adapters(merged_name_one)

and the other one could be like this:

pipe.set_adapters(adapters=[lora_one, lora_two], weights=[1.0, 1.0], combination_type="ties", density=0.5)

and do the add_weighted_adapter under the hood for the users, this would prevent the use of multiple merges like I did but makes it a lot more simpler, especially if we use the text_encoders in SDXL.

I would like the first option as an advanced user but the second option is easier to use. Maybe we can have both of them?

@pacman100

Yeah, this is currently a limitation of the API, which primarily stems from the underlying methods that it implements. Is that correct @pacman100 @BenjaminBossan? In that case, we could potentially catch that check early and error out. Documenting this behavior will also be useful.

Indeed, some merging methods require the same rank. We raise an early error in PEFT if we find that the ranks differ. Maybe we could think of a method to "compress" a LoRA weight to have lower rank, not sure if there is any research on that.

I would like the first option as an advanced user but the second option is easier to use. Maybe we can have both of them?

I see the argument for greater simplicity, but would also prefer the first, more powerful method. With good documentation, this should hopefully be understandable for users.

Yeah sure, why not! That's good to know. The second option would be easy to cook up and will give results faster. But if we want to go beyond there are always possibilities to explore. Pretty much in line with our philosophy of being "simple" over "easy".

especially if we use the text_encoders in SDXL.

Could you elaborate this a bit further? Note, FWIW, we will support LoRAs that have text encoder-related parts too.

@prateeky2806

Indeed, some merging methods require the same rank. We raise an early error in PEFT if we find that the ranks differ. Maybe we could think of a method to "compress" a LoRA weight to have lower rank, not sure if there is any research on that.

@prateeky2806 are you of aware of any directions here or should we start cooking one up? 😅

especially if we use the text_encoders in SDXL.

Could you elaborate this a bit further? Note, FWIW, we will support LoRAs that have text encoder-related parts too.

Don't mind my comment, I was thinking that we'll need to do it for each text encoder in addition to the unet but this probably would be wrapped inside a function.

@BenjaminBossan

I would like the first option as an advanced user but the second option is easier to use. Maybe we can have both of them?

super nice results! My humble opinion is that I agree with @BenjaminBossan and @asomoza, and also vote the first option, as it accommodates more use cases and adds flexibility, versatility etc. good documentation as @BenjaminBossan said and a walkthrough colab will bridge the gap I hope :)

Wow, this is supercool. Is there a way to create a gradio demo of it? I would be down to make one but I am not sure how that is done. This is so helpful, if people can see what merging can do then they will start using it more often.

I anticipate that with this, we may incorporate model merging options into invoke's training UI (gradio-based) - If you're interested in contributing, let us know!

Very cool research on this. Also agreen on users typically looking for more use-cases/flexibility/versatility - typically makes it more future proof as well

Application layer can offer end-user safeguards from the more insane reaches of the LaTeNt SpAcE

@prateeky2806

Indeed, some merging methods require the same rank. We raise an early error in PEFT if we find that the ranks differ. Maybe we could think of a method to "compress" a LoRA weight to have lower rank, not sure if there is any research on that.

@prateeky2806 are you of aware of any directions here or should we start cooking one up? 😅

I am actually working on this with one of the undergrad students I am mentoring. We should have something in the coming months that would solve these potential issues and could be used in PEFT as well.

@prateeky2806

I am actually working on this with one of the undergrad students I am mentoring. We should have something in the coming months that would solve these potential issues and could be used in PEFT as well.

@prateeky2806 the peft team is happy to more deeply collaborate with your and your mentee on that if that's possible. Happy to set up a channel on Slack on our workspace if you're down.

@yiyixuxu

Okay, so I think there's enough interest from the community for us to take some action here. I am curious to hear your thoughts too: @yiyixuxu @DN6, generally for this direction.

Approach 1

Follow what's done in the Colab Notebook from the OP of this issue.

From diffusers, we could provide easy utility functions to the users to prepare PeftModels from a given LoRA checkpoint.
Then the users just need to set the UNets and Text Encoders components of the underlying DiffusionPipelines they're using.

I think we can make it quite understandable and easy for the users to be comfortable with this workflow through solid documentation.

Pros

As long as new merging methods are added to peft, we will continue to benefit from them as long as they're compatible with the underlying checkpoints.
As a direct consequence of the above, we won't have to do much on the diffusers side and delegate a huge part of the workflow to peft. This gives us a good balance between the utility side of peft as well as the modeling side of peft.
We inherit all the peft specific methods for dealing with activating, de-activating, deleting the loaded adapters.
Easy sharing of the constructed PeftModels on the Hub as they come supported with push_to_hub() already.

Cons

It's a bit involved. But I would also argue that this will be used by advanced users.
We're introducing a new format to our users which isn't ideal. We are not leveraging load_lora_weights() here.

If we explore this direction, we might also benefit from PeftMixedModel that allows one to combine different adapters not just LoRA. We're also leveraging our ecosystem powerfully here by taking advantage of the strengths of what each library has to offer. I think it's also inline with our philosophy of being "simple" over "easy".

Approach 2

We expose a combination_type argument (along with the complementary arguments that might be necessary) in the set_adapters() function of a DiffusionPipeline.

Pros

Users are already familiar with doing inference with peft from diffusers via load_lora_weights() and other methods such as set_adapters().

Cons

Significant amount of code work from diffusers.
We might have to copy over the merging utils from peft and sync with it whenever there's a new and compatible merging method available in peft.
To be able to share the final merged adapters, we might have to share the entire pipeline. It'd be quite difficult to just share the state dict of the merged LoRA adapters. But I don't think this will be a super high-prio use case anyway.

Curious to hear what others think of this. Cc: @pacman100 @BenjaminBossan @younesbelkada

Well, I think the list of pros and cons speaks a very clear message :)

Also, I think that if we go with 1, it doesn't mean that we cannot later add a more "native" diffusers integration. Maybe once we have a very clear picture of how this will look like in PEFT, it will allow us to do some refactoring there which facilitates integration, resulting in the amount of extra code on diffusers side to be kept at a minimum.

Yeah I think so too. For now, I think we should allow the community to play with the option 1 and see how it goes.

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.

Based on the community interest, we landed a dedicated doc for merging LoRAs: https://huggingface.co/docs/diffusers/main/en/using-diffusers/merge_loras.

Navigation Menu

Repository navigation

[Discussion] allow new merging methods from `peft` #6892

Limitations

Questions to discuss

Pixel Art + Toy Face (with TIES merging)

Sticker + Toy Face (with DARE Linear merging)

Pixel Art + Toy Face (with DARE merging)

Approach 1

Pros

Cons

Approach 2

Pros

Cons

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[Discussion] allow new merging methods from peft #6892

Description

Limitations

Questions to discuss

Activity

BenjaminBossan commented on Feb 7, 2024

sayakpaul commented on Feb 7, 2024

prateeky2806 commented on Feb 7, 2024

sayakpaul commented on Feb 8, 2024

pacman100 commented on Feb 8, 2024

BenjaminBossan commented on Feb 9, 2024

sayakpaul commented on Feb 15, 2024

Pixel Art + Toy Face (with TIES merging)

Sticker + Toy Face (with DARE Linear merging)

Pixel Art + Toy Face (with DARE merging)

prateeky2806 commented on Feb 15, 2024

sayakpaul commented on Feb 15, 2024

asomoza commented on Feb 15, 2024

sayakpaul commented on Feb 15, 2024

asomoza commented on Feb 15, 2024

BenjaminBossan commented on Feb 15, 2024

sayakpaul commented on Feb 15, 2024

sayakpaul commented on Feb 15, 2024

asomoza commented on Feb 15, 2024

linoytsaban commented on Feb 15, 2024

hipsterusername commented on Feb 15, 2024

prateeky2806 commented on Feb 15, 2024

sayakpaul commented on Feb 16, 2024

sayakpaul commented on Feb 16, 2024

Approach 1

Pros

Cons

Approach 2

Pros

Cons

BenjaminBossan commented on Feb 16, 2024

sayakpaul commented on Feb 16, 2024

github-actions commented on Mar 13, 2024

sayakpaul commented on Mar 13, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

[Discussion] allow new merging methods from `peft` #6892