-
Notifications
You must be signed in to change notification settings - Fork 26.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PEFT
] Fix peft version
#25710
[PEFT
] Fix peft version
#25710
Conversation
src/transformers/utils/peft_utils.py
Outdated
@@ -91,7 +91,7 @@ def check_peft_version(min_version: str) -> None: | |||
if not is_peft_available(): | |||
raise ValueError("PEFT is not installed. Please install it with `pip install peft`") | |||
|
|||
is_peft_version_compatible = version.parse(importlib.metadata.version("peft")) <= version.parse(min_version) | |||
is_peft_version_compatible = version.parse(importlib.metadata.version("peft")) > version.parse(min_version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the min_version
should be compatible. Using > min_version
looks strange. We can min_version
to use >=
if necessary.
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with @ydshieh comment, thanks!
@@ -113,7 +113,7 @@ def load_adapter( | |||
offload_index (`int`, `optional`): | |||
`offload_index` argument to be passed to `accelerate.dispatch_model` method. | |||
""" | |||
check_peft_version(min_version="0.4.0") | |||
check_peft_version(min_version="0.5.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe set this on top of this file? I don't know if this is going to be changed along time, but if so, a single place of definition is easier for life.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is that we always import the PeftMixin
object, meaning that check would be always called and it will raise an error if PEFT is not installed I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline and got it, will push somethning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
* fix peft version * address comments * adapt suggestion
* fix peft version * address comments * adapt suggestion
* fix peft version * address comments * adapt suggestion
What does this PR do?
Fixes the peft version check, in fact we should check if the current version is strictly greater than the required minimum version, not the opposite. That was leading to failing tests in the nightly CI.
cc @ydshieh