ENH: Extend the regex for rank/alpha pattern #2419
Merged
Conversation
This file contains hidden or 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
Supersedes huggingface#2382 Right now, the regex used to match the keys passed for rank_pattern and alpha_pattern requires that either: 1. The module name is identical to the key 2. The module name having a prefix and then ending on the key This is restrictive, since it doesn't allow to disambiguate between all cases. E.g. if we have a model with these attributes: - model.foo - model.bar.foo We cannot currently target just model.foo. (We can already target only model.bar.foo by passing "bar.foo" as a key to the rank_pattern / alpha_pattern dict). This PR makes it possible to pass "^foo" as a key. This way, model.bar.foo is not targeted, as the key does not start with "foo". As a general rule for users, if they intend to have a full match, they should pass the full name of the module preceded by a ^. This is the least ambigious way. When running the test case with the old code, all the test cases with ^ will fail, which is fine, since ^ was not working anyway. At the same time, all test cases not using ^ pass, which means they are backwards compatible.
|
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. |
Some PEFT methods falsely claimed in docs that they support rank/alpha pattern when they don't, this was just a copy paste error.
For consistency with LoRA
|
@sayakpaul @hlky After some internal discussion, Marian and I came up with this new solution which would replace the initial proposal of having "fully qualified" keys from #2382. With this suggestion, it should be enough to add the |
|
@BenjaminBossan thanks! Posted an update here. TL;DR: works like a charm. |
githubnemo
approved these changes
Mar 13, 2025
|
@sayakpaul it's merged. A new PEFT release will be happening soon (we aim for next week). |
|
Thanks, @BenjaminBossan. Would be nice to get a nudge once that happens. I will get my PR out of DRAFT. |
Guy-Bilitski
pushed a commit
to Guy-Bilitski/peft
that referenced
this pull request
May 13, 2025
Supersedes huggingface#2382 Right now, the regex used to match the keys passed for rank_pattern and alpha_pattern requires that either: 1. The module name is identical to the key 2. The module name having a prefix and then ending on the key This is restrictive, since it doesn't allow to disambiguate between all cases. E.g. if we have a model with these attributes: - model.foo - model.bar.foo We cannot currently target just model.foo. (We can already target only model.bar.foo by passing "bar.foo" as a key to the rank_pattern / alpha_pattern dict). This PR makes it possible to pass "^foo" as a key. This way, model.bar.foo is not targeted, as the key does not start with "foo". As a general rule for users, if they intend to have a full match, they should pass the full name of the module preceded by a ^. This is the least ambigious way. When running the test case with the old code, all the test cases with ^ will fail, which is fine, since ^ was not working anyway. At the same time, all test cases not using ^ pass, which means they are backwards compatible.
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.
Supersedes #2382
Right now, the regex used to match the keys passed for
rank_patternandalpha_patternrequires that either:This is restrictive, since it doesn't allow to disambiguate between all cases. E.g. if we have a model with these attributes:
model.foomodel.bar.fooThen we:
"foo"to therank_pattern/alpha_patterndictmodel.bar.fooby passing"bar.foo"model.fooThis PR makes it possible to pass
"^foo"as a key. This way, onlymodel.foobut notmodel.bar.foois targeted, as the latter's name does not start with"foo".As a general rule for users, if they intend to have a full match, they should pass the full name of the module preceded by a
^. This is the least ambiguous way.When running the test case with the old code, all the test cases with
^will fail, which is fine, since^was not working anyway. At the same time, all test cases not using^pass, which means they are backwards compatible.Some changes related to this PR:
rank_pattern/alpha_patternto make it easier to findrank_patternandalpha_patternrank_pattern/alpha_patterneven though they are not supported. These are probably just copy-paste errors that slipped through review.rank_pattern/alpha_patternthat had syntax errors.rank_pattern/alpha_pattern.