-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Fix _merged_adapters bookkeeping desync on partial unfuse_lora(components=...) #14215
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
base: main
Are you sure you want to change the base?
Changes from all commits
0bb5845
cacdeee
f6c54bc
e11c893
b52aa28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1711,6 +1711,46 @@ def test_simple_inference_with_text_lora_denoiser_fused_multi( | |
| pipe.unfuse_lora(components=self.pipeline_class._lora_loadable_modules) | ||
| self.assertTrue(pipe.num_fused_loras == 0, f"{pipe.num_fused_loras=}, {pipe.fused_loras=}") | ||
|
|
||
| def test_fuse_unfuse_partial_components_keeps_merged_adapter_bookkeeping(self): | ||
| """ | ||
| `_merged_adapters` (backing `num_fused_loras`/`fused_loras`) is shared across the whole pipeline, | ||
| while actual merge state is tracked per component by PEFT. Fusing an adapter into every loadable | ||
| component and then unfusing only *some* of them should not report the adapter as fully unfused, | ||
| it's still merged into the untouched component(s). | ||
| """ | ||
| if not self.supports_text_encoder_loras: | ||
| pytest.skip("Skipping test as text encoder LoRAs are not currently supported.") | ||
|
|
||
| components, text_lora_config, denoiser_lora_config = self.get_dummy_components() | ||
| pipe = self.pipeline_class(**components) | ||
| pipe = pipe.to(torch_device) | ||
| pipe.set_progress_bar_config(disable=None) | ||
|
|
||
| pipe.text_encoder.add_adapter(text_lora_config, "adapter-1") | ||
| self.assertTrue(check_if_lora_correctly_set(pipe.text_encoder), "Lora not correctly set in text encoder") | ||
|
|
||
| denoiser = pipe.transformer if self.unet_kwargs is None else pipe.unet | ||
| denoiser_component_name = "unet" if self.unet_kwargs is not None else "transformer" | ||
| denoiser.add_adapter(denoiser_lora_config, "adapter-1") | ||
| self.assertTrue(check_if_lora_correctly_set(denoiser), "Lora not correctly set in denoiser.") | ||
|
|
||
| pipe.fuse_lora(components=["text_encoder", denoiser_component_name], adapter_names=["adapter-1"]) | ||
| self.assertTrue(pipe.num_fused_loras == 1, f"{pipe.num_fused_loras=}, {pipe.fused_loras=}") | ||
|
|
||
| # Only unfuse the text encoder; the denoiser still has "adapter-1" merged into its base weights. | ||
| pipe.unfuse_lora(components=["text_encoder"]) | ||
| self.assertTrue( | ||
| pipe.num_fused_loras == 1, | ||
| f"adapter-1 is still merged into {denoiser_component_name}, but num_fused_loras dropped to " | ||
| f"{pipe.num_fused_loras=}, {pipe.fused_loras=}", | ||
| ) | ||
| self.assertIn("adapter-1", pipe.fused_loras) | ||
|
|
||
| # Now unfuse the remaining component; bookkeeping should correctly drop to 0. | ||
| pipe.unfuse_lora(components=[denoiser_component_name]) | ||
| self.assertTrue(pipe.num_fused_loras == 0, f"{pipe.num_fused_loras=}, {pipe.fused_loras=}") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For completeness, I would also check "adapter-1" is not in
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in f6c54bc - now also asserts adapter-1 is out of fused_loras once it's unfused from every component. |
||
| self.assertNotIn("adapter-1", pipe.fused_loras) | ||
|
|
||
| def test_lora_scale_kwargs_match_fusion(self, expected_atol: float = 1e-3, expected_rtol: float = 1e-3): | ||
| attention_kwargs_name = determine_attention_kwargs_name(self.pipeline_class) | ||
|
|
||
|
|
||
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.
Why
issubclass(model.__class__, ...)and notisinstance(model, ...)?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.
That's copied from the existing pattern right above it in the same unfuse_lora loop (and used throughout this file, 16 occurrences), not a deliberate choice on my part - happy to switch to isinstance() here if you'd rather not perpetuate it, but left it matching the surrounding code for now.
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 see. IMO,
isinstancemakes more sense but maybe I'm missing something. I'll leave that decision up to the Diffusers maintainers.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.
Sounds like the consensus is to leave it as-is for now and revisit library-wide later, so no change here.