[GitHub] cjolivier01 commented on a change in pull request #9373: Proof of concept for exception handling in operators (DON'T MERGE)

[GitBox <gi...@apache.org>]

cjolivier01 commented on a change in pull request #9373: Proof of concept for exception handling in operators (DON'T MERGE)
URL: https://github.com/apache/incubator-mxnet/pull/9373#discussion_r162719371
 
 

 ##########
 File path: src/engine/threaded_engine.cc
 ##########
 @@ -391,18 +394,29 @@ void ThreadedEngine::WaitForAll() {
   finished_cv_.wait(lock, [this]() {
       return pending_.load() == 0 || kill_.load();
     });
+
+  if (global_exc_waitall_ptr) {
+    std::rethrow_exception(global_exc_waitall_ptr);
+  }
 }
 
 inline void ThreadedEngine::OnComplete(ThreadedOpr* threaded_opr) {
   bool is_temporary_opr = threaded_opr->temporary;
   // Mark complete for read variables
   for (auto&& i : threaded_opr->const_vars) {
+    if (i->var_ex) {
+        global_exc_waitall_ptr = i->var_ex;
+        threaded_opr->opr_ex = i->var_ex;
+    }
     i->CompleteReadDependency([this](OprBlock* opr) {
         this->PushToExecute(opr, false);
       });
   }
   // Mark complete for write variables.
   for (auto&& i : threaded_opr->mutable_vars) {
+    if (threaded_opr->opr_ex) {
 
 Review comment:
   Instead of each operator holding its exception and having to iterate through them afterwards, would a more generalized approach be simpler where a pointer to a shared exception-container object is in each threaded_opr? It could be a queue or list or just one. I'd imagine we only care about the first exception, right?  

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services