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

Use logging.warning instead of warnings.warn in pipeline.__call__ #29717

Merged
merged 2 commits into from Mar 19, 2024

Conversation

tokestermw
Copy link
Contributor

What does this PR do?

Use HF logging instead of warnings.warn for the following warning:

You seem to be using the pipelines sequentially on GPU. In order to maximize efficiency please use a dataset

When using HF pipeline for inference, this warning is shown a lot, and logging can get expensive. Moving to HF logging, then we can control using transformers.logging.set_loglevel instead of the warnings package which applies to all libraries using the warnings package.

There is a heuristic here #26527. Not quite sure if this applies in this case.

What does that mean for developers of the library? We should respect the following heuristic:
 - `warnings` should be favored for developers of the library and libraries dependent on `transformers`
 - `logging` should be used for end-users of the library using it in every-day projects

Maybe another solution is to warn just once.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@LysandreJik

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 adding this! Looks OK to me - suggestion to use warning_once instead

@@ -1164,7 +1164,7 @@ def __call__(self, inputs, *args, num_workers=None, batch_size=None, **kwargs):

self.call_count += 1
if self.call_count > 10 and self.framework == "pt" and self.device.type == "cuda":
warnings.warn(
logger.warning(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend using warning_once here. This way, the warning will only display the first time it's hit, so shouldn't spam the logs. This will hopefully discourage having to change the logging level to get it to disappear (who knows what other useful warnings you might be surpressing! :D )

Suggested change
logger.warning(
logger.warning_once(

@amyeroberts amyeroberts merged commit 484e10f into huggingface:main Mar 19, 2024
20 checks passed
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.

None yet

2 participants