Repository navigation
fix: preserve explicit PEFT LoRA dtype - #9832
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
ErenAta16
left a comment
There was a problem hiding this comment.
The premise checks out against PEFT rather than just against the symptom. _cast_adapter_dtype says so in its own docstring:
peft 0.19.1
signature: (self, adapter_name: str, autocast_adapter_dtype: bool = True) -> None
A helper method to cast the adapter weights to the correct dtype.
Currently, this only upcasts float16 and bfloat16 to float32.
So an explicitly configured fp16/bf16 lora_dtype really is overridden after the modules are built, and the signature the hook wraps matches exactly, including the default.
The scoping is also right, and this is the part worth stating because it is not obvious from the diff. _cast_adapter_dtype is defined on BaseTuner, not on LoraModel:
defined on: BaseTuner (peft.tuners.tuners_utils)
LoraModel.__mro__: LoraModel -> BaseTuner -> Module -> ABC -> object
so assigning to LoraModel._cast_adapter_dtype shadows the inherited method on the LoRA branch only. Tuners that inherit BaseTuner directly keep PEFT's behaviour untouched, and LoRA variants that subclass LoraModel pick the hook up, which is what you want given lora_dtype is a LoRA-side setting. Gating on getattr(config, 'lora_dtype', None) is not None keeps it to adapters Swift actually configured, so a plain PEFT adapter in the same process is unaffected.
Two things worth a look.
The idempotency guard protects against double-wrapping but not against a silent no-op. if not hasattr(LoraModel, '_cast_adapter_dtype_origin') is the right shape for repeated __new_init__ calls. The failure mode I would want covered is a future PEFT that renames or moves this private method: LoraModel._cast_adapter_dtype_origin = LoraModel._cast_adapter_dtype would raise AttributeError at that point, which is loud and fine, but only if that line is not inside a try. Worth confirming it is not, because a swallowed exception there means the patch quietly stops applying and lora_dtype starts being overridden again with no signal, which is exactly the bug this fixes.
Explicit fp16 against an fp32 base. Disabling the autocast for a configured lora_dtype is correct as an expression of user intent, but it means someone setting lora_dtype='float16' on a model loaded in fp32 now gets an adapter whose dtype differs from the base weights, where before PEFT quietly harmonised them. That is presumably the point, so I would not change it; a line in the docstring saying the cast is skipped entirely rather than adjusted would save the next reader from wondering whether a mismatch is a bug.
Pinning a private API is inherent to the approach and I do not think there is a better option here, but a comment naming the PEFT version this was verified against would give whoever hits a future break a starting point.
|
Thanks for the detailed review and for checking the PEFT implementation details. I confirmed both points:
The hook docstring also documents that the automatic cast is skipped only for explicitly configured Swift LoRA adapters; plain PEFT adapters retain PEFT's default behavior. No additional code change is needed from these observations. |
What does this PR do?
Fixes #9694. When the PEFT backend receives Swift's explicit
lora_dtype(float16/bfloat16), PEFT's post-initialization_cast_adapter_dtypecurrently upcasts those adapter weights back to fp32. This makes the documented Swift option ineffective and produces unnecessarily large checkpoints.This patch hooks that cast only when the Swift
LoraConfigexplicitly haslora_dtype, preserving PEFT's default behavior for all other adapters. The existing skippedtest_peft_lora_dtyperegression test is enabled and now checks both initial injection and reload preserve fp16.Validation
python3 -m py_compile swift/tuners/peft.py tests/tuners/test_peft.pygit diff --checkon both changed filestorch,peft, ModelScope); they are not installed in this environment.