Skip to content

fix: preserve explicit PEFT LoRA dtype - #9832

Merged
tastelikefeet merged 1 commit into
modelscope:mainfrom
yaodong-shen:fix/peft-lora-dtype
Aug 3, 2026
Merged

tastelikefeet merged 1 commit into
modelscope:mainfrom
yaodong-shen:fix/peft-lora-dtype

Conversation

@yaodong-shen

Copy link
Copy Markdown
Contributor

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_dtype currently 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 LoraConfig explicitly has lora_dtype, preserving PEFT's default behavior for all other adapters. The existing skipped test_peft_lora_dtype regression 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.py
  • git diff --check on both changed files
  • Full PEFT test requires the repository CI dependencies (torch, peft, ModelScope); they are not installed in this environment.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yaodong-shen

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review and for checking the PEFT implementation details.

I confirmed both points:

  • The _cast_adapter_dtype_origin lookup and hook assignment are outside any try block, so a future PEFT rename/removal fails loudly during patch setup rather than silently disabling the fix.
  • test_peft_lora_dtype uses the default Sbert base (fp32) with an explicit lora_dtype='float16' and asserts the injected/reloaded LoRA weights remain fp16. That mismatch is intentional: the Swift setting is an explicit adapter-dtype request, while the hook only disables PEFT's automatic upcast for configured Swift adapters.

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.

@tastelikefeet
tastelikefeet merged commit 05b5685 into modelscope:main Aug 3, 2026
3 of 4 checks passed

This branch was previously deployed

1 inactive deployment
qoder-review — 6a4783a9 Deployed Aug 1, 2026 by yaodong-shen via qoder-review-external #104
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lora_dtype has no effect with peft backend — LoRA checkpoint always saved as float32

3 participants