Fix _merged_adapters bookkeeping desync on partial unfuse_lora(components=...)#14215
Open
ErenAta16 wants to merge 1 commit into
Open
Fix _merged_adapters bookkeeping desync on partial unfuse_lora(components=...)#14215ErenAta16 wants to merge 1 commit into
ErenAta16 wants to merge 1 commit into
Conversation
…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.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #14214.
What does this PR do?
LoraBaseMixin._merged_adapters(backingnum_fused_loras/fused_loras) is a single set shared across the whole pipeline, but merge state itself is tracked per component by PEFT.fuse_lora()/unfuse_lora()both accept acomponentsargument to operate on a subset of_lora_loadable_modules. When the same adapter is fused into multiple components (the default whenfuse_lora()is called with noadapter_names) and later only some of those components are unfused,unfuse_lora()dropped the adapter name from_merged_adaptersunconditionally, even though it's still physically merged into the base weights of the untouched component(s).Fix
Track which adapters got unmerged during the per-component loop, then only drop an adapter from the pipeline-wide
_merged_adaptersset once it's confirmed unmerged in every_lora_loadable_modulescomponent (new_is_adapter_merged_in_any_componenthelper), not just the ones passed to this particular call.Testing
Added
test_fuse_unfuse_partial_components_keeps_merged_adapter_bookkeepingto the sharedPeftLoraLoaderMixinTestsmixin intests/lora/utils.py, so it runs across the pipeline-specific test files that inherit from it (skips pipelines without atext_encoderloadable module, since the scenario needs at least two independent components). Ran it againstStableDiffusionLoRATests:Also ran the full existing fuse/unfuse suite for the same pipeline to check for regressions in the common (full-components) case:
5 passed, 1 skipped (torch_compile variant, unrelated), 1 xpassed (pre-existing marker, unrelated), no regressions.
I verified the underlying bug and the fix directly with the actual
LoraBaseMixin/PeftAdapterMixinclasses and a realpeft.LoraConfigbefore writing the pipeline-level test, in case that's useful: