MNT Make .merged a property #979
Merged
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
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.
|
The documentation is not available anymore as the PR was closed or merged. |
pacman100
approved these changes
Oct 3, 2023
There was a problem hiding this comment.
Thank you @BenjaminBossan for adding this! Good to merge once the conflicts are resolved.
4 tasks
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.
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 themergedattribute, 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
.mergedattribute. In order to keep the same interface, we add a.mergedproperty, which returnsTrueif there are anymerged_adapters.This PR also contains some fixes for IA³ from #976 to ensure that tests pass.
Note: This PR should be updated to include LoHa once #956 is merged.