Add capability to set a default device for all threads #699
Conversation
icicle/src/device_api.cpp
Outdated
| eIcicleError set_default_device(const Device& dev) | ||
| { | ||
| if (!is_device_registered(dev.type)) { | ||
| THROW_ICICLE_ERR( |
There was a problem hiding this comment.
Why not return an error and do nothing? instead of crashing
There was a problem hiding this comment.
I assume that the user doesn't have a set_device on each thread so returning an error here will result on making the program run on cpu instead of what the user wanted
There was a problem hiding this comment.
Then he should not ignore errors. But ok
There was a problem hiding this comment.
I prefer not throwing here and allow the user to decide to terminate or fallback to CPU as the original default.
There are other areas in our code I think we should make this change but I was following what we did elsewhere
| THROW_ICICLE_ERR( | ||
| eIcicleError::INVALID_DEVICE, "Device type " + std::string(dev.type) + " has not been registered"); | ||
| } | ||
| m_default_device = dev; |
There was a problem hiding this comment.
note that this is not thread safe. Not sure if it's a problem though, since users are not supposed to call this from multiple threads.
There was a problem hiding this comment.
I will add docs for this and will mention it there
jeremyfelder
force-pushed
the
feat/set-default-device
branch
from
0604e41 to
e61b6c6
Compare
jeremyfelder
force-pushed
the
feat/set-default-device
branch
from
b6246a0 to
f1dbb8a
Compare
Describe the changes
This PR adds the capability to set a default device for all threads. Setting a default device does not override a thread's explicitly set device