Delete IA3 adapter #1153
Delete IA3 adapter #1153
Conversation
| # All names of other parameters that may contain adapter-related parameters | ||
| other_layer_names = ("scaling",) | ||
|
|
||
| def __init__(self, base_layer: nn.Module, is_feedforward: bool, **kwargs) -> None: | ||
| self.base_layer = base_layer | ||
| self.scaling = {} |
There was a problem hiding this comment.
As far as I understand, we do not use self.scaling and
There was a problem hiding this comment.
Right, thanks for cleaning that up.
There was a problem hiding this comment.
Thank you so much Alejandro for factoring out the deletion-feature and creating this super clean PR. Not only do you show great understanding of the PEFT code base, but also improved some unrelated lines on top, perfect!
I have only a few minor comments, they should be easy to fix and then we're good to merge.
| # All names of other parameters that may contain adapter-related parameters | ||
| other_layer_names = ("scaling",) | ||
|
|
||
| def __init__(self, base_layer: nn.Module, is_feedforward: bool, **kwargs) -> None: | ||
| self.base_layer = base_layer | ||
| self.scaling = {} |
There was a problem hiding this comment.
Right, thanks for cleaning that up.
src/peft/tuners/ia3/model.py
Outdated
| Args: | ||
| adapter_name (str): Name of the adapter to be deleted. | ||
| """ | ||
| if adapter_name not in list(self.peft_config.keys()): |
There was a problem hiding this comment.
| if adapter_name not in list(self.peft_config.keys()): | |
| if adapter_name not in self.peft_config: |
This should be more efficient.
tests/testing_common.py
Outdated
| @@ -905,7 +905,7 @@ def _test_delete_adapter(self, model_id, config_cls, config_kwargs): | |||
| self.assertFalse(adapter_to_delete in model.peft_config) | |||
| self.assertEqual(model.active_adapters, ["default"]) | |||
|
|
|||
| key_list = [key for key, _ in model.named_modules() if "lora" not in key] | |||
| key_list = [key for key, _ in model.named_modules() if model.prefix not in key] | |||
There was a problem hiding this comment.
Good amendment, but this made me curious why the code worked at all, because "lora" was hard-coded but we also test LoHa and LoKr. It turns out the that prefix check is totally unnecessary and the code can be simplified to:
key_list = [key for key, _ in model.named_modules()]
Could you please fix that? Same for _test_delete_inactive_adapter and _test_weighted_combination_of_adapters. Thanks.
There was a problem hiding this comment.
I was also curious about why this worked at all. I'll fix it!
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
alexrs
force-pushed
the
remove-ia3-adapter
branch
from
21e901d to
ca1d1ea
Compare
Thanks! I've spent quite a few hours navigating the codebase for my MSc thesis, and it has helped me a lot. Thanks for the good work! 👏 |
What
As discussed in #980 (specifically #980 (comment)), we are breaking that PR into smaller PRs that can be merged faster.
In this first PR, we include the ability to delete$(IA)^3$ adapters.
How
A new method
delete_adapterhas been added to theIA3Modelclass. This method is based onpeft/src/peft/tuners/lora/model.py
Line 608 in f1ecfa6
Test Plan
PeftType.IA3totesting_common.py#_test_delete_adapter