Better Exception Handling for Operators #9681

anirudh2290 · 2018-02-02T07:07:45Z

Description

Please see: https://cwiki.apache.org/confluence/display/MXNET/Improved+exception+handling+in+MXNet
Implements Exception Handling for Operators.

Fixes #7335 and related issues

Functional Testing

Tested multiple execution graphs with different dependencies to make sure able we are able to handle them seperately.
Tested multiple wait_to_reads and multiple waitall calls (Added to unittests)
Tested Gluon, Imperative and Symbolic modes (Added to unittests)
Tested CPU and GPU memory alloc failures.
Tested all the use cases pointed out by the community: fixes Python crashes (core-dump) instead of a graceful error message when GPU context is used on a CPU-only instance (EC2 x1.32xlarge) #8835, fixes random_uniform causes VM to crash #9131, fixes nd.argmax cause "Kernel Died" error in Jupyter Notebook #9567, fixes nd.stack causes abort for differently sized arrays #9380

Performance Testing

I did a small performance testing task with resnet50 model on cifar10 dataset to make sure that there is no performance degradation because of the additional overhead of OnStart callback in each ExecuteOprBlock. I don't see any change.

Model: resnet50
Dataset: cifar10
Tested on ec2: p2.8xlarge

Mode	Measurement	Before Exception Handling Change	After Exception Handling Change
Symbolic	Speed	1064 samples/sec	1073 samples/sec
Imperative	Speed	187 samples/sec	184 samples/sec

Checklist

Essentials

Passed code style checking (make lint)
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
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Exception handling for Operators, tests, (and when applicable, API doc)

Comments

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

@cjolivier01 @mli @piiswrong @madjam @asmushetzel @eric-haibin-lin @reminisce @rahul003 @KellenSunderland @eftiquar

…perators2

KellenSunderland · 2018-02-02T09:50:14Z

This is going to be a big improvement for new users experimenting with the library. Thanks for the great work @anirudh2290.

KellenSunderland · 2018-02-02T09:52:12Z

tests/python/unittest/test_exc_handling.py

+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+#Unless required by applicable law or agreed to in writing,


Nit: spacing

Good catch! Fixed.

piiswrong · 2018-02-02T18:42:39Z

include/mxnet/engine.h

@@ -182,7 +182,7 @@ class MXNET_API Engine {
                         std::vector<VarHandle> const& mutable_vars,
                         FnProperty prop = FnProperty::kNormal,
                         int priority = 0,
-                         const char* opr_name = nullptr) = 0;
+                         const char* opr_name = nullptr, bool wait = false) = 0;


what's wait? Why do you need it? Please document arguments

wait is used to indicate to the ExecuteOprBlock whether it is a waitforvar operation. it should not block the execution of the operator for WaitForVar.

piiswrong · 2018-02-02T18:43:48Z

src/engine/threaded_engine.cc

+    if (threaded_var->ex_ptr) {
+      std::rethrow_exception(threaded_var->ex_ptr);
+    }
+  }


why are you not returning?

Good catch! Fixed.

piiswrong · 2018-02-02T18:45:02Z

src/engine/threaded_engine.cc

@@ -391,6 +400,11 @@ void ThreadedEngine::WaitForAll() {
  finished_cv_.wait(lock, [this]() {
      return pending_.load() == 0 || kill_.load();
    });
+  if (global_ex_ptr) {
+    std::exception_ptr ex_ptr = global_ex_ptr;


use std::rethrow_exception(std::move(global_ex_ptr))

Rolling this back, Since the state of global_ex_ptr is not guaranteed to be nullptr after the move and it depends on the implementation. This probably explains why it started failing on windows, after the change. Please let me know if you have any concerns.

piiswrong · 2018-02-02T18:45:47Z

src/engine/threaded_engine.cc

+      i->ex_ptr = threaded_opr->ex_ptr;
+      if (!global_ex_ptr) global_ex_ptr = i->ex_ptr;
+    }
+    bool debug_info = (engine_info_ && debug_wait_var_ == i);


why remove const?

This was not intentional. Added it back.

piiswrong · 2018-02-02T18:48:54Z

src/engine/threaded_engine.cc

+    if (threaded_opr->ex_ptr) {
+      i->ex_ptr = threaded_opr->ex_ptr;
+      if (!global_ex_ptr) global_ex_ptr = i->ex_ptr;
+    }


suppose and operator has three outputs x, y and z and it raises an exception.
then x.asnumpy() would raise an error.
Then y.asnumpy() would raise the same error again.

and if I do z += 1 and it succeeds, z.asnumpy() would still raise the error

z += 1 won't execute since z already has an exception associated with it. z.asnumpy() will still raise the error.

piiswrong · 2018-02-02T18:51:54Z

src/engine/threaded_engine.h

@@ -175,6 +175,8 @@ class ThreadedVar final
  static std::atomic<std::size_t> counter;
  ~ThreadedVar() { LOG(INFO) << __func__ << " " << --counter; }
 #endif  // ENGINE_DEBUG
+  /*! \brief exception_ptr associated with the ThreadedVar */
+  std::exception_ptr ex_ptr;


ex_ptr is a bad variable name

i changed ex_ptr for var to var_exception, ex_ptr for opr to opr_exception and the global exception_ptr global_exception_ . I welcome if you have any other suggestions for naming them.

piiswrong · 2018-02-02T18:53:50Z

src/engine/threaded_engine.h

@@ -338,33 +346,46 @@ class ThreadedEngine : public Engine {
 #endif
    CallbackOnComplete callback = this->CreateCallback(
        ThreadedEngine::OnCompleteStatic, opr_block);
+    CallbackOnComplete on_start_callback = this->CreateCallback(


what's the point of creating a call back here?

The callback is not strictly necessary here, since it is called only once, but I included it to keep the ExecuteOprBlock easier to read and separate out the OnStart logic.

I think this is unnecessary overhead. Call OnStart directly if possible

Called OnStart directly.

piiswrong · 2018-02-02T18:54:27Z

src/engine/threaded_engine.h

+          } else {
+            callback();
+          }
+        } catch (dmlc::Error& e) {


why only catch dmlc error?

The reason I catch only dmlc::Error is that The guards in the c_api API_BEGIN and API_END/API_END_HANDLE_ERROR only catch dmlc::Error currently and propagate to frontend.

I have used dmlc::Error in the inner block and used std::exception to catch other stdlib exceptions thrown in the outer block. Currently, it will catch only dmlc::Error and for other exceptions(std::exception) the process will be terminated. I will open another PR to handle std::exception and change the c_api guards and frontend code.

reminisce · 2018-02-02T07:30:58Z

include/mxnet/engine.h

@@ -182,7 +182,7 @@ class MXNET_API Engine {
                         std::vector<VarHandle> const& mutable_vars,
                         FnProperty prop = FnProperty::kNormal,
                         int priority = 0,
-                         const char* opr_name = nullptr) = 0;
+                         const char* opr_name = nullptr, bool wait = false) = 0;


Add \param for wait in doc.

Put bool wait... in the next line to keep the coding style consistent with the existing context. Same all the following changes.

Good catch! Fixed here and other places.

reminisce · 2018-02-02T07:32:03Z

src/engine/naive_engine.cc

@@ -73,7 +73,7 @@ class NaiveEngine final : public Engine {
                        std::vector<VarHandle> const& const_vars,
                        std::vector<VarHandle> const& mutable_vars,
                        FnProperty prop = FnProperty::kNormal,
-                        const char* opr_name = nullptr) override {
+                        const char* opr_name = nullptr, bool wait = false) override {


Put in the next line.

reminisce · 2018-02-02T07:32:15Z

src/engine/naive_engine.cc

@@ -125,7 +125,7 @@ class NaiveEngine final : public Engine {
                 std::vector<VarHandle> const& mutable_vars,
                 FnProperty prop = FnProperty::kNormal,
                 int priority = 0,
-                 const char* opr_name = nullptr) override {
+                 const char* opr_name = nullptr, bool wait = false) override {


reminisce · 2018-02-02T07:36:51Z

tests/python/unittest/test_exc_handling.py

+        inputs = [x, y]
+        out = mx.symbol.ElementWiseSum(*inputs, name="esum")
+        out = mx.sym.dot(z, out)
+        out2 = mx.sym.random_normal(0, -1, x_shape, ctx=default_context())


random.normal as it's the preferred way now.

piiswrong · 2018-02-02T19:02:48Z

src/engine/threaded_engine.h

        if (debug_info) {
          LOG(INFO) << "Fin ExecuteOprFn ";
        }
-      } catch(dmlc::Error &e) {
+      } catch (dmlc::Error& e) {


what would this catch now? There is already a try block inside

@piiswrong my intention for the outer block was to catch all other exceptions which are not caught in the inner block. I should change dmlc::Error to std::exception to catch all standard exception. But you make a good point about propagating all the exceptions and not just dmlc::Error to the frontend. We can take one of the two approaches here: 1. catch dmlc::Error and terminate the process for everything else. 2. catch both dmlc::Error and standard exceptions and propagate to frontend. This will require code changes to the guards , c_api_error and potentially frontend code.

piiswrong · 2018-02-02T19:03:15Z

src/storage/cpu_device_storage.h

@@ -61,10 +61,10 @@ inline void* CPUDeviceStorage::Alloc(size_t size) {
  void* ptr;
 #if _MSC_VER
  ptr = _aligned_malloc(size, alignment_);
-  if (ptr == NULL) throw std::bad_alloc();
+  if (ptr == NULL) LOG(FATAL) << "Malloc failure";


Failed to allocate CPU memory

piiswrong · 2018-02-02T19:03:40Z

src/storage/gpu_device_storage.h

@@ -62,7 +62,7 @@ inline void* GPUDeviceStorage::Alloc(size_t size) {
 #endif  // MXNET_USE_NCCL
  cudaError_t e = cudaMalloc(&ret, size);
  if (e != cudaSuccess && e != cudaErrorCudartUnloading)
-    throw std::bad_alloc();
+    LOG(FATAL) << cudaGetLastError();


Does cudaGetLastError return a string or an error code?

Good catch! I have changed it to call cudaGetErrorString

piiswrong · 2018-02-02T19:05:11Z

src/engine/threaded_engine.h

@@ -476,6 +507,9 @@ class ThreadedEngine : public Engine {
   */
  std::mutex finished_m_;
  std::condition_variable finished_cv_;
+  /*! \brief exception_ptr associated with the engine,
+   * which is used to throw exception in waitall */
+  std::exception_ptr global_ex_ptr;


ex_ptr is a bad name. class members should end with _.

piiswrong · 2018-02-05T19:13:49Z

suppose and operator has three outputs x, y and z and it raises an exception.
then x.asnumpy() would raise an error.
Then y.asnumpy() would raise the same error again.

I think an error should only be raised once. After it's raised, it should be cleared from all arrays that is pointing to that error.

This can be achieved by setting the object referenced by exception_ptr to an invalid value

anirudh2290 · 2018-02-05T22:23:47Z

@piiswrong Trying to understand your comment.

Lets say we have some code snippet like the below:

try:
       x, y, z = op()
       x.asnumpy()
except:
       handle_exc()
y = op2(y)
y.asnumpy()

If we clear the exception_ptr corresponding to the var y when x.asnumpy() is executed, y may have some garbage value in it. op2 may end up executing fine, and after the last line y.asnumpy() we have no exception thrown. Shouldn't all the vars and ops which are in the chain following a failed op also fail ? Not doing this, will lead to the vars in the dependency chain following a failed op having non-deterministic garbage values depending on how the failed op and the following ops behave.

KellenSunderland · 2018-02-06T09:59:27Z

@anirudh2290 Good idea to use an example. From my perspective as a user I generally prefer that things fail as quickly as possible for me so that I don't have to track down root causes. Following this logic, I'd tend to agree with you, but would actually prefer it if this line threw an exception:

try:
       x, y, z = op()
       x.asnumpy()
except:
       handle_exc()
y = op2(y)  # Runtime exception
y.asnumpy()

I understand that this is a lazy operation, but wonder if it's still possible to do some failure validation here? If not the first blocking call (asnumpy) would also be a fairly intuitive place to throw.

piiswrong · 2018-02-06T18:18:30Z

What I meant is, for example:

try:
       x, y, z = op()
       x.asnumpy()
except:
       handle_exc()
y.asnumpy()  # Fail

Currently y.asnumpy() will fail again with the same error as x.asnumpy().
But a single exception shouldn't be raised twice

anirudh2290 · 2018-02-06T18:45:24Z

try:
      x, y, z = op()
      x.asnumpy()
except:
      handle_exc()

'''
The below just pushes operation to the engine, no guarantee that op2 is executed (ExecuteOprBlock may not be called)
'''
y = op2(y)
y.asnumpy() # Guarantees that all the operations that are writing to y are executed

@KellenSunderland As you mentioned since it is a lazy operation, there is no guarantee that operation is executed, just that it is pushed to the engine. So, there is no guarantee that ExecuteOprBlock is called for the operator. On the other hand, it is guaranteed that all operations which write to a particular variable are executed when the blocking call on that variable is made. Therefore, I have rethrown exceptions in WaitForVar and WaitForAll. I understand that this may not be as intuitive to users as throwing on the y=op2(y) line itself, but I don't think it is possible to rethrow when it is pushed to engine.

anirudh2290 · 2018-02-06T19:22:31Z

try:
     x, y, z = op()
     x.asnumpy() #Throws exception, sets exception_ptr to nullptr
except:
     handle_exc()
y.asnumpy() #exception_ptr is nullptr, doesn't throw
y = op2(y)  
y.asnumpy() # y may have garbage values, op2 may execute just fine, exception_ptr still nullptr, doesn't throw ?

@piiswrong As depicted in the example above, if we decide to invalidate exception_ptr for y by setting it to nullptr when we WaitToRead x (I am unsure how we will do this), then we won't be propagating exceptions down the chain. Therefore, the last line here will execute just fine instead of throwing an exception, and user will end up with garbage values for y.

I understand your point that if an op has multiple write vars, and if we waited for one of the write vars and re-threw exception, we shouldn't throw it again for other vars. But, if we end up invalidating the exception_ptr, any continuing operators may or may not fail, and since the exception_ptr is invalidated we wouldn't be re-throwing the exception in any of the following WaitToReads.

anirudh2290 · 2018-02-08T07:59:01Z

After discussion with @piiswrong , we came to a conclusion that in an execution graph once an exception is thrown, the same exception should not be thrown again. For example:

x = None
y = None
try:
     x, y = op1() # Fails
     x.asnumpy() # Throw exception
except:
     handle_exc(x, y)
y.asnumpy() # Should execute fine
z = op2(x) # Should execute fine, expectation is that user modified the value of x from garbage, when handling exception

You can see that op1 throws an exception and it may end up writing garbage values to x and y. The line x.asnumpy() throws exception. Once this is done, user may handle the exception or keep the garbage values as it is. Any consequent usage of x or op2 should not throw the same exception, since user is not expecting and it is already handled.

One challenge during the implementation was that dereferencing exception_ptr in C++ will cause undefined behavior. So there is no way to modify state of the exception_object that exception_ptr points to, just by using exception_ptr itself. To workaround this limitation, we are holding the exception_ptr itself in a shared_ptr object.

We decided to remove global_exception thrown in WaitForAll, since it adds unnecessary complexity and is not really used much except during benchmarking.

anirudh2290 · 2018-02-12T18:38:39Z

@piiswrong: Do you have additional suggestions ?

* Add support for threaded engine * Add support for threaded engine * Remove on_start_callback for else * Add support for global_ex_ptr * Rethrow in waitall only once * run tests for gpu * Add comments for exception_ptr * Fix lint * Push exc_handling tests * Add comments for OnStart * Fixes for exc handling * Catch std::exception for all other exceptions * Rollback std::move use * Fix style * Fix onstart * Fix debug_info * Throw exception only once in an execution graph * make test naming consistent * Fix symbolic test * Remove unused code

anirudh2290 added 12 commits January 25, 2018 00:33

Add support for threaded engine

a366ef5

Add support for threaded engine

0d12ebe

Remove on_start_callback for else

a9728d6

Merge branch 'master' of https://github.com/dmlc/mxnet into POC_exc_o…

fb5e4ef

…perators2

Add support for global_ex_ptr

8d59764

Rethrow in waitall only once

70e914f

run tests for gpu

a9a51e8

Add comments for exception_ptr

eb1166b

Merge latest

dcd5fab

Fix lint

c80d286

Push exc_handling tests

bcbe19a

Add comments for OnStart

51b0dfb

KellenSunderland approved these changes Feb 2, 2018

View reviewed changes

piiswrong reviewed Feb 2, 2018

View reviewed changes

reminisce reviewed Feb 2, 2018

View reviewed changes

piiswrong reviewed Feb 2, 2018

View reviewed changes

anirudh2290 added 3 commits February 2, 2018 22:04

Fixes for exc handling

fe1fa4e

Catch std::exception for all other exceptions

3bcc372

Rollback std::move use

87da504

Fix style

fa0d83d

anirudh2290 added 2 commits February 6, 2018 01:12

Fix onstart

bbf6702

Fix debug_info

320de0e

Throw exception only once in an execution graph

ec5d68b

anirudh2290 added 3 commits February 8, 2018 19:08

make test naming consistent

04dd9de

Fix symbolic test

f7349cb

Remove unused code

5ae7743

piiswrong merged commit 7b24137 into apache:master Feb 13, 2018

anirudh2290 mentioned this pull request May 5, 2018

argmax causes python VM to crash #9118

Closed

Lancelot365 mentioned this pull request Jun 19, 2018

Failed to run the R-FCN 3K demo - nnvm check failed mahyarnajibi/SNIPER#3

Closed

anirudh2290 mentioned this pull request Nov 13, 2018

waitforall() hides errors #13234

Closed

arcadiaphy mentioned this pull request Mar 29, 2019

mx.nd.Custom conflicts with memory management #14522

Open

Better Exception Handling for Operators #9681

Better Exception Handling for Operators #9681

Conversation

anirudh2290 commented Feb 2, 2018 • edited Loading

Description

Functional Testing

Performance Testing

Checklist

Essentials

Changes

Comments

KellenSunderland commented Feb 2, 2018

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

piiswrong Feb 2, 2018 • edited Loading

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

piiswrong Feb 2, 2018 • edited Loading

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

anirudh2290 Feb 5, 2018 • edited Loading

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

anirudh2290 Feb 2, 2018 • edited Loading

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

piiswrong commented Feb 5, 2018

anirudh2290 commented Feb 5, 2018

KellenSunderland commented Feb 6, 2018 • edited Loading

piiswrong commented Feb 6, 2018

anirudh2290 commented Feb 6, 2018

anirudh2290 commented Feb 6, 2018

anirudh2290 commented Feb 8, 2018

anirudh2290 commented Feb 12, 2018

anirudh2290 commented Feb 2, 2018 •

edited

Loading

piiswrong Feb 2, 2018 •

edited

Loading

piiswrong Feb 2, 2018 •

edited

Loading

anirudh2290 Feb 5, 2018 •

edited

Loading

anirudh2290 Feb 2, 2018 •

edited

Loading

KellenSunderland commented Feb 6, 2018 •

edited

Loading