Add lora+ implementation

[kallewoof]

LoRA+

Builds on #1509.

[HuggingFaceDocBuilderDev]

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.

[BenjaminBossan]

@kallewoof Thanks for pushing this PR forward. By now, we have waited long enough that I feel it's fair to continue with this PR.

I haven't done a full review yet, but it seems that tests are failing. Could you please take a look?

[kallewoof]

@BenjaminBossan Thanks. I think I addressed the errors.

[BenjaminBossan]

We get an error in Python 3.8 as we're missing a from __future__ import annotations.

[kallewoof]

Thanks, fixed. Also added a license header.

[BenjaminBossan]

Thanks for the updates. No in-depth review yet, but I found a couple of issues that should be quick to address.

[kallewoof]

Failing tests appear unrelated to the PR.

[BenjaminBossan]

Thanks for continuing the work on LoRA+. I found a few areas for improvement, but overall it looks good already. Tested it on a small example and results were slightly improved.

[BenjaminBossan]

@kallewoof LMK when this is ready for another review.

[kallewoof]

@BenjaminBossan Should be ready! Sorry if I missed anything.

[BenjaminBossan]

Fantastic, I think we're almost done. I only have two small comments left, the rest looks good.

[BenjaminBossan]

@kallewoof Please ping me once this is ready for review. Otherwise, I don't know if you're still working on some changes or not :)

[kallewoof]

@BenjaminBossan Ping! Sorry about all the force-pushes.

[kallewoof]

@BenjaminBossan Sorry, I thought about this overnight and I think you're right that weight_decay should be popped. If someone provides a weight decay meant for something other than LoRA+ then it will be picked up by both, which is most likely undesired.

[kallewoof]

Playing around with this, I noticed that LoraPlusConfig is actually not used anywhere. I think the code is missing a part where you create a LoraConfig with lora+ feature and it initializes the optimizer and such.

[BenjaminBossan]

Sorry about all the force-pushes.

Not a big deal with this PR, but especially on bigger ones they're better to avoid to make reviews easier. Note that there is no need to clean up the git history, if that was what you're going for, as we squash before merging.

Sorry, I thought about this overnight and I think you're right that weight_decay should be popped. If someone provides a weight decay meant for something other than LoRA+ then it will be picked up by both, which is most likely undesired.

What do you mean by "both"?

Playing around with this, I noticed that LoraPlusConfig is actually not used anywhere.

Hmm, yes, you're right. How about removing it completely then? I guess an argument could be made that something like this API could be useful:

from peft import LoraPlusConfig

optimizer_config = LoraPlusConfig(...)
optimizer = create_loraplus_optimizer(model, optimizer_config)

to make it easier to share the config settings, but IMO the value is very marginal.

[kallewoof]

Sorry about all the force-pushes.

Not a big deal with this PR, but especially on bigger ones they're better to avoid to make reviews easier. Note that there is no need to clean up the git history, if that was what you're going for, as we squash before merging.

Right. It's a very ingrained habit from other projects where they don't squash.

Sorry, I thought about this overnight and I think you're right that weight_decay should be popped. If someone provides a weight decay meant for something other than LoRA+ then it will be picked up by both, which is most likely undesired.

What do you mean by "both"?

Imagine FutureOptimizerX which has a fancy weight_decay parameter that does something, and now imagine someone attaching a LoRA+ optimizer to it:

def create_loraplus_optimizer(
    model: PeftModel, optimizer_cls: type[Optimizer], *, lr: float, loraplus_lr_ratio: float, **kwargs
) -> Optimizer:

The LoRA+ optimizer picks out and uses the passed-in weight decay value in its setup. Then, if we don't pop it,

    optimizer = optimizer_cls(optimizer_grouped_parameters, **kwargs)

the FutureOptimizerX optimizer will now also use our weight decay param. It is unclear which optimizer the user was referring to for the weight decay, but it's probably unlikely that they intended for both FutureOptimizerX and LoRA+ to get the same weight decay with the same value.

Maybe I'm overcomplicating this?

Playing around with this, I noticed that LoraPlusConfig is actually not used anywhere.

Hmm, yes, you're right. How about removing it completely then? I guess an argument could be made that something like this API could be useful:

from peft import LoraPlusConfig

optimizer_config = LoraPlusConfig(...)
optimizer = create_loraplus_optimizer(model, optimizer_config)

to make it easier to share the config settings, but IMO the value is very marginal.

I think the cleanest approach is to remove it in this PR and then make a follow-up where we make it easier to use, if necessary.

Ultimately, without some tweaks to transformers, I don't think we can do this automatically e.g. from get_peft_model because we need to actually access the Trainer. I think.

Edit: We probably should add an example to the docs on how to use it though, at least. Let me look into that.

[kallewoof]

@BenjaminBossan Sounds good. I rebased on main. If it is easier I will merge instead in the future.

[BenjaminBossan]

Thanks everyone involved in this PR, good work.