-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-1357] Fix the cpp-examples to add exception handling #14441
Conversation
@@ -228,6 +228,7 @@ int main(int argc, char const *argv[]) { | |||
} | |||
#endif | |||
|
|||
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.
can we just add exception handling to cpp-package the way it is done in python. Every C API call is inside a check_call function which checks the return code and throws corresponding frontend exception.
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 certainly possible to add exception handling in cpp-package API itself. However, it will require careful design and we may require to change the cpp-package API as well.
We will certainly consider this enhancement as a part of C++ API enhancement project.
@mxnet-label-bot add [C++, Example, Exception Handling, pr-awaiting-response ] |
@mxnet-jenkins The unix-cpu and unix-gpu build failures are not related to the changes in PR. |
try { | ||
#define CATCH \ | ||
} catch(...) { \ | ||
LG << "Status: FAIL";\ |
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.
catch only dmlc::Error. For non backend exceptions MXGetLastError will be empty.
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.
Fixed.
cpp-package/example/utils.h
Outdated
LG << "Status: FAIL";\ | ||
LG << "With Error: " << MXGetLastError(); \ | ||
return 1; \ | ||
} catch(...) { \ |
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 you should only have dmlc::Error catch and remove the catch all. If there is any other exception thrown it could be because of some MXNet backend showing non dmlc::Errors (which is very unlikely) or some other exception thrown from the example code because of user or programming error. Users should be able to see what these exceptions are without having to modify utils.
@anirudh2290 Are you comments addressed now in this PR ? |
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
…4441) * Adding exception handling to the cpp-package examples. * Updating exception handling to catch dmlc::error. * Removed the catch all exception.
* Adding exception handling to the cpp-package examples. * Updating exception handling to catch dmlc::error. * Removed the catch all exception.
…4441) * Adding exception handling to the cpp-package examples. * Updating exception handling to catch dmlc::error. * Removed the catch all exception.
Description
The scope of this PR is to add missing exception handling to the examples. In future, if the underlying MXNet code throws any exception, the example will catch this exception and fail gracefully instead of crashing due to unhandled exception.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments