Skip to content

corrected problematic pip commands generated by _download_additional_modules #715

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.

Sign up for GitHub

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

Merged
lhoestq merged 2 commits into from
Nov 14, 2025
Merged

corrected problematic pip commands generated by _download_additional_modules #715

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
52480c8
corrected problematic pip commands generated by _download_additional_…
Yacklin Nov 6, 2025
8ddcdd1
Merge branch 'main' into correct_problematic_command_in_loading
lhoestq Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/evaluate/loading.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,14 +260,15 @@ def _download_additional_modules(
lib = importlib.import_module(library_import_name) # noqa F841
except ImportError:
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
Comment on lines 262 to +263
Copy link
Member

@lhoestq lhoestq Nov 10, 2025

Choose a reason for hiding this comment

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

this duplicate line doesn't seem to fix anything, can you check again ?

Copy link
Contributor Author

@Yacklin Yacklin Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

!!!!!!!!!!!!!!!!!
Annotation 2025-11-11 092318

Copy link
Contributor Author

@Yacklin Yacklin Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @lhoestq ,

To make sure you can view it clearly, i added some notes for you.
ss_edited

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

Copy link
Member

@lhoestq lhoestq Nov 12, 2025

Choose a reason for hiding this comment

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

Noted thanks, no need for violence in your message.

Will approve and merge your PRs soon

Copy link
Contributor Author

@Yacklin Yacklin Nov 13, 2025

Choose a reason for hiding this comment

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

Thank you so much

library_import_name = "absl-py" if library_import_name == "absl" else library_import_name
library_import_path = "absl-py" if library_import_path == "absl" else library_import_path
needs_to_be_installed.add((library_import_name, library_import_path))
if needs_to_be_installed:
raise ImportError(
f"To be able to use {name}, you need to install the following dependencies"
f"{[lib_name for lib_name, lib_path in needs_to_be_installed]} using 'pip install "
f"{' '.join([lib_path for lib_name, lib_path in needs_to_be_installed])}' for instance'"
f"{' '.join([lib_path for lib_name, lib_path in needs_to_be_installed])}' for instance."
)
return local_imports

Expand Down