ENH Allow rank/alpha keys to be "fully qualified" #2382
Closed
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
See huggingface/diffusers#10808 for context. Right now, if we have a key in rank_pattern or alpha_pattern, it is interpreted as a pattern to be matched against the module names of the model (basically it is an endswith match). The issue with this logic is that we may sometimes have false matches. E.g. if we have a model with modules "foo" and "bar", and if "bar" also has a sub-module called "foo", it is impossible to target just the outer "foo" but not "bar.foo". (Note: It is already possible target only "bar.foo" and not "foo" by using "bar.foo" as key) This PR adds the possibility to indicate to PEFT that a key should be considered to be the "fully qualified" key, i.e. a strict match should be performed instead of a pattern match. For this, users need to prefix the string "FULL-NAME-" before the actual key. In our example, that would be "FULL-NAME-foo". Notice that since the prefix contains "-", there is no possibility that it could accidentally match a valid module name.
|
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. |
|
@sayakpaul I forgot about this PR, is it good from the diffusers point of view? |
sayakpaul
approved these changes
Mar 4, 2025
shethaadit
approved these changes
Mar 4, 2025
githubnemo
reviewed
Mar 7, 2025
Comment on lines
+1058
to
1068
| # handling of a special case: Users can prefix the key in rank_pattern or alpha_pattern with this special prefix to | ||
| # indicate that this key is supposed to be the full name, not a pattern. That way, the key "foo" can be matched | ||
| # without inadvertently matching bar.foo as well. | ||
| for key in pattern_keys: | ||
| if ( | ||
| key.startswith(FULLY_QUALIFIED_PATTERN_KEY_PREFIX) | ||
| and key[len(FULLY_QUALIFIED_PATTERN_KEY_PREFIX) :] == key_to_match | ||
| ): | ||
| return key | ||
|
|
||
| return next(filter(lambda key: re.match(rf".*\.{key}$", key_to_match), pattern_keys), key_to_match) |
There was a problem hiding this comment.
I'm not sure if the introduction of a special prefix is necessary. We're already utilizing regex here, why not use the caret operator to force a full match? The current pattern doesn't allow this but I think it is easily changed.
Proof of concept:
import re
def match(pattern, name):
return re.match(rf".*(^|\.){pattern}$", name)
assert match("foo", "model.foo")
assert not match("foo", "model.bar")
assert not match("foo", "bofoo")
assert not match("foo", "model.bofoo")
assert match("^foo", "foo")
assert not match("^foo", "model.foo")
|
@sayakpaul I have converted this PR to draft to put it on hold for now, given what @githubnemo suggested and the discussion in #2413. Sorry for the inconvenience, but I think it's worth it to invest a bit more time to figure out the best way forward. |
|
Then I guess the |
|
There is no final decision yet, but maybe we can check if the solution proposed above is enough or not.
|
|
I will temporarily remove the conditional branch and settle with what we have in the other branch in |
|
Whatever works best for you is fine with me. |
This was referenced Mar 7, 2025
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this pull request
Mar 11, 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.
BenjaminBossan
added a commit
that referenced
this pull request
Mar 13, 2025
Supersedes #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.
|
Superseded by #2419. |
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.
See huggingface/diffusers#10808 for context.
Right now, if we have a key in
rank_patternoralpha_pattern, it is interpreted as a pattern to be matched against the module names of the model (basically it is anendswithmatch). The issue with this logic is that we may sometimes have false matches. E.g. if we have a model with modules"foo"and"bar", and if"bar"also has a sub-module called"foo", it is impossible to target just the outer"foo"but not"bar.foo". (Note: It is already possible target only"bar.foo"and not"foo"by using"bar.foo"as key)This PR adds the possibility to indicate to PEFT that a key should be considered to be the "fully qualified" key, i.e. a strict match should be performed instead of a pattern match. For this, users need to prefix the string
"FULL-NAME-"before the actual key. In our example, that would be"FULL-NAME-foo".Notice that since the prefix contains
"-", there is no possibility that it could accidentally match a valid module name.For now, this is a draft PR, until we can verify that it is sufficient to solve the original diffusers issue.