FIX: issues with (un)merging multiple LoRA and IA³ adapters #976
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This should resolve the failing slow test test_4bit_merge_and_disable_lora. While investigating, I also noticed that merging multiple adapters was not correct for IA³. I added a test that should catch this bug and provided a fix for it too. However, the test does not check IA³ at the moment because the test parameters do not contain IA³. For this, huggingface#972 needs to be merged too, which adds IA³ to the test parameters.
|
The documentation is not available anymore as the PR was closed or merged. |
Previously, tests had some exploding gradients, making them unstable.
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this pull request
Sep 29, 2023
As discussed internally. After adding support for multiple active adapters, we have some double bookkeeping when it comes to tracking merged adapters. On the one hand, we have merged_adapters, which lists all merged adapters, and on the other hand, we have the merged attribute, which indicates if at least one adapter is merged. Having two sources of truth is bad, because it's more work to keep them in sync and there is a risk of them getting out of sync. Therefore, this PR removes the .merged attribute. In order to keep the same interface, we add a merged property, which returns True if there are any merged_adapters. This PR also contains some fixes from huggingface#976 to ensure that tests pass.
pacman100
approved these changes
Oct 3, 2023
There was a problem hiding this comment.
Thank you @BenjaminBossan for the quick fixes and related tests, LGTM!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should resolve the failing slow test
test_4bit_merge_and_disable_lora(see e.g. here).While investigating, I also noticed that merging multiple adapters was not correct for IA³. I added a test that should catch this bug and provided a fix for it too. However, the test does not check IA³ at the moment because the test parameters do not contain IA³. For this, #972 needs to be merged too, which adds IA³ to the test parameters.
Note: I did some small fixes on the tests to avoid exploding/vanishing gradients, as they made it difficult to find the right tolerance for comparing outputs.