Skip to content
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

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Conversation

younesbelkada
Copy link
Contributor

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

@@ -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)
Copy link
Collaborator

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.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 24, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Collaborator

@sgugger sgugger left a 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")
Copy link
Collaborator

@ydshieh ydshieh Aug 24, 2023

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thanks!

@younesbelkada younesbelkada merged commit 70b49f0 into huggingface:main Aug 24, 2023
21 checks passed
@younesbelkada younesbelkada deleted the fix-peft-version branch August 24, 2023 10:09
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
* fix peft version

* address comments

* adapt suggestion
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
* fix peft version

* address comments

* adapt suggestion
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 18, 2023
* fix peft version

* address comments

* adapt suggestion
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.

4 participants