Refuse to hot-swap adapters while they are merged into the base weights - #3590
Refuse to hot-swap adapters while they are merged into the base weights#3590Sravanjangam wants to merge 1 commit into
Conversation
hotswap_adapter only replaces the adapter weights, but a merged adapter has its delta already folded into the base weights. Swapping in that state left the old adapter effectively active (merged forward skips adapter weights), and a later unmerge subtracted the NEW delta from a base containing the OLD delta, silently corrupting the model. Raise a ValueError pointing users to unmerge_adapter() first. Auto-unmerging would be an alternative semantics; raising is the conservative choice and can be revisited. Fixes huggingface#3581 (case 1)
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for working on this fix to prevent hotswapping when there are merged layers. Raising an error instead of unmerging is the right call.
I have a few comments on this PR, please check.
Note that you should always await approval from the maintainers on the corresponding issue before opening the PR. We may close future PRs if there is no prior approval if we get overwhelmed with agent PRs.
| assert len(model.tinylora_v["b"]) == len(model_control.tinylora_v["b"]) == 1 | ||
|
|
||
|
|
||
| class TestHotswapMergedGuard: |
There was a problem hiding this comment.
Please move the tests into the already existing TestHotSwapping class and follow the conventions of the tests there.
| # (see #3581). Refuse to run instead. | ||
| from peft.tuners.tuners_utils import BaseTunerLayer | ||
|
|
||
| merged_layers = [ |
There was a problem hiding this comment.
IMO it's sufficient to check if any layer has merged modules. This breaks the loop early if there is a match. There is no need to report which layer exactly has the merged layer.
| from peft.tuners.tuners_utils import BaseTunerLayer | ||
|
|
||
| merged_layers = [ | ||
| name for name, module in model.named_modules() if isinstance(module, BaseTunerLayer) and module.merged |
There was a problem hiding this comment.
Theoretically, if adapter "foo" is merged but we hotswap adapter "bar", we should be good. As is, we still raise an error though. We should check if the adapter that is being swapped out is the one being merged. Please update the tests accordingly.
hotswap_adapterreplaces only the adapter weights, but a merged adapter has its delta already folded into the base weights. Swapping in that state left the old adapter effectively active (merged forward skips adapter weights), and a later unmerge subtracted the new delta from a base containing the old delta — silently corrupting the model. This was case 1 from the linked issue.Fixes #3581
Details
The low-level
hotswap_adapter_from_state_dictnow scans forBaseTunerLayers with non-empty merge state and raises aValueErrorpointing users tounmerge_adapter()first. Raising is deliberately conservative — auto-unmerging would be an alternative semantic, and we'd be glad to switch if maintainers prefer that.Tests
TestHotswapMergedGuardintests/test_initialization.py: swap-while-merged raises; the same swap on an unmerged model still works end-to-end including a forward pass (guard test fails before, passes after).Test environment: Python 3.12, torch 2.13.0+cpu, transformers 5.15.1, peft @ 5d602fd, ruff 0.16.4 (check + format clean).