Changes are not rolled back when loading an adapter fails #798
Comments
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this issue
Aug 7, 2023
When a user tries to add a 2nd adapter, Lora and AdaLora make some checks to
ensure the new adapter is compatible with existing adapters. Currently, that
check is performed halfway through the method. This means that if the check
fails, the new adapter is partially applied, leaving the model in a bad state.
The main purpose of this PR is to ensure that the model state is correct after
such a failure is encountered.
Tests were added to catch this potential bug.
While working on this, I also did some related, but not strictly necessary
changes to the add_adapter methods:
- So far, PeftModel would initialize the underlying Lora/AdaLora/IA³ model by
passing the nested dict as config (i.e. config={adapter_name:
adapter_config}). This has been changed so that the adapter config is passed
directly (i.e. config=adapter_config) and the Lora/AdaLora/IA³ is now
responsible for adding the adapter config to the config dict. This makes it
easier to initialize Lora/AdaLora/IA³ directly and should also address huggingface#791.
- add_adapter no longer accepts config=None. AFAICT, this possibility was never
used. TBH I'm unsure if it even makes sense. Please LMK if it should be added
back in, and what the purpose is of that option.
- More consistency between Lora/AdaLora/IA³ when adding configs. More code is
reused between Lora and AdaLora, and IA³ is harmonized.
- Add some type annotations
- Extend docstrings to contain adapter_name
I might be lacking some background info to explain some of the choices that were
made here, so please check if my changes make sense and are not breaking
existing code.
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this issue
Aug 7, 2023
When a user tries to add a 2nd adapter, Lora and AdaLora make some checks to ensure the new adapter is compatible with existing adapters. Currently, that check is performed halfway through the method. This means that if the check fails, the new adapter is partially applied, leaving the model in a bad state. The main purpose of this PR is to ensure that the model state is correct after such a failure is encountered. Tests were added to catch this potential bug. While working on this, I also did some related, but not strictly necessary changes to the add_adapter methods: - Previously, the peft_config from the PeftModel was passed to the base model. This meant that sometimes, the base model would hold a reference to PeftModel.peft_config, but not always, as some base models would create new dicts. This is problematic, because some code would rely on the objects being the same. Now, they are never the same, leading to more consistency. - I think that the check if multiple adapters have biases (which is not supported) was accidentally removed by huggingface#749. It is added back in. - Add some type annotations - Extend docstrings to contain adapter_name
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
System Info
Independent of versions.
Who can help?
No response
Information
Tasks
examplesfolderReproduction
See issue #792
Expected behavior
When a user calls
add_adapteron a LoRA model and there is an error (e.g. multiple adapters with biases), the resultingLoraModelis left in a bad state. Instead, when there is an error, the state should be the same as it was before theadd_adaptercall, if possible (if not possible, say so in the error message).The text was updated successfully, but these errors were encountered: