Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Multiple adapters and modules_to_save #1615

Conversation

BenjaminBossan
Copy link
Member

@BenjaminBossan BenjaminBossan commented Apr 2, 2024

Resolves #1574.

Previously, we had the bug that if we had multiple adapters, some with
modules_to_save and others without, when trying to switch to an adapter
without modules_to_save, the ModulesToSaveWrapper would raise an error
because it cannot find that adapter. Now, when it detects this, it is
just disabled (so it uses the original weight).

Moreover, we had the issue that when we were using classes such as
PeftModelForSequenceClassification, we implicitly added the classifier
layers to model.modules_to_save. However, this would only add a new
ModulesToSaveWrapper instance for the first adapter being initialized.
When initializing a 2nd adapter via add_adapter, this information was
ignored. To fix this, I now update the peft_config.modules_to_save to
explicitly add the classifier layers. This is a departure from how this
worked previously, but I'm couldn't find a better way to ensure that
this bug was fixed (LMK if you have a suggestion).

Finally, there was a bug in add_weighted_adapters when we were merging
multiple adapters with modules_to_save. Previously, when we called
model.add_weighted_adapter, the LoRA weights were merged and a new
ModulesToSaveWrapper was added for the new adapter based on the first
LoraConfig of the two adapters. This ModulesToSaveWrapper is just a copy
of the original weights. Thus, when we switch to the newly merged
adapter, we just use the original weights for modules_to_save. This
doesn't make a lot of sense and is probably surprising for the user.
Now, we raise an error when we detect this to alert the user to this
fact.

Note that when only one of the adapters to be merged has a
modules_to_save, this does not raise an error, instead that module is
being used.

Edit: If this is merged, we should add a note to the next release notes
about possible changes, as this results in a different model for the same
config (though it's a rather specific edge case). We should clarify that the
previous behavior was erroneous and new one is correct.

Partially addresses huggingface#1574

This fixes an issue that occurs when we have multiple adapters with
modules_to_save. Previously, the modules_to_save of the 2nd adapter
which is added with add_adapter was not being honored. This is fixed
now.
@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.

The previous fix was not quite correct, so this commit adds further
fixes.

Previously, we had the bug that if we had multiple adapters, some with
modules_to_save and others without, when trying to switch to an adapter
without modules_to_save, the ModulesToSaveWrapper would raise an error
because it cannot find that adapter. Now, when it detects this, it is
just disabled (so it uses the original weight).

Moreover, we had the issue that when we were using classes such as
PeftModelForSequenceClassification, we implicitly added the classifier
layers to model.modules_to_save. However, this would only add a new
ModulesToSaveWrapper instance for the first adapter being initialized.
When initializing a 2nd adapter via add_adapter, this information was
ignored. To fix this, I now update the peft_config.modules_to_save to
explicitly add the classifier layers. This is a departure from how this
worked previously, but I'm couldn't find a better way to ensure that
this works correctly.

Finally, there was a bug in add_weighted_adapters when we were merging
multiple adapters with modules_to_save. Previously, when we called
model.add_weighted_adapter, the LoRA weights were merged and a new
ModulesToSaveWrapper was added for the new adapter based on the first
LoraConfig of the two adapters. This ModulesToSaveWrapper is just a copy
of the original weights. Thus, when we switch to the newly merged
adapter, we just use the original weights for modules_to_save. This
doesn't make a lot of sense and is probably surprising for the user.
Now, we raise an error when we detect this to alert the user to this
fact.

Note that when only one of the adapters to be merged has a
modules_to_save, this does not raise an error, instead that module is
being used.
@BenjaminBossan
Copy link
Member Author

@pacman100 @younesbelkada I updated this PR after the internal discussion (check the updated PR description).

Copy link
Contributor

@pacman100 pacman100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @BenjaminBossan for fixing various issues related to modeules_to_save and thorough tests for the same! 🚀

@BenjaminBossan BenjaminBossan merged commit 0d283ae into huggingface:main Apr 9, 2024
14 checks passed
@BenjaminBossan BenjaminBossan deleted the fix-multiple-adapters-modules-to-save branch April 9, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'set_adapter()' throws "ValueError: Adapter not found in odict_keys" after 'load_adapter()'
3 participants