-
Notifications
You must be signed in to change notification settings - Fork 6.1k
[lora]feat: use exclude modules to loraconfig. #11806
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. As already discussed, for the final PR, we need a version guard on PEFT, as exclude_modules
was only added in v0.14.0. For the finished PR, there should also be unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good to me.
pipe_cp, _ = self.check_if_adapters_added_correctly( | ||
pipe_cp, text_lora_config=text_lora_config, denoiser_lora_config=denoiser_lora_config | ||
) | ||
denoiser_exclude_modules = self._get_exclude_modules(pipe_cp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I don't fully understand how this test works but that shouldn't be a blocker. If this tests the exact same condition we're facing in the FusionX lora, then it should be good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not the cleanest test this one. I added a description in the test, see if that helps?
Currently, we don't have:
If this tests the exact same condition we're facing in the FusionX lora
I will think of a way to include a test for that, too.
string_to_replace = f"{adapter_name}." if adapter_name else "" | ||
|
||
for name in model_state_dict.keys(): | ||
if string_to_replace: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the if-statement is not needed here because string_to_replace will be an empty string, but no problem keeping as micro optimization
tests/lora/test_lora_layers_wan.py
Outdated
denoiser_lora_config.target_modules = ["to_q", "to_k", "to_v", "out"] | ||
denoiser_lora_config.exclude_modules = ["proj_out"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder: Would this test not pass even without the fix of this PR? When I tested it on main, it passed, even when I commented out the denoiser_lora_config.exclude_modules = ["proj_out"]
line.
We would need an example with something like:
target_modules = ["proj_out"]
exclude_modules = ["foo.proj_out"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. But we will have to then use the Wan VACE model as the current model being tested doesn't meet the module naming criterion we're looking for. I will delete this test case and add it in a future PR then if that sounds good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM.
What does this PR do?
If we try to do:
It prints:
Unfold
Loading adapter weights from state_dict led to missing keys in the model: vace_blocks.0.proj_out.lora_A.default_0.weight, vace_blocks.0.proj_out.lora_B.default_0.weight, vace_blocks.1.proj_out.lora_A.default_0.weight, vace_blocks.1.proj_out.lora_B.default_0.weight, vace_blocks.2.proj_out.lora_A.default_0.weight, vace_blocks.2.proj_out.lora_B.default_0.weight, vace_blocks.3.proj_out.lora_A.default_0.weight, vace_blocks.3.proj_out.lora_B.default_0.weight, vace_blocks.4.proj_out.lora_A.default_0.weight, vace_blocks.4.proj_out.lora_B.default_0.weight, vace_blocks.5.proj_out.lora_A.default_0.weight, vace_blocks.5.proj_out.lora_B.default_0.weight, vace_blocks.6.proj_out.lora_A.default_0.weight, vace_blocks.6.proj_out.lora_B.default_0.weight, vace_blocks.7.proj_out.lora_A.default_0.weight, vace_blocks.7.proj_out.lora_B.default_0.weight.
It happens because
target_modules
inLoraConfig
treats the values intarget_modules
as suffixes. In this case,proj_out
is a part oftarget_modules
which is whyvace_blocks_*
get targeted here (and hence the missing warning).LoraConfig
allows us to specifyexclude_modules
, too. This PR introduces support to add it when initializing theLoraConfig
.@apolinario does it work for you?