Use std::exception as the param in engine complete callback #14911
Conversation
|
@mxnet-label-bot add [pr-work-in-progress] |
| @@ -74,15 +74,15 @@ class CallbackOnComplete { | |||
| public: | |||
| // use implicit copy and assign | |||
| /*! \brief involve the callback */ | |||
| inline void operator()(const dmlc::Error* error = nullptr) const { | |||
| inline void operator()(const std::exception* error = nullptr) const { | |||
There was a problem hiding this comment.
Is this to support horovod to handle non dmlc exceptions ?
There was a problem hiding this comment.
No, there is no need for this in Horovod at this moment. dmlc exception is a subclass of std::exception. I feel it is better to be more generic on the interface. Additionally, your recent improvements on exception handling also started to catch a broader range of exceptions using std::exception instead of dmlc::Error.
There was a problem hiding this comment.
Is it possible to add a unit test? For example, purposely performing nd.ones((1,) + extremely_small_number to cause std::stof to fail?
There was a problem hiding this comment.
I changed this place to use this new interface. A lot existing negative test cases have already covered it.
There was a problem hiding this comment.
The link was broken. Do you mind sending the link again?
yuxihu
force-pushed
the
callback_ex
branch
3 times, most recently
from
ee563d0 to
48601d8
Compare
|
@yuxihu Can you please address comments? |
Description
This PR replaces
dmlc::Errorwithstd::exceptionfor the param in engine complete callback functions.Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments