corrected problematic pip commands generated by _download_additional_modules#715
Conversation
|
I'll leave this one for the |
| library_import_name = "scikit-learn" if library_import_name == "sklearn" else library_import_name | ||
| library_import_path = "scikit-learn" if library_import_path == "sklearn" else library_import_path |
There was a problem hiding this comment.
this duplicate line doesn't seem to fix anything, can you check again ?
There was a problem hiding this comment.
Hi @lhoestq , the first line changes the variable library_import_name, which changes the package name displayed in "dependency['']". The second line changes the library_import_path, which changes the package name displayed in "pip install".
Please carefully view the screenshot i attached in this Pull Request.
To make sure you can see it clearly, please find the attached.
This is
NOT
DUPLICATE
LINE
!!!!!!!!!!!!!!!!!
There was a problem hiding this comment.
Hi @lhoestq ,
To make sure you can view it clearly, i added some notes for you.
In the image above, the "1" is controlled by 'import_library_name' and the "2" (displayed in pip commands) is controlled by 'import_library_path'. If you forget how it works, please read source code carefully.
The library_import_name only changes the package names within 'dependency['']'.
But it doesn't change the package name in "pip install".
So i added a new line (please read it CAREFULLY!!!!!!!!!! THIS IS NOT DUPLICATE LINE!!!!!!!!!!!!!!!!!!)
library_import_path (IT'S PATH!!!!!)
so that the package name, in this context, scikit-learn, could be displayed correctly in "pip install".
There was a problem hiding this comment.
Noted thanks, no need for violence in your message.
Will approve and merge your PRs soon
3 participants
What does this PR do?
(Pattern of PR for transformers is reused)
It corrects problematic pip commands generated by _download_additional_moddules when it prompts the users to install scikit-learn.
Before changes, it would prompt the user to install sklearn (deprecated, now called scikit-learn) with 'pip install sklearn' being displayed. Running 'pip install sklearn' would raise error.
To make it ready-to-use for users, this Pull Request is created.
This Pull Request adds one line (and correct a tiny typo) to make sure library_import_path is set to "scikit-learn" when installing scikit-learn and 'pip install scikit-learn', the correct command, would be displayed.
Who can review?
@lhoestq
@sgugger
@Rocketknight1
(sorry to bother you, but this repo looks less active compared with Transformers. I believe you have write access to all repos in huggingface and you are the first one who approved my pr in transformers, so i would like to let you know.)
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.