Add a regression test for partial unfuse_lora bookkeeping by ErenAta16 · Pull Request #14215 · huggingface/diffusers

ErenAta16 · 2026-07-17T16:21:01Z

What this is now

The fix this PR originally carried is no longer needed: an equivalent one has landed on main. unfuse_lora now collects what is still merged across every loadable component and intersects, rather than dropping an adapter on the first component it unmerges from (src/diffusers/loaders/lora_base.py):

# Only remove an adapter from _merged_adapters once it is no longer
# physically merged in any remaining loadable component.
remaining_merged: set[str] = set()
for component_name in self._lora_loadable_modules:
    component_model = getattr(self, component_name, None)
    if isinstance(component_model, nn.Module):
        for module in component_model.modules():
            if isinstance(module, BaseTunerLayer):
                remaining_merged.update(module.merged_adapters)
self._merged_adapters = self._merged_adapters & remaining_merged

So I have dropped my implementation and reduced this to the regression test. It is now a single file, +40/-0.

Why the test is still worth having

Nothing in the suite exercises a partial unfuse. Every unfuse_lora call in tests/lora/utils.py passes the full set:

utils.py:954    pipe.unfuse_lora(components=self.pipeline_class._lora_loadable_modules)
utils.py:1688   pipe.unfuse_lora(components=self.pipeline_class._lora_loadable_modules)
utils.py:1711   pipe.unfuse_lora(components=self.pipeline_class._lora_loadable_modules)

With all components unfused at once, the old code and the new code agree, so the bug was invisible to the suite and would be invisible to it again. test_fuse_unfuse_partial_components_keeps_merged_adapter_bookkeeping fuses an adapter into the text encoder and the denoiser, unfuses only the text encoder, and asserts that num_fused_loras is still 1 and that the adapter is still in fused_loras, then unfuses the denoiser and asserts it drops to 0.

How far I verified it

I could not run the LoRA suite on this machine, so this is not a full green run and I am not claiming one. What I did do is transcribe the bookkeeping block above verbatim and drive it with stub components standing in for BaseTunerLayer, to check the assertions match what the current implementation produces:

after fuse on both           num_fused_loras = 1
after unfuse(text_encoder)   num_fused_loras = 1   fused = ['adapter-1']
after unfuse(transformer)    num_fused_loras = 0   fused = []

That is the sequence the test asserts. I also checked that everything it depends on is still present in tests/lora/utils.py on main (supports_text_encoder_loras, check_if_lora_correctly_set, get_dummy_components, unet_kwargs, num_fused_loras, fused_loras), and it is inserted at the same place as before, between test_simple_inference_with_text_lora_denoiser_fused_multi and test_lora_scale_kwargs_match_fusion.

The branch has been merged up to main (360bef80) and is mergeable again.

If you would rather not carry a test for an already-fixed bug, closing this is a perfectly good outcome and I will not take it as a loss.

A practical note

My account cannot post comments on this repository, so updates from me arrive as description edits and commits rather than replies.

…ents=...) _merged_adapters is a single set shared across the whole pipeline, while actual merge state is tracked per component by PEFT. When the same adapter is fused into multiple components (the default when fuse_lora() is called with no adapter_names) and later only some of those components are unfused via unfuse_lora(components=[...]), the old code removed the adapter name from _merged_adapters unconditionally, even when it was still physically merged into the base weights of the untouched component(s). num_fused_loras/fused_loras would then report the pipeline as having nothing (or less) fused than it actually does. Track unmerge candidates during the per-component unmerge loop, then only drop an adapter from the pipeline-wide set once confirmed unmerged in every _lora_loadable_modules component via the new _is_adapter_merged_in_any_component helper. Also adds a regression test (test_fuse_unfuse_partial_components_keeps_merged_adapter_bookkeeping in tests/lora/utils.py) covering the partial-components case, since the existing fuse/unfuse tests only ever pass every loadable component at once. Fixes huggingface#14214.

sayakpaul · 2026-07-18T02:56:23Z

Sign in to view

+        if "text_encoder" not in self.pipeline_class._lora_loadable_modules:
+            return


We should use skip rather than returning so that the test properly gets skipped.

sergereview

🤗 Serge says:

The core fix in lora_base.py is correct and well-reasoned: tracking unmerge_candidates and only dropping an adapter from the pipeline-wide _merged_adapters once _is_adapter_merged_in_any_component confirms it's gone from every loadable component correctly resolves the partial-unfuse_lora bookkeeping desync described in #14214. The None-guard from the old code (if adapter and ...) is safely subsumed by the if adapter not in self._merged_adapters: continue filter, so no regression there.

Tests

The new test uses a bare if "text_encoder" not in ... : return, which makes the test report as passed (green) on pipelines without a text encoder rather than skipped. The established convention in this file is the supports_text_encoder_loras flag plus pytest.skip(...) (see lines 385, 521). Using that keeps the skip visible and consistent, and correctly reflects pipelines that carry a text_encoder module but don't support text-encoder LoRAs.

Style (minor)

_is_adapter_merged_in_any_component has a single caller. Per the repo's coding-style guidance (inline single-use private helpers), this could be inlined into unfuse_lora, though the helper is self-contained and the current form is readable — not blocking.

No correctness or security concerns; the description's claims match the diff.

serge v0.1.0 · model: claude-opus-4-8 · 12 LLM turns · 11 tool calls · 59.1s · 180773 in / 3307 out tokens

sergereview · 2026-07-18T02:59:17Z

Sign in to view

+        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 "text_encoder" not in self.pipeline_class._lora_loadable_modules:


Prefer the existing skip convention over a bare return. As written, this test reports as passed for pipelines that don't have a text_encoder loadable module, which hides the fact that it never exercised the scenario. The rest of this file gates on the supports_text_encoder_loras flag with pytest.skip(...) (e.g. lines 385, 521), which is also more accurate — a pipeline can have a text_encoder module but not support text-encoder LoRAs.

Suggested change

if "text_encoder" not in self.pipeline_class._lora_loadable_modules:

if not self.supports_text_encoder_loras:

pytest.skip("Skipping test as text encoder LoRAs are not currently supported.")

Matches the established skip pattern used elsewhere in this file instead of a silent early return, per review feedback.

BenjaminBossan

Generally looks good and makes sense. Just some small comments.

unfuse_lora's existing docstring doesn't mention that _merged_adapters/num_fused_loras is pipeline-wide rather than per-component

Yes, that's worth documenting.

BenjaminBossan · 2026-07-18T09:13:26Z

Sign in to view

+        per-component PEFT merge state after a partial `unfuse_lora(components=...)` call."""
+        for component in self._lora_loadable_modules:
+            model = getattr(self, component, None)
+            if model is None or not issubclass(model.__class__, (ModelMixin, PreTrainedModel)):


Why issubclass(model.__class__, ...) and not isinstance(model, ...)?

I see. IMO, isinstance makes more sense but maybe I'm missing something. I'll leave that decision up to the Diffusers maintainers.

BenjaminBossan · 2026-07-18T09:16:46Z

Sign in to view

+
+        # 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=}")


For completeness, I would also check "adapter-1" is not in pipe.fused_loras.

… test - unfuse_lora docstring now notes num_fused_loras/fused_loras are tracked pipeline-wide, not per component. - Regression test now also asserts the adapter is absent from fused_loras after it's been unfused from every component.

BenjaminBossan

Thanks for the changes, just one nit.

BenjaminBossan · 2026-07-18T09:46:52Z

+        per-component PEFT merge state after a partial `unfuse_lora(components=...)` call."""
+        for component in self._lora_loadable_modules:
+            model = getattr(self, component, None)
+            if model is None or not issubclass(model.__class__, (ModelMixin, PreTrainedModel)):


I see. IMO, isinstance makes more sense but maybe I'm missing something. I'll leave that decision up to the Diffusers maintainers.

BenjaminBossan · 2026-07-18T09:48:45Z

Sign in to view

                Whether to unfuse the text encoder LoRA parameters. If the text encoder wasn't monkey-patched with the
                LoRA parameters then it won't have any effect.
+
+        Note that `num_fused_loras`/`fused_loras` (backed by `self._merged_adapters`) are tracked pipeline-wide,


Let's not mention implementation details here, as those don't matter to the user. It's sufficient to mention the part about fused_loras, as this is what's exposed to the uesr.

I think it's fine to keep it as is and it can be replaced library-wide to fix on a common pattern.

sayakpaul

Thanks for your work!

sayakpaul · 2026-07-18T09:58:17Z

@bot /style

github-actions · 2026-07-18T09:59:35Z

Style bot fixed some files and pushed the changes.

HuggingFaceDocBuilderDev · 2026-07-18T10:01:13Z

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.

Drop the _merged_adapters implementation detail per review feedback; fused_loras is what's actually exposed to callers.

github-actions Bot added fixes-issue lora tests size/M PR with diff < 200 LOC and removed fixes-issue labels Jul 17, 2026

sayakpaul reviewed Jul 18, 2026

View reviewed changes

sergereview Bot reviewed Jul 18, 2026

View reviewed changes

sayakpaul requested a review from BenjaminBossan July 18, 2026 02:59

This comment was marked as spam.

Sign in to view

Use pytest.skip convention for unsupported text-encoder LoRA case

cacdeee

Matches the established skip pattern used elsewhere in this file instead of a silent early return, per review feedback.

github-actions Bot added the fixes-issue label Jul 18, 2026

BenjaminBossan requested changes Jul 18, 2026

View reviewed changes

This comment was marked as spam.

Sign in to view

BenjaminBossan requested changes Jul 18, 2026

View reviewed changes

sayakpaul approved these changes Jul 18, 2026

View reviewed changes

Apply style fixes

e11c893

ErenAta16 and others added 4 commits July 18, 2026 15:24

Trim unfuse_lora docstring to user-facing behavior only

b52aa28

Drop the _merged_adapters implementation detail per review feedback; fused_loras is what's actually exposed to callers.

Drop the lora_base fix; an equivalent one landed on main

266daa1

Keep only the partial-unfuse regression test

3466f53

Merge upstream main

a953a48

github-actions Bot added size/S PR with diff < 50 LOC and removed size/M PR with diff < 200 LOC lora labels Aug 19, 2026

		if "text_encoder" not in self.pipeline_class._lora_loadable_modules:
		return

	if "text_encoder" not in self.pipeline_class._lora_loadable_modules:
	if not self.supports_text_encoder_loras:
	pytest.skip("Skipping test as text encoder LoRAs are not currently supported.")

Uh oh!

Conversation

ErenAta16 commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this is now

Why the test is still worth having

How far I verified it

A practical note

Uh oh!

sayakpaul Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

sergereview Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sergereview Bot Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

BenjaminBossan Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

BenjaminBossan Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

sayakpaul Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

sayakpaul left a comment

Choose a reason for hiding this comment

Uh oh!

sayakpaul commented Jul 18, 2026

Uh oh!

github-actions Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HuggingFaceDocBuilderDev commented Jul 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ErenAta16 commented Jul 17, 2026 •

edited

Loading

github-actions Bot commented Jul 18, 2026 •

edited

Loading