-
Notifications
You must be signed in to change notification settings - Fork 518
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
Add OMPException class and use it for Text Parser #445
Conversation
@anirudh2290 Can you use this for |
@hcho3 yes you should be able to use it and i am using it in the related PR for mxnet https://github.com/apache/incubator-mxnet/pull/12051/files |
@hcho3 does that answer your question ? is this PR good to go ? |
@anirudh2290 Let me review it today. |
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.
Overall, this pull request is nicely done. It will be useful for other projects as well, such as XGBoost. Let me know what you think about my comment.
src/data/text_parser.h
Outdated
parser_exception_ = std::current_exception(); | ||
} | ||
} | ||
omp_exc_.Run([=] { |
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'm a bit wary about the default value capture here. For example, is it cheap to copy chunk
?
I think we should stick to capture by reference [&]
, since there is no danger of the lambda outliving the captured objects.
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.
good point. you are right it should be by reference. i fixed it at other places in mxnet code but missed it here.
LGTM |
* Add OMPException class and use it for Text Parser * Fix lint * Add documentation for Rethrow and Run * Fix lint * Add documentation for OMPException * Add more documentation * Fix comments * Change capture by value to capture by reference
Adding a class for OMPException since this needs to be handled in TextParser in dmlc-core and also the ImageDetRecordIOParser, ImageRecordIOParser and other parsers that maybe added in future in MXNet.