-
Notifications
You must be signed in to change notification settings - Fork 237
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
Check for mismatch in device setup in evaluator #287
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.
Looks great! Could you add a few tests to check if the desired behaviour is achieved?
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.
Cool ! Raising an error is definitely ok in ambiguous situations
src/evaluate/evaluator/base.py
Outdated
elif device != model_or_pipeline.device.index: | ||
logger.warning( | ||
f"This pipeline was instantiated on device {model_or_pipeline.device.index} but device={device} was passed to `compute`." | ||
) |
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.
I think it's ok to raise an error is this case as well :)
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.
Done! Also added some tests :)
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.
Thank you !
Per #285, we missed an edge case in the
compute
function for the evaluator where if you pass a pipeline which was already instantiated without a device then it won't run on device even if a device is later passed tocompute
.Alternatives considered were:
So I went with @lhoestq's suggestion to raise an error to force the user to disambiguate.
Tests with: