Repository navigation
Require peft>=0.13.0 and drop the 0.12 version guards - #7102
Conversation
autocast_adapter_dtype was introduced in PEFT 0.12.0, so the floor makes the guard dead code in all 13 places it appeared.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 960a3f8e31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and not _is_quantized_model | ||
| and Version(peft.__version__) >= Version("0.12.0") | ||
| ): | ||
| if args.deepspeed_plugin is not None and args.deepspeed_plugin.zero_stage == 3 and not _is_quantized_model: |
There was a problem hiding this comment.
This makes the PEFT branch unconditional but leaves the 0.12 compatibility comment in BCO and the equivalent CPO, IW-OPD, ORPO, and TPO blocks, while deleting it from all stable trainer copies. These blocks now implement identical logic with different comments despite the repository's explicit requirement that duplicated trainer logic and comments remain aligned; remove the obsolete comment from the experimental copies as well.
AGENTS.md reference: AGENTS.md:L26-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
LGTM, the change is correct and complete. Verified on 70261825.
One thing I would like us to settle separately: the criterion for raising a floor
Not a request to change anything here. It is about the rule the next one of these gets reviewed against.
Every floor bump in the repo so far has been feature-driven, with the required feature named in the title or body: datasets>=4.7.0 for the Json dtype (#5376), kernels>=0.14.0 for trust_remote_code, vllm>=0.11.0 (#5443). This PR adds a second, different rationale: the floor should be no older than two years. I think that rationale is defensible, and it happens to match SPEC 0, the Scientific Python standard that superseded NEP 29 and recommends dropping dependency releases older than 24 months. But it is not written down anywhere, and it is in direct tension with something I wrote myself: in #6447 I stated the opposite rule for bitsandbytes, "Do not bump the floor solely to drop this filter: wait for an independent reason to require 0.50.0."
Why this needs to be written down rather than settled per PR
- Contributors cannot tell in advance whether the work is wanted. #6893 raises the bitsandbytes floor to drop a warning filter, which is exactly what #6447 says not to do, and it has been open for two weeks with no way to resolve it in either direction. That is not the contributor's mistake. There was nothing for them to read.
- Any maintainer should be able to review one of these. Without a written rule, a floor bump needs a judgment call rather than a check, so it queues behind whoever is presumed to hold the position instead of being reviewable by whoever picks it up. A criterion turns the review into something verifiable from the diff.
- The floor is a public promise. It is what users install against, so deciding it case by case means it drifts in whichever direction the most recent PR argued.
The two questions
- When is a bump justified at all? Feature needed, dead guards or shims removed, or calendar staleness. Whichever we pick, a reviewer should be able to check it mechanically instead of forming an opinion per PR.
- Guard or bump? Guards exist so the floor can stay low and newer features light up only on newer installs, so guarding should be the default and a bump should be argued on what it deletes.
I will open an issue with a concrete proposal for both questions so we can discuss it there.
albertvillanova
left a comment
albertvillanova
left a comment
There was a problem hiding this comment.
Opened #7116 for the criterion discussion.
2 participants
I think it's high time to raise the PEFT floor from 0.8.0 (Jan 2024) to 0.13.0 (Sep 2024) and removes the version guards it makes dead.
autocast_adapter_dtypewas added in PEFT 0.12.0. TheVersion(peft.__version__) >= Version("0.12")check appeared in 13 places (7 stable trainers, 5 experimental trainers,experimental/utils.py); all are now unconditional.import peft,packaging.version.Version).Note
Low Risk
Dependency floor bump may break installs pinned to older PEFT; training logic is simplified, not changed, for supported versions.
Overview
Raises the minimum PEFT dependency from
0.8.0to0.13.0inpyproject.toml(both thepeftextra and the bundled dev dependency set).Because
autocast_adapter_dtypeis guaranteed on supported PEFT versions, TRL dropsVersion(peft.__version__) >= 0.12checks across stable and experimental trainers (DPO, GRPO, SFT, KTO, RLOO, reward, distillation, BCO, CPO, ORPO, TPO, IW-OPD) and inexperimental/utils.py. DeepSpeed ZeRO-3 + non-quantized PEFT still passesautocast_adapter_dtype=False; sharded QLoRA (4-bit) still disables autocast without a version gate.Unused
import peft(only needed for__version__) andpackaging.version.Versionimports tied to those guards are removed where applicable.Reviewed by Cursor Bugbot for commit 7026182. Bugbot is set up for automated code reviews on this repo. Configure here.