ENH Allow rank/alpha keys to be "fully qualified" #2382
Conversation
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? |
| # 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. |
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.