Skip to content

Conversation

@Yacklin
Copy link
Contributor

@Yacklin Yacklin commented Nov 6, 2025

This PR removes two comments that pollute variable library_import_path in _download_additional_modules.

Before changes, when error message generated by _download_additional_modules prompts the user to install nltk, rouge_score and absl (wrong name, it should be called "absl-py"), the error message prints out the comments from metrics/rouge/rouge.py, rather than actual lib_path. Please see the image below.
polluted_error_messages

"Here to have a nice missing dependency error message" is stored in lib_path and it is printed out.
These two "Here to have a nice missing dependency error message" are from metrics/rouge/rouge.py. Remove these two comments can solve the issues.
Also, pip install absl, as advised by error message, would result in error. It should be "absl-py"

After this PR, the error message would output correct pip commands that could be used by users directly and provide information they need.

@lhoestq
@sgugger

@Rocketknight1
(sorry to bother you again, this pr is also important. I would like to let you know if other contributors in this repo are not able to review my pr)

…itional_modules and correct import_library_name for "absl"
@Yacklin
Copy link
Contributor Author

Yacklin commented Nov 11, 2025

Hi @Rocketknight1 ,

I just realized that the person who reviewed my PRs to this repo is NOT maintainer of evaluate. He is the maintainer of datasets.

(and he seems to be unfamiliar with source code of this repo..........)

Since it seems to be no formal team responsible for maintenance of evaluate, i thought it should be okay to let other members review PRs.

There are so many issues i found for this repo and i am planning to open more PRs. I would like to make this repo better. Can you review PRs for me? I appreciate it. Thank you so much in advance!

@lhoestq
Copy link
Member

lhoestq commented Nov 12, 2025

Hi, I'm the maintainer and familiar with the code, let me know if I missed anything in your PR that makes you think otherwise

(and a little respect please)

@Rocketknight1
Copy link
Member

lol

@Yacklin
Copy link
Contributor Author

Yacklin commented Nov 13, 2025

@lhoestq Okay i apologize.

These two "Here to have a nice missing dependency error message early on" would also pollute the error messages when user didn't install these two modules. You could consider to remove these two comments but it is not in the scope of this Pull Request.
image
Just to let you know.
I think all of what you need can be found in this Pull Request.

If you need changes, feel free to tag me.

@Yacklin
Copy link
Contributor Author

Yacklin commented Nov 14, 2025

Hi @lhoestq , you can close this PR because PR #717 seems to be much better than mine on this issue.

Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

lgtm ! I'll merge both to account for both contributions

@lhoestq lhoestq merged commit 91453fd into huggingface:main Nov 14, 2025
@Yacklin Yacklin deleted the remove-comments-that-pollutes-error-messages-and-correct-dependency-name branch November 14, 2025 14:46
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.

3 participants