FIX: Bug in target module optimization if child module name is suffix of parent module name #2144
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
Solves the following bug: huggingface/diffusers#9622 (comment) The cause for the bug is as follows: When we have, say, a module called "bar.0.query" that we want to target and another module called "foo_bar.0.query" that we don't want to target, there was potential for an error. This is not caused by _find_minimal_target_modules directly, but rather the bug was inside of BaseTuner.inject_adapter and how the names_no_target were chosen. Those used to be chosen based on suffix. In our example, however, "bar.0.query" is a suffix of "foo_bar.0.query", therefore "foo_bar.0.query" was *not* added to names_no_target when it should have. As a consequence, during the optimization, it looks like "query" is safe to use as target_modules because we don't see that it wrongly matches "foo_bar.0.query".
BenjaminBossan
deleted the
fix-bug-find_minimal_target_modules-parent-name-is-suffix
branch
BenjaminBossan
added a commit
that referenced
this pull request
Oct 11, 2024
Solves the following bug: huggingface/diffusers#9622 (comment) The cause for the bug is as follows: When we have, say, a module called "bar.0.query" that we want to target and another module called "foo_bar.0.query" that we don't want to target, there was potential for an error. This is not caused by _find_minimal_target_modules directly, but rather the bug was inside of BaseTuner.inject_adapter and how the names_no_target were chosen. Those used to be chosen based on suffix. In our example, however, "bar.0.query" is a suffix of "foo_bar.0.query", therefore "foo_bar.0.query" was *not* added to names_no_target when it should have. As a consequence, during the optimization, it looks like "query" is safe to use as target_modules because we don't see that it wrongly matches "foo_bar.0.query".
BenjaminBossan
added a commit
that referenced
this pull request
Oct 11, 2024
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this pull request
Oct 22, 2024
Solves the following bug: huggingface/diffusers#9622 (comment) The cause for the bug is as follows: When we have, say, a module called "bar.0.query" that we want to target and another module called "foo_bar.0.query" that we don't want to target, there was potential for an error. This is not caused by _find_minimal_target_modules directly, but rather the bug was inside of BaseTuner.inject_adapter and how the names_no_target were chosen. Those used to be chosen based on suffix. In our example, however, "bar.0.query" is a suffix of "foo_bar.0.query", therefore "foo_bar.0.query" was *not* added to names_no_target when it should have. As a consequence, during the optimization, it looks like "query" is safe to use as target_modules because we don't see that it wrongly matches "foo_bar.0.query".
Guy-Bilitski
pushed a commit
to Guy-Bilitski/peft
that referenced
this pull request
May 13, 2025
Solves the following bug: huggingface/diffusers#9622 (comment) The cause for the bug is as follows: When we have, say, a module called "bar.0.query" that we want to target and another module called "foo_bar.0.query" that we don't want to target, there was potential for an error. This is not caused by _find_minimal_target_modules directly, but rather the bug was inside of BaseTuner.inject_adapter and how the names_no_target were chosen. Those used to be chosen based on suffix. In our example, however, "bar.0.query" is a suffix of "foo_bar.0.query", therefore "foo_bar.0.query" was *not* added to names_no_target when it should have. As a consequence, during the optimization, it looks like "query" is safe to use as target_modules because we don't see that it wrongly matches "foo_bar.0.query".
cyyever
pushed a commit
to cyyever/peft
that referenced
this pull request
Sep 4, 2025
* extract prompt and apply chat template in cpo trainer * default leanring rate * simplify example * update doc * test all formats * extend exptract prompt * improve doc format * link in dataset formats * Update docs/source/cpo_trainer.mdx Co-authored-by: lewtun <[email protected]> * Update docs/source/cpo_trainer.mdx Co-authored-by: lewtun <[email protected]> --------- Co-authored-by: lewtun <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Solves the following bug:
huggingface/diffusers#9622 (comment)
The cause for the bug is as follows: When we have, say, a module called
"bar.0.query"that we want to target and another module called"foo_bar.0.query"that we don't want to target, there was potential for an error.This is not caused by
_find_minimal_target_modulesdirectly, but rather the bug was inside ofBaseTuner.inject_adapterand how thenames_no_targetwere chosen. Those used to be chosen based on suffix. In our example, however,"bar.0.query"is a suffix of"foo_bar.0.query", therefore"foo_bar.0.query"was not added tonames_no_targetwhen it should have. As a consequence, during the optimization, it looks like"query"is safe to use astarget_modulesbecause we don't see that it wrongly matches"foo_bar.0.query".