FIX: Disabling adapter works with modules_to_save #736
Conversation
Resolves huggingface#493 For LoRA and IA³, there was a bug that even even using the disable_adapter context, if the module was listed in modules_to_save, the updated weights would be used instead of the original weights. This meant that disable_adapter would not return the same results as the base model without adaptation. This PR fixes the issue and provides a test. Note: I tried to adjust AdaLoRA too, since it seemed that the same reasoning should apply there. However, I think that AdaLoRA does not really support disabling adapters at all. E.g. there is no disable_adapter_layers method. Therefore, AdaLoRA was not changed.
|
The documentation is not available anymore as the PR was closed or merged. |
src/peft/tuners/ia3.py
Outdated
| @@ -303,6 +303,8 @@ def _set_adapter_layers(self, enabled=True): | |||
| for module in self.model.modules(): | |||
| if isinstance(module, IA3Layer): | |||
| module.disable_adapters = False if enabled else True | |||
| elif isinstance(module, ModulesToSaveWrapper): | |||
| module.disable_adapter = False if enabled else True | |||
There was a problem hiding this comment.
I would be a bit more intuitive to use the same name disable_adapters with a "s" for both IA3Layer and ModulesToSaveWrapper, for homogeneity.
There was a problem hiding this comment.
Otherwise the PR looks good to me, as far as I can tell with my limited experience of peft.
There was a problem hiding this comment.
Thanks for taking a look.
Good point about disable_adapter. I was going with singular since it's only a single adapter, but I can see your argument too. Let's wait what the other reviewers prefer.
There was a problem hiding this comment.
Yeah, let's have disable_adapters for homogeneity.
src/peft/tuners/ia3.py
Outdated
| @@ -303,6 +303,8 @@ def _set_adapter_layers(self, enabled=True): | |||
| for module in self.model.modules(): | |||
| if isinstance(module, IA3Layer): | |||
| module.disable_adapters = False if enabled else True | |||
| elif isinstance(module, ModulesToSaveWrapper): | |||
| module.disable_adapter = False if enabled else True | |||
There was a problem hiding this comment.
Yeah, let's have disable_adapters for homogeneity.
Resolves #493
For LoRA and IA³, there was a bug that even even using the
disable_adaptercontext, if the module was listed inmodules_to_save, the updated weights would be used instead of the original weights. This meant thatdisable_adapterwould not return the same results as the base model without adaptation. This PR fixes the issue and provides a test.Note: I tried to adjust AdaLoRA too, since it seemed that the same reasoning should apply there. However, I think that AdaLoRA does not really support disabling adapters at all. E.g. there is no
disable_adapter_layersmethod. Therefore, AdaLoRA was not changed.