Skip to content

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

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!

`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
@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
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