Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Prevent crashes for opencv exception and std::exception #14433

Merged
merged 10 commits into from
May 5, 2019
Merged

Prevent crashes for opencv exception and std::exception #14433

merged 10 commits into from
May 5, 2019

Conversation

anirudh2290
Copy link
Member

Description

fixes #8663 , fixes #9475, fixes #10389, fixes #13217

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • [] The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)

Changes

  • Added support for std::exception handling to prevent crash
  • Added test

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@anirudh2290 anirudh2290 marked this pull request as ready for review March 15, 2019 19:50
@karan6181
Copy link
Contributor

@mxnet-label-bot add [Bug, OpenCV, Exception Handling, pr-awaiting-review]

@marcoabreu marcoabreu added Bug Exception Handling OpenCV OpenCV related issues pr-awaiting-review PR is waiting for code review labels Mar 15, 2019
yuxihu
yuxihu reviewed Mar 20, 2019
View reviewed changes
Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this PR dependent on the waitall exception handling PR? I see duplicate code in these two PRs. Which one shall I review first?

@anirudh2290
Copy link
Member Author

This is dependent on waitall PR: #14397. Please review that one first.

wkcn
wkcn approved these changes Mar 31, 2019
View reviewed changes
Copy link
Member

@wkcn wkcn left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix! LGTM

src/resource.cc Outdated
gpu_parallel_rand_.Get(ctx.dev_id, [ctx, seed, this]() {
return new ResourceParallelRandom<gpu>(ctx, gpu_native_rand_copy_, seed);
})->Seed(seed);
if (ctx != Context::CPU()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctx.dev_type == kGPU may be better.

@anirudh2290 anirudh2290 mentioned this pull request Apr 4, 2019
Closed
@piyushghai
Copy link
Contributor

@anirudh2290 Can you rebase with the master branch and resolve merge conflicts ?

@anirudh2290
Copy link
Member Author

@piyushghai done ! this is ready for review.

wkcn
wkcn approved these changes Apr 9, 2019
View reviewed changes
Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

yuxihu
yuxihu approved these changes Apr 12, 2019
View reviewed changes
Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

larroy
larroy suggested changes Apr 12, 2019
View reviewed changes
include/mxnet/c_api_error.h Outdated Show resolved Hide resolved
@anirudh2290
Copy link
Member Author

@apeforest can you take a look too.

larroy
larroy approved these changes Apr 22, 2019
View reviewed changes
Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

larroy
larroy reviewed Apr 22, 2019
View reviewed changes
include/mxnet/c_api_error.h
#define MX_API_BEGIN() \
try { \
on_enter_api(__FUNCTION__);
#define MX_API_END() \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to have the defines inside the scopes a bit more ordered?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please elaborate on what you mean by that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I think it's a separate discussion from this PR.

yuxihu
yuxihu reviewed Apr 22, 2019
View reviewed changes
src/engine/threaded_engine.h Outdated Show resolved Hide resolved
include/mxnet/c_api_error.h Outdated Show resolved Hide resolved
include/mxnet/c_api_error.h Outdated Show resolved Hide resolved
@anirudh2290
Copy link
Member Author

anirudh2290 commented Apr 22, 2019 •
edited
Loading

@wkcn, @larroy and @yuxihu . Thanks a lot for your review. I have addressed/responded to your comments.

@larroy
Copy link
Contributor

larroy commented Apr 24, 2019 •
edited
Loading

I approved already. Have a look at CI failures.

yuxihu
yuxihu approved these changes Apr 24, 2019
View reviewed changes
Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@anirudhacharya
Copy link
Member

@mxnet-label-bot update [pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed Bug Exception Handling OpenCV OpenCV related issues pr-awaiting-review PR is waiting for code review labels May 2, 2019
@wkcn wkcn requested a review from eric-haibin-lin as a code owner May 5, 2019 01:55
@wkcn wkcn merged commit 25ba1d1 into apache:master May 5, 2019
@wkcn
Copy link
Member

wkcn commented May 5, 2019

Merged. Thank you!

access2rohit pushed a commit to access2rohit/incubator-mxnet that referenced this pull request May 14, 2019
@anirudh2290
Prevent crashes for opencv exception and std::exception (apache#14433)
3bee946 
* Relax constexpr restriction

* Change imagenet_gen_qsym_mkldnn

* Revert constexpr change

* Add dummy image for test

* Fix unreverted change

* Remove url

* Fix imagenet change

* Catch only std::exception

* Fix const, remove dmlc::Error catch

* Retrigger CI
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@anirudh2290
Prevent crashes for opencv exception and std::exception (apache#14433)
9812da8 
* Relax constexpr restriction

* Change imagenet_gen_qsym_mkldnn

* Revert constexpr change

* Add dummy image for test

* Fix unreverted change

* Remove url

* Fix imagenet change

* Catch only std::exception

* Fix const, remove dmlc::Error catch

* Retrigger CI
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@larroy larroy larroy approved these changes

@yuxihu yuxihu yuxihu approved these changes

@wkcn wkcn wkcn approved these changes

@nswamy nswamy Awaiting requested review from nswamy

@szha szha Awaiting requested review from szha

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin eric-haibin-lin is a code owner

Assignees
No one assigned
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Milestone
No milestone
8 participants
@anirudh2290 @karan6181 @piyushghai @larroy @anirudhacharya @wkcn @yuxihu @marcoabreu