-
Notifications
You must be signed in to change notification settings - Fork 258
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
Evaluator device placement #193
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.
LGTM ✅
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 adding this killer feature @lvwerra - finally evaluations can go brum brum brum 🏎️ !
I've left a few nits and a question about the device
variable being set when neither torch
or tf
are installed (edge case). Otherwise, this LGTM!
except ImportError: | ||
device = -1 | ||
|
||
if device == -1: |
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.
In the rare case that neither torch
or tensorflow
are installed, this will error out because device
is not assigned a value. Would it make sense to have a warning in that case?
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.
See comment in test
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.
Ah sorry I missed that - all good then!
# neither pt or tf are available | ||
pt_available = False | ||
tf_available = False | ||
self.assertEqual(Evaluator._infer_device(), -1) |
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.
Interesting, I wonder how this test passes since my understanding is that device
is never explicitly defined when neither torch
or tf
are installed 🤔
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.
If neither tf or torch are installed then the second except
sets it to -1
:
evaluate/src/evaluate/evaluator/base.py
Lines 149 to 150 in b48e48c
except ImportError: | |
device = -1 |
Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>
This PR ads device placement for the pipeline in the evaluator. There are two mechanisms:
Evaluator.compute
.Alternatively, a user can always use a custom configuration and initialize the pipeline before passing it to the
evaluator
. If the an initialized pipeline is passed the device has no effect.