-
Notifications
You must be signed in to change notification settings - Fork 26.3k
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
Fix some is_xxx_available
#23365
Conversation
@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. |
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.
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.
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.
@sgugger Do you have any comment regarding the changes I made in this PR?
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.
I think it's fine as it is. No need to make _is_package_available
more complex since the alternative takes one line.
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.
Thanks for fixing!
* fix * fix --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
* fix * fix --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
* fix * fix --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
What does this PR do?
FYI, after #23163,
is_bs4_available()
andis_faiss_available()
givesFalse
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)