Lift rpc_timeout to RpcAgent, for other RpcAgents to reuse. #29341

xush6528 · 2019-11-07T00:49:43Z

Stack from ghstack:

Lift rpc_timeout to RpcAgent, for other RpcAgents to reuse. #29341 Lift rpc_timeout to RpcAgent, for other RpcAgents to reuse.

So that other RpcAgent could use this timeout setting as well.

Differential Revision: D5681951

#29336 - Rename the concept from "rpcTimeout" to "globalProcessTimeout", because 1) this timeout is used to limit the server-side request processing time. 2) This timeout is a global setting in contract to per-RPC setting. Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/) [ghstack-poisoned]

Pull Request resolved: #29341 - Rename the concept from "rpcTimeout" to "globalProcessTimeout", because 1) this timeout is used to limit the server-side request processing time. 2) This timeout is a global setting in contract to per-RPC setting. ghstack-source-id: 93429217 Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/)

#29336 - Rename the concept from "rpcTimeout" to "globalProcessTimeout", because 1) this timeout is used to limit the server-side request processing time. 2) This timeout is a global setting in contract to per-RPC setting. Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/) [ghstack-poisoned]

Pull Request resolved: #29341 - Rename the concept from "rpcTimeout" to "globalProcessTimeout", because 1) this timeout is used to limit the server-side request processing time. 2) This timeout is a global setting in contract to per-RPC setting. ghstack-source-id: 93433933 Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/)

pietern

@pritamdamania87 commented in the original PR #29336 (comment) about why there should be 3 timeouts and I agree. From the client's perspective there is no difference between the phases you list. Allowing different timeouts to be used means it's practically impossible to reason about timeouts (or spot them in log output) anymore.

Looking at the contents of the PR, I see two things:

Lifting rpcTimeout from ProcessGroupAgent into RPCAgent and renaming it
Unrelated formatting changes

We can all agree that lifting the parameter to RPCAgent is a good idea. Renaming is a separate issue and assumes we're going to have a bunch of timeouts, so we should discuss this more before going through with it. The formatting changes should be a separate formatting-only PR so it keeps the history usable (or you'll end up investigating why those lines were touched as part of a timeout change).

I propose the following:

Remove the rename from this PR and make it only move the timeout field to the base class.
Submit an issue for the rename so we can discuss what you're proposing.
Submit a separate PR for the formatting changes.

xush6528 · 2019-11-07T19:47:07Z

@pietern

Thanks for the suggestions.

For 2), opened #29402 to explain the whole context and idea, and to start discussion.

Follow 1), 3), reverting renaming and formatting in this PR.

#29336 - Rename the concept from "rpcTimeout" to "globalProcessTimeout", because 1) this timeout is used to limit the server-side request processing time. 2) This timeout is a global setting in contract to per-RPC setting. Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/) [ghstack-poisoned]

Pull Request resolved: #29341 So that other RpcAgent could use this timeout setting as well. ghstack-source-id: 93471330 Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/)

mrshenli · 2019-11-07T21:00:55Z

torch/csrc/distributed/rpc/rpc_agent.h

@@ -93,6 +96,11 @@ class TORCH_API RpcAgent {

  virtual const WorkerInfo& getWorkerInfo(worker_id_t id) const = 0;

+  // Retrieve the timeout for all RPCs.
+  virtual const std::chrono::milliseconds& getRpcTimeout() const {


Why this needs to be virtual. In which situation a derived class would like to override this value?

Can we make it inline?

Removing it.

Marking it as inline.

Turned out pybind11 does not require it to be declared in sub-classes. pybind11 is able to look up this member function in the base class.

mrshenli · 2019-11-07T21:02:33Z

torch/distributed/rpc/__init__.py

@@ -53,7 +53,10 @@ def init_model_parallel(
            self_rank (int): a globally unique id/rank of this node.
            init_method(str): backend specific init arguments.
            num_send_recv_threads(int): Number of threads for send/recv work.
-            rpc_timeout (datetime.timedelta): Timeout for RPCs. Defaults to 10 seconds.
+            rpc_timeout (datetime.timedelta):
+                Global timeout for server to process an RPC request.


Before we made the decision on whether we should have finer grain timeout (server, client, etc.), shall we avoid mentioning server here?

Makes sense. Removing it for now.

mrshenli · 2019-11-07T21:05:23Z

torch/csrc/distributed/rpc/init.cpp

-              &RpcAgent::sync,
+              "sync", &RpcAgent::sync, py::call_guard<py::gil_scoped_release>())
+          .def(
+              "_get_rpc_timeout",


Question, if the RpcAgent will handle timeout internally, why do we need to expose this API in Python and C++? Is this only for test and debugging?

I think what @rohan-varma mentioned was since we allow users to set timeout, we need to have a getter to allow users to inspect this value as well.

I see, makes sense

mrshenli

LGTM! Approve to unblock. Please make sure tests pass before landing.

#29336 So that other RpcAgent could use this timeout setting as well. Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/) [ghstack-poisoned]

Pull Request resolved: #29341 So that other RpcAgent could use this timeout setting as well. ghstack-source-id: 93481902 Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/)

facebook-github-bot · 2019-11-08T05:17:55Z

This pull request has been merged in e66626a.

pietern · 2019-11-08T08:09:38Z

@xush6528 Thanks!

pritamdamania87 · 2019-11-12T01:03:05Z

torch/distributed/rpc/api.py

@@ -78,6 +79,11 @@ def _init_rpc(
        raise RuntimeError("RPC is already initialized")

    # Initialize RPC.
+    if not isinstance(rpc_timeout, datetime.timedelta):
+        raise RuntimeError(
+            "`rpc_timeout` must be a `datetime.timedelta`."


nit: I don't think we should have backticks (`) in exception messages. This would be displayed on the user's terminal and there is no markdown there.

@pritamdamania87

I was not expecting a markdown format. But want to make clear that it's a python variable. Any other suggestion to make it distinct? or you think plain text is fine?

pritamdamania87 · 2019-11-12T01:07:57Z

test/rpc_test.py

@@ -962,13 +962,11 @@ def test_call_method_on_rref(self):

        self.assertEqual(result, sum(vals))

-    @requires_process_group_agent("PROCESS_GROUP rpc backend specific test, skip")
    @dist_init
    def test_get_default_rpc_timeout(self):
        timeout = rpc.get_rpc_timeout()


See the discussion in #28926, we should directly add docstrings to the pybind method.

@pritamdamania87 currently, ProcessGroupAgent is not exposed in the docs since I assumed that this is a private class and there's no reason for the user to directly access it (we provide higher level APIs for everything that needs to be done). If we wanted to move this doc to pybind, we'd have to expose documentation for ProcessGroupAgent, since get_rpc_timeout is a method on that class - is this something we'd want to do?

Created issue #29628 for tracking thihs.

Can we move this method from RpcAgent to torch.distributed.rpc and the implementation uses the default RpcAgent?

xush6528 requested review from apaszke, mrshenli and pietern as code owners November 7, 2019 00:49

xush6528 requested review from rohan-varma, satgera and pritamdamania87 November 7, 2019 00:50

xush6528 mentioned this pull request Nov 7, 2019

Pass global_process_timeout to RpcAgent #29336

Closed

This was referenced Nov 7, 2019

Add processTimeout for RPC Message #29345

Closed

Add processTimeout for RPC Message #29347

Closed

pietern reviewed Nov 7, 2019

View reviewed changes

xush6528 changed the title ~~Pass global_process_timeout to RpcAgent~~ Lift rpc_timeout to RpcAgent, for other RpcAgents to reuse. Nov 7, 2019

mrshenli reviewed Nov 7, 2019

View reviewed changes

mrshenli approved these changes Nov 7, 2019

View reviewed changes

Update on "Lift rpc_timeout to RpcAgent, for other RpcAgents to reuse."

ad79043

#29336 So that other RpcAgent could use this timeout setting as well. Differential Revision: [D5681951](https://our.internmc.facebook.com/intern/diff/D5681951/) [ghstack-poisoned]

xush6528 mentioned this pull request Nov 7, 2019

[distributed] allow rpc timeout to be set from python for different rpc backends #29068

Closed

facebook-github-bot closed this in e66626a Nov 8, 2019

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

facebook-github-bot deleted the gh/xush6528/30/head branch November 11, 2019 15:16

pritamdamania87 reviewed Nov 12, 2019

View reviewed changes

xush6528 mentioned this pull request Nov 12, 2019

Remove thin Python wrapper in RPC module to avoid performance loss, also put docstring into rpc/init.cpp C++ Python bindings #29628

Closed

mruberry added the Merged label Oct 28, 2020

Lift rpc_timeout to RpcAgent, for other RpcAgents to reuse. #29341

Lift rpc_timeout to RpcAgent, for other RpcAgents to reuse. #29341

Uh oh!

Conversation

xush6528 commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 8, 2019

Uh oh!

pietern commented Nov 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohan-varma Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xush6528 commented Nov 7, 2019 •

edited

Loading

xush6528 commented Nov 7, 2019 •

edited

Loading

rohan-varma Nov 12, 2019 •

edited

Loading