-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Restore original task in test_warning_logs #17985
Conversation
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.
nice catch. cc @sgugger @LysandreJik
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!
# restore | ||
PIPELINE_REGISTRY.register_pipeline(alias, original_task) |
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.
This should be done in a finally
after a try
for the rest, so it doesn't mess other tests when it fails.
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.
It is the last statement in test_warning_logs
. By the rest
, what do you mean?
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.
Oh, I guess I understand. Let me try
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.
so it would be
try:
with ...
finally:
PIPELINE_REGISTRY.register_pipeline(alias, original_task)
self.assertIn(f"{alias} is already registered", cm.out) | ||
finally: | ||
# restore | ||
PIPELINE_REGISTRY.register_pipeline(alias, original_task) |
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.
Is this good 😄 ?
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.
Yup!
Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
What does this PR do?
test_warning_logs
modifiesPIPELINE_REGISTRY
viaPIPELINE_REGISTRY.register_pipeline(alias, {})
, which fails the tests inTextClassificationPipelineTests
.This PR restores the original task at the end of
test_warning_logs
.cc @aarnphm