make python remote exception to rethrow when using remote reference to itself #29930

zhaojuanmao · 2019-11-15T22:56:17Z

Stack from ghstack:

make python remote exception to rethrow when using remote reference to itself #29930 make python remote exception to rethrow when using remote reference to itself

Right now, python call remote exception rethrown is coupled with deserializtiaon.
For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown.

This diff is to move remote exception rethrown out of deserialization, and exception can be handled for owner ref getValue()

close #29924

Differential Revision: D18541916

…o itself Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for owner ref getValue() close #29924 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/) [ghstack-poisoned]

…o itself Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for owner ref getValue() close #29924 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/) ghstack-source-id: 94037095 Pull Request resolved: #29930

pritamdamania87 · 2019-11-16T01:46:45Z

Looks like we have a duplicate PR: #29929?

pritamdamania87 · 2019-11-16T02:30:36Z

torch/csrc/distributed/rpc/rref.cpp

-template <typename T>
-const T& OwnerRRef<T>::getValue() const {
+template <>
+const IValue& OwnerRRef<IValue>::getValue() const {


Why do we have to separate IValue and python objects like this? Shouldn't the same code work in python/torchscript mode since this is C++ code?

because python object and IValue exception handing is different per previous request.

For IValue exception handling, it catches C++ exception and returns a message with message::Exception, then exception will be rethrown based on message::Exception type.

For pyobject exception handing, it did not throw a C++ exception, but catch it in python side and wrap it into a RemoteException to caller. Caller will check this returned result is RemoteException or not and then rethrown. This is based on previous request that we need to return python call stack trace to users. C++ exception can only return a message, no stack trace

mrshenli

Thanks for fixing! Shall we add tests for both to_here and local_value for both local (creating an RRef locally) and remote RRef?

mrshenli · 2019-11-17T21:13:55Z

torch/csrc/distributed/rpc/rref.cpp

  // TODO: use callback to make this non-blocking
  std::unique_lock<std::mutex> lock(mutex_);
  valueCV_.wait(lock, [this] { return value_.has_value(); });
+  PythonRpcHandler::getInstance().handleException(value_.value());


Is this inconsistent with what we do in PYTHON_CALL? In PYTHON_CALL the exception is wrapped into payload in generatePythonUDFResult, and then the exception message is sent to the caller and thrown there in toPyObjInternal. But here, the exception is thrown on the callee (owner) and relies on RequestCallback::operator() to catch it and convert that into an exception message? Is this intentional?

both PYTHON_CALL and PYTHON_FETCH_CALL re-throw python exception during deserialization.

Here, callee and caller are in the same process, did not go through deserialization, so skipped python exception re-thrown

zhaojuanmao · 2019-11-18T19:21:42Z

@mrshenli and @pritamdamania it is my bad to make it confusing, let me move python exception rethrown to PyRRef::localValue() where only caller can call it.

…reference to itself" Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for owner ref getValue() close #29924 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/) [ghstack-poisoned]

…o itself Pull Request resolved: #29930 Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for owner ref getValue() close #29924 ghstack-source-id: 94130312 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/)

mrshenli

LGTM! Please make sure all tests pass before landing.

mrshenli · 2019-11-18T21:16:14Z

@zhaojuanmao please update the PR/diff description accordingly.

…reference to itself" Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for owner ref getValue() close #29924 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/) [ghstack-poisoned]

…o itself Pull Request resolved: #29930 Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for ownerRef.localValue() or ownerRef.to_here() close #29924 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/) ghstack-source-id: 94148099

zhaojuanmao · 2019-11-19T14:51:35Z

checked these tests actually finished, but CI showed they are pending

zhaojuanmao · 2019-11-19T14:51:54Z

@pytorchbot retest please

…reference to itself" Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for owner ref getValue() close #29924 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/) [ghstack-poisoned]

…o itself Pull Request resolved: #29930 Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for ownerRef.localValue() or ownerRef.to_here() close #29924 ghstack-source-id: 94188833 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/)

zzzwen · 2019-11-19T18:27:53Z

torch/distributed/rpc/internal.py

@@ -119,6 +119,9 @@ def _run_function(binary_data, tensor_table):
        result = RemoteException(except_str)
    return result

+def _handle_exception(result):
+    if isinstance(result, RemoteException):


question/nit, do we lost the original Exception type while wrapping it as RemoteException? If we are able to retrieve the original type, rethrowing it as the original type will be helpful.

@zzzwen all exception type, msg and stack trace are wrapped into remoteException and users can read these info in client.

zhaojuanmao · 2019-11-19T19:37:13Z

some tests stuck again, rerun workflow did not trigger to retest it somehow.

for safety, I'm rebasing it and try one more round of tests again.

…reference to itself" Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for owner ref getValue() close #29924 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/) [ghstack-poisoned]

…o itself Pull Request resolved: #29930 Right now, python call remote exception rethrown is coupled with deserializtiaon. For owner ref, the setValue() and getValue() do not use serialization and deserialization, so when users create a ref to itself, and call ownerRef.to_here(), python call remote exception will not be rethrown. This diff is to move remote exception rethrown out of deserialization, and exception can be handled for ownerRef.localValue() or ownerRef.to_here() close #29924 ghstack-source-id: 94210894 Differential Revision: [D18541916](https://our.internmc.facebook.com/intern/diff/D18541916/)

facebook-github-bot · 2019-11-20T14:10:49Z

This pull request has been merged in b410d86.

zhaojuanmao requested review from mrshenli and pritamdamania87 as code owners November 15, 2019 22:56

pritamdamania87 reviewed Nov 16, 2019

View reviewed changes

mrshenli reviewed Nov 17, 2019

View reviewed changes

zhaojuanmao mentioned this pull request Nov 18, 2019

make python remote exception to rethrow when using remote reference to itself #29929

Closed

mrshenli approved these changes Nov 18, 2019

View reviewed changes

zzzwen reviewed Nov 19, 2019

View reviewed changes

facebook-github-bot closed this in b410d86 Nov 20, 2019

facebook-github-bot added the merged label Nov 20, 2019

facebook-github-bot deleted the gh/zhaojuanmao/16/head branch November 23, 2019 15:16

pytorch deleted a comment from zhaojuanmao Nov 25, 2019

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make python remote exception to rethrow when using remote reference to itself #29930

make python remote exception to rethrow when using remote reference to itself #29930

zhaojuanmao commented Nov 15, 2019 •

edited

pritamdamania87 commented Nov 16, 2019

pritamdamania87 Nov 16, 2019

zhaojuanmao Nov 18, 2019

mrshenli left a comment

mrshenli Nov 17, 2019

zhaojuanmao Nov 18, 2019

zhaojuanmao commented Nov 18, 2019 •

edited

mrshenli left a comment

mrshenli commented Nov 18, 2019

zhaojuanmao commented Nov 19, 2019

zhaojuanmao commented Nov 19, 2019

zzzwen Nov 19, 2019

zhaojuanmao Nov 19, 2019

zhaojuanmao commented Nov 19, 2019

facebook-github-bot commented Nov 20, 2019

make python remote exception to rethrow when using remote reference to itself #29930

make python remote exception to rethrow when using remote reference to itself #29930

Conversation

zhaojuanmao commented Nov 15, 2019 • edited

pritamdamania87 commented Nov 16, 2019

pritamdamania87 Nov 16, 2019

Choose a reason for hiding this comment

zhaojuanmao Nov 18, 2019

Choose a reason for hiding this comment

mrshenli left a comment

Choose a reason for hiding this comment

mrshenli Nov 17, 2019

Choose a reason for hiding this comment

zhaojuanmao Nov 18, 2019

Choose a reason for hiding this comment

zhaojuanmao commented Nov 18, 2019 • edited

mrshenli left a comment

Choose a reason for hiding this comment

mrshenli commented Nov 18, 2019

zhaojuanmao commented Nov 19, 2019

zhaojuanmao commented Nov 19, 2019

zzzwen Nov 19, 2019

Choose a reason for hiding this comment

zhaojuanmao Nov 19, 2019

Choose a reason for hiding this comment

zhaojuanmao commented Nov 19, 2019

facebook-github-bot commented Nov 20, 2019

zhaojuanmao commented Nov 15, 2019 •

edited

zhaojuanmao commented Nov 18, 2019 •

edited