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

Fix some is_xxx_available #23365

Merged
merged 2 commits into from
May 15, 2023
Merged

Fix some is_xxx_available #23365

merged 2 commits into from
May 15, 2023

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented May 15, 2023

What does this PR do?

FYI, after #23163, is_bs4_available() and is_faiss_available() gives False even if they are actually available. This causes some CI errors, in particularly, MarkupLM.

This PR fixes the issue in a quick way. It's better to discuss if we want to enhance the function _is_package_available to be able to handle such edge cases.

(cc. @apbard FYI)

@ydshieh ydshieh changed the title Fix import Fix some is_xxx_available May 15, 2023
@apbard
Copy link
Contributor

apbard commented May 15, 2023

@ydshieh thanks for fixing this and sorry for having introduced these bugs in first place.

@@ -36,6 +36,7 @@
logger = logging.get_logger(__name__) # pylint: disable=invalid-name


# TODO: This doesn't work for all packages (`bs4`, `faiss`, etc.) Talk to Sylvain to see how to do with it better.
Copy link
Contributor

Choose a reason for hiding this comment

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

a possibility could be to split the pkg_name arg in two. one for find_spec and the other for metadata, with the default being equal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sgugger Do you have any comment regarding the changes I made in this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine as it is. No need to make _is_package_available more complex since the alternative takes one line.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 15, 2023

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

@ydshieh ydshieh requested a review from amyeroberts May 15, 2023 10:37
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

@ydshieh ydshieh merged commit 96ae83a into main May 15, 2023
@ydshieh ydshieh deleted the fix_import branch May 15, 2023 12:08
sheonhan pushed a commit to sheonhan/transformers that referenced this pull request Jun 1, 2023
* fix

* fix

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
gojiteji pushed a commit to gojiteji/transformers that referenced this pull request Jun 5, 2023
* fix

* fix

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
* fix

* fix

---------

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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.

5 participants