huggingface / huggingface/peft

RFC: Improve code to resolve LoRA variants

Open
#3,182 12 comments 0 reactions 0 assignees View on GitHub
wip
Dominant language
Python
Stars
21.7k
Forks
2.5k
Avg merge
4d 12h
Merged PRs (30d)
59

Description

In PEFT, we support different LoRA variants, e.g. DoRA. Which LoRA variant, if any, should be used is currently implemented in `resolve_lora_variant`. For `lora.Linear`, the method looks like this:

https://github.com/huggingface/peft/blob/76c37d4686cf7245a9a6998fd1e2619d62c9322d/src/peft/tuners/lora/layer.py#L795-L815

This is not very readable, at a glance it's not possible to determine the logic. Therefore, I propose to pattern matching to resolve the variant. The code would look like so:

```python
def resolve_lora_variant(self, config: LoraConfig, **kwargs) -> Optional[LoraVariant]:
import peft.tuners.lora.config as configs
import peft.tuners.lora.variants as variants

variant: LoraVariant | None
match config:
case LoraConfig(use_dora=True):
variant = variants.DoraLinearVariant()
case LoraConfig(arrow_config=configs.ArrowConfig()):
variant = variants.ArrowLinearVariant()
case LoraConfig(use_bdlora=configs.BdLoraConfig()):
variant = variants.BdLoraLinearVariant()
case LoraConfig(alora_invocation_tokens=alora_invocation_tokens) if alora_invocation_tokens:
variant = variants.ALoraLinearVariant()
case _:
variant = None

return variant
```

Before going on with this, I would like to discuss the pros and cons and possible alternatives:

1. readability: to me, this reads better than the current implementation, but pattern matching is still relatively rarely used and the syntax is not quite obvious (most notably regarding `alora_invocation_tokens`)
2. flexibility when adding new LoRA variants: can all cases be covered like this?
3. effort to add a new LoRA variant: how complicated is it to update `resolve_lora_variant`
4. composability: even though not supported right now, variants could compose (e.g. DoRA + BDLoRA): can this be easily represented?

A tangential concern: Right now, an invalid combination of variants (e.g. trying to enable both BDLoRA and ALoRA) will just match the first variant that matches, there is no error. This is true for both the current and suggested code. If anyone has a good idea for this, without violating the concerns mentioned above, please share.

Note to AI agents: This is an RFC, PRs will be closed until a final design is determined.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.