[peft] Support key_mapping with PEFT models - #46766
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a PEFT integration edge case where a user-provided key_mapping passed to from_pretrained() was applied to the base model weights but not to the PEFT adapter weights, which could silently result in adapters not loading (leaving LoRA weights at fresh init).
Changes:
- Reuse the
from_pretrained()-computedload_config.weight_mappingwhen loading PEFT adapters, so userkey_mappingaffects adapter weights too. - Add a regression test that rewrites adapter weight keys and verifies that
key_mappingrestores the adapter weights on reload.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/transformers/integrations/peft.py |
Reuses load_config.weight_mapping in load_adapter() to ensure adapter weights undergo the same conversions/renamings as base weights (including user key_mapping). |
tests/peft_integration/test_peft_integration.py |
Adds a regression test ensuring key_mapping is applied to PEFT adapter weights during from_pretrained() reload. |
| # Reuse `from_pretrained`'s `weight_mapping` as recomputing here would drop any user-supplied `key_mapping`. | ||
| weight_conversions = load_config.weight_mapping or get_model_conversion_mapping(self) |
There was a problem hiding this comment.
This was deliberate for safety, e.g. if load_adapter is called outright instead of via from_pretrained and somehow weight_conversions=[], then we should consider still checking get_model_conversion_mapping. It might be unnecessary though, I didn't chase down every scenario.
There was a problem hiding this comment.
I think this is still necessary as we may have cases like in the trainer
transformers/src/transformers/trainer.py
Line 3421 in 1048e9a
Here it is not the case that we pass already converted mappings. But tbh maybe we should fix all calls to pass key mappings in some way or form
|
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. |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Thanks for this PR. The fix generally looks good to me. I tried to understand if we can generally replace get_model_conversion_mapping(self) by load_config.weight_mapping, which would happen with this change; AFAICT, it should be okay, but ideally a maintainer can double-check.
If I have a complaint, it's that the test looks a bit forced with the manual renaming of the keys, I wonder if a more realistic test could not be constructed that follows your report with usage of a nested model.
Yeah, we can probably fully drop the
Oh yeah, it'll be possible to create and upload a tiny-random model that requires
|
Or wouldn't it be enough to adjust the test to create a small custom module: class WrapperModule(nn.Module):
def __init__(self):
super().__init__()
self.model = AutoModelForCausalLM.from_pretrained(model_id).to(torch_device)Or is it not enough if the indirection is at the outer-most level? |
|
I uploaded https://huggingface.co/hf-internal-testing/tiny-random-paligemma-lora-key-mapping, which matches roughly my scenario with https://huggingface.co/vidore/colpali, except with a tiny-random base model instead. The test was also simplified a lot, i.e. just loading it with the required I'm also okay to drop the
|
| # Reuse `from_pretrained`'s `weight_mapping` as recomputing here would drop any user-supplied `key_mapping`. | ||
| weight_conversions = load_config.weight_mapping or get_model_conversion_mapping(self) |
There was a problem hiding this comment.
I think this is still necessary as we may have cases like in the trainer
transformers/src/transformers/trainer.py
Line 3421 in 1048e9a
Here it is not the case that we pass already converted mappings. But tbh maybe we should fix all calls to pass key mappings in some way or form
| f"(expected uniform {expected}, got first values {p.flatten()[:4].tolist()})", | ||
| ) | ||
|
|
||
| def test_peft_load_adapter_applies_user_key_mapping(self): |
There was a problem hiding this comment.
Like I said, would be nice to check the trainer case. There might be more but at least those would be the common cases
|
Edit: I see there's been changes in #46442 on the weight conversion code, but it still carried the same bug as before. I've pulled that work and applied the same fix on it. The below still applies. Apologies for the delay, I totally forgot about this. I'd still like to push this through though, as it would allow me to simplify some model implementations a lot (e.g. avoid or shrink this file: https://huggingface.co/tomaarsen/colpali-hard-v1.1-st/blob/main/modeling_st_colpali.py#L21-L23) I had an agent dig into it and the Trainer actually never reaches this Both Trainer call sites are behind transformers/src/transformers/trainer.py Lines 3405 to 3420 in 3fa92a0 and transformers/src/transformers/trainer.py Lines 3473 to 3482 in 3fa92a0 and that is isinstance(model, (PeftModel, PeftMixedModel)). For a PeftModel, model.load_adapter resolves to PEFT's own peft.peft_model.PeftModel.load_adapter, not PeftAdapterMixin.load_adapter:
That said, the fallback is still necessary, just for a different reason. On testing the Trainer case: since the Trainer routes through PEFT's method, a Trainer test would not exercise this code path at all. What is worth pinning is the no- So I'll keep that fallback. I think this is ready for a final review (once I fix the merge conflict).
|
vasqu
left a comment
There was a problem hiding this comment.
So iiuc then we still keep the fallback - I mentioned the trainer (which likely doesnt have the problem) but we still have other BC related things that would break otherwise
I'm fine with the change then but maybe cc @BenjaminBossan just in case
|
From my understanding, this change should be good from the PEFT perspective.
I agree that this might be the cleaner solution. Right now, it makes no difference compared to the suggested change AFAICT, but maybe it's more robust to future changes. Regarding trainer: IIUC, training a Transformer model with PEFT weights loaded directly (i.e. not a |
|
Understandable. Should we move forward with this fix in the meantime, however? It should simplify some things on my end.
|
|
From my perspective, it would be fine. |
|
Also fine with this, just need to merge with main and recheck that CI doesnt complain 🫡 |
|
This new test might be failing as the file system is read-only: https://github.com/huggingface/transformers/actions/runs/29009986379/job/86091350053?pr=46766
|
|
Ah crap it's about the read-only cache. I think it was fixed on main otherwise we need to ping in slack |
|
This branch was still outdated, I merged from
|
|
Even more unrelated failures 😢 Edit: rerunning CI, let's see if they are flaky (or at least some of them) |
|
The error was now But it seems that this error is always shown when So it's hiding the real error with the catch-all try-except, and surprise-surprise: it's our friend again: So I'll try setting
|
|
run-slow: peft |
|
Trying run slow iirc it does cache afterwards but not sure if it works on peft |
|
P.s. when #47338 is merged, I'll remove the cherry-picked commits, pull from main, and queue the merge for this PR.
|
0af980c to
bad94db
Compare
|
run-slow: peft |
|
Hey, at least the
|
|
Should be ready now, sorry release broke main a bit 😬 |
CI recapDashboard: View test results in Grafana |
* Support key_mapping with PEFT models * Simplify test using uploaded tiny-random PEFT adapter * Add extra edge case test * Use a tmp_cache to avoid the potentially read-only default CI cache dir --------- Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
* Support key_mapping with PEFT models * Simplify test using uploaded tiny-random PEFT adapter * Add extra edge case test * Use a tmp_cache to avoid the potentially read-only default CI cache dir --------- Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
Hello!
What does this PR do?
When a
key_mappingis passed tofrom_pretrainedfor a model that also loads a PEFT adapter, the mapping was applied to the base model weights but silently not to the adapter (LoRA) weights. As a result, an adapter whose keys only line up with the model after the mapping is dropped on load, leaving the LoRA at its fresh/zero init, i.e. the model silently degrades to the untrained base.This PR makes
load_adapterreuse the conversion mapping thatfrom_pretrainedalready computed (which includes the userkey_mapping) instead of recomputing it from scratch.Details
from_pretrainedbuilds the weight conversions once, including any userkey_mapping, and stashes them onload_config.weight_mapping:But
load_adapterignoredload_config.weight_mappingand recomputed the mapping without thekey_mapping:The fix
load_config.weight_mappingalready contains the model's built-in conversions plus the userkey_mapping(and accounts for the quantizer), so the adapter weights now go through the same conversion the base weights did. Theorfallback preserves behaviour for directmodel.load_adapter(...)calls, where nofrom_pretrained-supplied mapping exists.Code Agent Policy
The original bug was spotted when I tried to integrate https://huggingface.co/vidore/colpali into Sentence Transformers using agents. This model was trained with
colpali-engine, whoseColPaliclass wraps PaliGemma under an extramodel.level. Loading the adapter onto a current PaliGemma backbone therefore needskey_mapping={"^model\\.": ""}to strip the wrapper. This works fine for a pure PaliGemma model (e.g. https://huggingface.co/vidore/colpali-v1.2-merged), but not for a PEFT Adapter on a PaliGemma model (e.g. https://huggingface.co/vidore/colpali) as thekey_mappingis ignored. An Agent also helped write the test.Before submitting
Pull Request checks?
to it if that's the case.
Who can review?
@BenjaminBossan @githubnemo