Add exception handling support for waitall #14397

anirudh2290 · 2019-03-12T02:12:33Z

Description

Add exception handling support for waitall
Fixes: #13234, Fixes: #14426

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

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)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

added exception handling support for waitall
added tests
modified documentation

Comments

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

szha

I thought it was by design, given the documentation. What changed?

anirudh2290 · 2019-03-12T06:12:41Z

We had earlier decided this based on complication associated with adding global exception_ptr and having to reset all the exception_ptrs corresponding to vars and ops.Another, reason was the performance impact. I think we overestimated the difficulty since the decision to used shared_ptr<exception_ptr> enables us to modify the exception_ptrs associated with vars and ops by just setting it to nullptr. Also, there would be performance implication if there is an exception in the code and this can be more pronounced for a tool supposed to used only for benchmarking. But customers seems to be using it outside benchmarking and the performance impact will only happen when there is an exception thrown so it should be acceptable.

karan6181 · 2019-03-12T17:14:34Z

@mxnet-label-bot add [Exception Handling, pr-work-in-progress]

apeforest · 2019-03-12T17:16:52Z

This is good stuff. waitall() is needed when we want to synchronize on multiple tensors.

src/engine/threaded_engine.h

docs/architecture/exception_handling.md

anirudh2290 · 2019-03-14T19:36:49Z

this is ready for review

anirudh2290 · 2019-03-15T18:33:22Z

The CI stage is complete but shows pending. Any idea @marcoabreu @lebeg

yuxihu

Nice work. Some minor comments.

@larroy Please also take a look at this PR. You raised questions about using shared_ptr around exception_ptr. Aniruh had an answer here.

yuxihu · 2019-03-20T01:08:49Z

src/engine/threaded_engine.cc

@@ -428,6 +447,14 @@ inline void ThreadedEngine::OnComplete(ThreadedOpr* threaded_opr) {
  for (auto&& i : threaded_opr->mutable_vars) {
    if (threaded_opr->opr_exception && *threaded_opr->opr_exception) {
      i->var_exception = threaded_opr->opr_exception;
+      // add current operator exceptions to global exceptions if not already
+      // added
+      auto it = std::find(global_exception_refs_.begin(),


L452-L457 are used in three places. Can we make it a function?

Thanks for the review ! I have moved it into a function.

yuxihu · 2019-03-20T01:10:55Z

src/resource.cc

-    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()) {


Was this a bug? Shall we check ctx == Context::GPU()? There are other non-GPU context.

yes this is a bug. I have changed the check for GPU context.

larroy · 2019-03-21T19:47:04Z

src/engine/threaded_engine.cc

+  std::exception_ptr tmp;
+  if (!global_exception_refs_.empty()) {
+    // iterate through all exception refs
+    for (auto itr = global_exception_refs_.begin();


shall we use range for with const reference? is much less noisy.

Thanks for the review!

larroy · 2019-03-21T19:48:39Z

src/engine/threaded_engine.h

@@ -60,6 +60,9 @@ namespace engine {
 // Forward declarations
 struct ThreadedOpr;

+/*! shared_ptr to exception_ptr, used for exception handling */
+typedef std::shared_ptr<std::exception_ptr> ExceptionRef;


Do we need to wrap it in a shared_ptr? exception_ptr has already shared ptr semantics according to cppreference.

Also, the name Ref to me is confusing, why not Ptr? why add a suffix of the type at all?

exception_ptr cannot be dereferenced , so we cannot update the exception object it is pointing to or make it nullptr. Since this is a requirement for us we wrapped it in a shared_ptr. Used ref to make it consistent with other places in MXNet.

larroy · 2019-03-21T19:50:28Z

src/engine/threaded_engine.cc

+         itr != global_exception_refs_.end(); ++itr) {
+      const ExceptionRef& ptr = *itr;
+      // the first exception will be saved to be rethrown later
+      if (*ptr != nullptr && !tmp) {


can be evaluated in bool context, so less noise.

larroy · 2019-03-21T19:52:45Z

src/engine/threaded_engine.cc

@@ -415,6 +415,25 @@ void ThreadedEngine::WaitForAll() {
  finished_cv_.wait(lock, [this]() {
      return pending_.load() == 0 || kill_.load();
    });
+  std::exception_ptr tmp;


Shall this code be wrapped in a function?

Currently it is used only once so it is fine to not use a function.

Maybe then a better variable name than tmp? ex_to_rethrow?

larroy · 2019-03-21T19:55:30Z

tests/python/unittest/test_exc_handling.py

-    except MXNetError:
-        caught = True
-    assert caught, "No exception thrown"
+    def multiple_waits(waitall=False):


does it make sense to use "@raises"? maybe it would be easier to read.

https://nose.readthedocs.io/en/latest/testing_tools.html

At least a small comment explaining the test approach for future readers and that we expect exception to be thrown, is that the intent?

I have added comments. Intention is to test multiple wait_to_reads and waitalls for vars in same scope.

…to add_waitall_support

access2rohit · 2019-03-27T02:26:46Z

src/engine/threaded_engine.cc

+  if (!global_exception_refs_.empty()) {
+    // iterate through all exception refs
+    for (const auto& global_exception_ref : global_exception_refs_) {
+      // the first exception will be saved to be rethrown later


What is the order of exceptions stored in the "global_exception_refs_" ? If we are throwing the first one then is it the innermost in the stack that causes all other exceptions or the outermost ? If its outermost then it might not give correct idea about what was the root cause

@access2rohit the order of the exceptions will be maintained exception thrown first will be rethrown first.

lebeg

nice

…to add_waitall_support

piyushghai · 2019-04-05T21:54:58Z

@anirudh2290 Is this PR good to merge now ?

wkcn · 2019-04-08T07:22:01Z

Merged. Thanks for your contribution!

ThomasDelteil · 2019-04-10T19:01:50Z

This PR likely broke the profiler tutorial, http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/NightlyTestsForBinaries/detail/master/275/pipeline/
@anirudh2290 any idea why? https://mxnet.incubator.apache.org/versions/master/tutorials/python/profiler.html

anirudh2290 · 2019-04-10T19:35:21Z

checking this now.

* Relax constexpr restriction * Change imagenet_gen_qsym_mkldnn * Add exception handling support for waitall * Fix exception handling documentation * Revert constexpr change * Add comments * Fix test * Skip exception for op check names * Print exceptions thrown for CPP Package NDArray module * Reducing batch_size to make cpp-package example pass * Fix bug: apache#14426 * use ExceptionRef in threaded_engine code * add note for performance impact of waitall * Add check for GPU contxt * Use range for with const reference * Improve comments and error message for exception handling test * Change exception_ptr name in waitall * Fix bug

anirudh2290 added 10 commits February 5, 2019 23:12

Relax constexpr restriction

a85b3f0

Change imagenet_gen_qsym_mkldnn

5bd6428

Merge branch 'master' of https://github.com/apache/incubator-mxnet

d684e4c

Merge branch 'master' of https://github.com/apache/incubator-mxnet

5debcc2

Merge branch 'master' of https://github.com/apache/incubator-mxnet

082a0aa

Merge branch 'master' of https://github.com/apache/incubator-mxnet

f194aa2

Add exception handling support for waitall

26079e6

Fix exception handling documentation

f0a76e3

Fix quantization file

c169a86

Revert constexpr change

cd98fa9

anirudh2290 requested a review from szha as a code owner March 12, 2019 02:12

anirudh2290 added 2 commits March 12, 2019 05:30

Add comments

4f694f6

Fix test

d79560b

szha reviewed Mar 12, 2019

View reviewed changes

Skip exception for op check names

3a581e8

marcoabreu added Exception Handling pr-work-in-progress PR is still work in progress labels Mar 12, 2019

Print exceptions thrown for CPP Package NDArray module

0b5444b

anirudh2290 requested a review from nswamy as a code owner March 12, 2019 19:04

yuxihu reviewed Mar 12, 2019

View reviewed changes

src/engine/threaded_engine.h Show resolved Hide resolved

src/engine/threaded_engine.h Outdated Show resolved Hide resolved

docs/architecture/exception_handling.md Show resolved Hide resolved

anirudh2290 mentioned this pull request Mar 13, 2019

resnet cpp-package test is broken #14406

Closed

Reducing batch_size to make cpp-package example pass

38b8dca

anirudh2290 mentioned this pull request Mar 14, 2019

mx.random.seed with ctx failures on a gpu build when run with cpu context #14426

Closed

anirudh2290 added 3 commits March 14, 2019 02:41

Fix bug: apache#14426

1c0d936

use ExceptionRef in threaded_engine code

034e9c7

add note for performance impact of waitall

d34d95e

anirudh2290 changed the title ~~[WIP] Add exception handling support for waitall~~ Add exception handling support for waitall Mar 14, 2019

anirudh2290 mentioned this pull request Mar 17, 2019

fix custom operation in fork #14451

Merged

7 tasks

yuxihu mentioned this pull request Mar 20, 2019

[WIP] Remove redundant shared_ptr for exception_ptr #13933

Closed

yuxihu reviewed Mar 20, 2019

View reviewed changes

anirudh2290 mentioned this pull request Mar 20, 2019

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

Merged

4 tasks

Add check for GPU contxt

48a6638

anirudh2290 added pr-awaiting-review PR is waiting for code review pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-work-in-progress PR is still work in progress labels Mar 21, 2019

larroy reviewed Mar 21, 2019

View reviewed changes

anirudh2290 added 3 commits March 21, 2019 20:38

Use range for with const reference

f631d57

Improve comments and error message for exception handling test

54e301f

Change exception_ptr name in waitall

9e6972a

anirudh2290 force-pushed the add_waitall_support branch from c5f0b28 to 9e6972a Compare March 21, 2019 21:33

anirudh2290 added 2 commits March 22, 2019 23:52

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

ee1fabd

…to add_waitall_support

Fix bug

ff8151c

access2rohit reviewed Mar 27, 2019

View reviewed changes

lebeg approved these changes Mar 28, 2019

View reviewed changes

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

2a13065

…to add_waitall_support

anirudh2290 mentioned this pull request Apr 4, 2019

[Discussion] 1.5.0 Roadmap #14619

Closed

wkcn merged commit 3781816 into apache:master Apr 8, 2019

anirudh2290 mentioned this pull request Apr 11, 2019

Fix profiler check #14677

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exception handling support for waitall #14397

Add exception handling support for waitall #14397

anirudh2290 commented Mar 12, 2019 •

edited

Loading

szha left a comment

anirudh2290 commented Mar 12, 2019

karan6181 commented Mar 12, 2019

apeforest commented Mar 12, 2019

anirudh2290 commented Mar 14, 2019

anirudh2290 commented Mar 15, 2019

yuxihu left a comment

yuxihu Mar 20, 2019

anirudh2290 Mar 20, 2019

yuxihu Mar 20, 2019

anirudh2290 Mar 20, 2019

larroy Mar 21, 2019

anirudh2290 Mar 21, 2019

anirudh2290 Mar 21, 2019

larroy Mar 21, 2019

larroy Mar 21, 2019

anirudh2290 Mar 21, 2019

larroy Mar 21, 2019

anirudh2290 Mar 21, 2019

larroy Mar 21, 2019

anirudh2290 Mar 21, 2019

larroy Mar 21, 2019

larroy Mar 21, 2019

anirudh2290 Mar 21, 2019

access2rohit Mar 27, 2019

anirudh2290 Apr 2, 2019

lebeg left a comment

piyushghai commented Apr 5, 2019

wkcn commented Apr 8, 2019

ThomasDelteil commented Apr 10, 2019

anirudh2290 commented Apr 10, 2019

Add exception handling support for waitall #14397

Add exception handling support for waitall #14397

Conversation

anirudh2290 commented Mar 12, 2019 • edited Loading

Description

Checklist

Essentials

Changes

Comments

szha left a comment

Choose a reason for hiding this comment

anirudh2290 commented Mar 12, 2019

karan6181 commented Mar 12, 2019

apeforest commented Mar 12, 2019

anirudh2290 commented Mar 14, 2019

anirudh2290 commented Mar 15, 2019

yuxihu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebeg left a comment

Choose a reason for hiding this comment

piyushghai commented Apr 5, 2019

wkcn commented Apr 8, 2019

ThomasDelteil commented Apr 10, 2019

anirudh2290 commented Apr 10, 2019

anirudh2290 commented Mar 12, 2019 •

edited

Loading