Skip to content

[PEFT] Fix peft version - #25710

Merged
younesbelkada merged 3 commits into
huggingface:mainfrom
younesbelkada:fix-peft-version
Aug 24, 2023
Merged

[PEFT] Fix peft version#25710
younesbelkada merged 3 commits into
huggingface:mainfrom
younesbelkada:fix-peft-version

Conversation

@younesbelkada

Copy link
Copy Markdown
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

Comment thread src/transformers/utils/peft_utils.py Outdated
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
Copy Markdown
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

HuggingFaceDocBuilderDev commented Aug 24, 2023

Copy link
Copy Markdown

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

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

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")

@ydshieh ydshieh Aug 24, 2023

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@ydshieh ydshieh left a comment

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown

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