[RPC] Add option to make rref.get_type not block. #50977

rohan-varma · 2021-01-23T03:09:00Z

Stack from ghstack:

[JIT/Futures] Support set_exception API on python future. #50983 [JIT/Futures] Support set_exception API on python future.
[torch.futures] Fix doc inconsistency about callback args #50979 [torch.futures] Fix doc inconsistency about callback args
[torch.futures] Clarify callback behavior when future is completed #50978 [torch.futures] Clarify callback behavior when future is completed
[RPC] Add option to make rref.get_type not block. #50977 [RPC] Add option to make rref.get_type not block.

Adds a blocking flag that can be set to False to make this API return a Future to the type. This is to make this function non-blocking, mostly for a future change that will allow rref.rpc_async() to be completely non-blocking (it currently calls and waits for this function that issues an RPC in-line).

Differential Revision: D25944582

Per title. This allows it not to block Differential Revision: [D25944582](https://our.internmc.facebook.com/intern/diff/D25944582/) [ghstack-poisoned]

facebook-github-bot · 2021-01-23T03:09:18Z

💊 CI failures summary and remediations

As of commit 77db3d0 (more details on the Dr. CI page):

2/2 failures possibly* introduced in this PR
- 2/2 non-CircleCI failure(s)

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

mrshenli · 2021-01-25T15:43:11Z

torch/distributed/rpc/api.py

+        # or owner.
+        future = Future()
+        future.set_result(rref_type)
+        return future


if this returns a future, do we need to annotate this function with an @rpc.functions.async_execution decorator?

I recall Future is not picklable, and we didn't plan to make Future picklable, as communicating a future to a remote process does not seem reasonable? E.g., if the future is not completed yet, do we need to update both local and remote futures when the local one is marked as completed?

This won't return a future when called remotely, as the only remote call is from the below _rref_typeof_on_user, and we call that with blocking (just need to get the type in that case).

I didn't decorate it with rpc.functions.async_execution for that reason, because its always ran synchronously when called over RPC. So I guess this means picklability of Future isn't a concern?

I think this could potentially cause issues later if this function is ran over RPC with blocking=False, but since its private I'm assuming use cases will be like the ones below.

mrshenli

Does it work if we always let the functions like getRRefType and _rref_typeof_on_owner return a future, and chain subsequent logic as a callback? It the Future is already completed when returning, subsequent logic will be run inline, which is the same as the current behavior. Otherwise, it's async.

mrshenli · 2021-01-25T15:53:46Z

torch/csrc/distributed/rpc/py_rref.cpp

  }
-
+  // Returns py::object that can Python type or future.


can -> can be

Adds a `blocking` flag that can be set to False to make this API return a `Future` to the type. This is to make this function non-blocking, mostly for a future change that will allow `rref.rpc_async()` to be completely non-blocking (it currently calls and waits for this function that issues an RPC in-line). Differential Revision: [D25944582](https://our.internmc.facebook.com/intern/diff/D25944582/) [ghstack-poisoned]

rohan-varma · 2021-01-26T06:38:32Z

Does it work if we always let the functions like getRRefType and _rref_typeof_on_owner return a future, and chain subsequent logic as a callback? It the Future is already completed when returning, subsequent logic will be run inline, which is the same as the current behavior. Otherwise, it's async.

This should work, will add unittests.

Edit: discussed offline that we will consider doing this in a follow up change.

Adds a `blocking` flag that can be set to False to make this API return a `Future` to the type. This is to make this function non-blocking, mostly for a future change that will allow `rref.rpc_async()` to be completely non-blocking (it currently calls and waits for this function that issues an RPC in-line). Differential Revision: [D25944582](https://our.internmc.facebook.com/intern/diff/D25944582/) [ghstack-poisoned]

codecov · 2021-01-29T01:13:17Z

Codecov Report

Merging #50977 (77db3d0) into gh/rohan-varma/219/base (1518aee) will decrease coverage by 0.01%.
The diff coverage is 23.75%.

@@                     Coverage Diff                     @@
##           gh/rohan-varma/219/base   #50977      +/-   ##
===========================================================
- Coverage                    80.92%   80.91%   -0.02%     
===========================================================
  Files                         1948     1948              
  Lines                       212255   212313      +58     
===========================================================
+ Hits                        171777   171787      +10     
- Misses                       40478    40526      +48

rohan-varma · 2021-02-01T20:34:28Z

@mrshenli Do you mind taking another look at this one? Thanks!

Adds a `blocking` flag that can be set to False to make this API return a `Future` to the type. This is to make this function non-blocking, mostly for a future change that will allow `rref.rpc_async()` to be completely non-blocking (it currently calls and waits for this function that issues an RPC in-line). Differential Revision: [D25944582](https://our.internmc.facebook.com/intern/diff/D25944582/) [ghstack-poisoned]

mrshenli

LGTM! Shall we create an issue to track followup changes?

Adds a `blocking` flag that can be set to False to make this API return a `Future` to the type. This is to make this function non-blocking, mostly for a future change that will allow `rref.rpc_async()` to be completely non-blocking (it currently calls and waits for this function that issues an RPC in-line). Differential Revision: [D25944582](https://our.internmc.facebook.com/intern/diff/D25944582/) [ghstack-poisoned]

facebook-github-bot · 2021-02-05T04:22:40Z

This pull request has been merged in c3f2f32.

[RPC] Add option to make rref.get_type not block.

5c5e878

Per title. This allows it not to block Differential Revision: [D25944582](https://our.internmc.facebook.com/intern/diff/D25944582/) [ghstack-poisoned]

rohan-varma requested review from mingzhe09088, mrshenli, pritamdamania87 and zhaojuanmao as code owners January 23, 2021 03:09

facebook-github-bot added cla signed oncall: distributed Add this issue/PR to distributed oncall triage queue labels Jan 23, 2021

This was referenced Jan 23, 2021

[torch.futures] Clarify callback behavior when future is completed #50978

Closed

[torch.futures] Fix doc inconsistency about callback args #50979

Closed

[JIT/Futures] Support set_exception API on python future. #50983

Closed

mrshenli reviewed Jan 25, 2021

View reviewed changes

rohan-varma added 2 commits January 26, 2021 18:59

rohan-varma requested a review from mrshenli January 27, 2021 18:50

rohan-varma added 2 commits January 27, 2021 11:08

mrshenli approved these changes Feb 2, 2021

View reviewed changes

rohan-varma mentioned this pull request Feb 3, 2021

[RPC] rref.get_type() should have default return type of future #51616

Open

rohan-varma added 2 commits February 3, 2021 12:04

facebook-github-bot closed this in c3f2f32 Feb 5, 2021

facebook-github-bot added the Merged label Feb 5, 2021

facebook-github-bot deleted the gh/rohan-varma/219/head branch February 8, 2021 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RPC] Add option to make rref.get_type not block. #50977

[RPC] Add option to make rref.get_type not block. #50977

rohan-varma commented Jan 23, 2021 •

edited

facebook-github-bot commented Jan 23, 2021 •

edited

mrshenli Jan 25, 2021

mrshenli Jan 25, 2021

rohan-varma Jan 25, 2021 •

edited

mrshenli left a comment

mrshenli Jan 25, 2021

rohan-varma commented Jan 26, 2021 •

edited

codecov bot commented Jan 29, 2021 •

edited

rohan-varma commented Feb 1, 2021

mrshenli left a comment

facebook-github-bot commented Feb 5, 2021

[RPC] Add option to make rref.get_type not block. #50977

[RPC] Add option to make rref.get_type not block. #50977

Conversation

rohan-varma commented Jan 23, 2021 • edited

facebook-github-bot commented Jan 23, 2021 • edited

💊 CI failures summary and remediations

mrshenli Jan 25, 2021

Choose a reason for hiding this comment

mrshenli Jan 25, 2021

Choose a reason for hiding this comment

rohan-varma Jan 25, 2021 • edited

Choose a reason for hiding this comment

mrshenli left a comment

Choose a reason for hiding this comment

mrshenli Jan 25, 2021

Choose a reason for hiding this comment

rohan-varma commented Jan 26, 2021 • edited

codecov bot commented Jan 29, 2021 • edited

Codecov Report

rohan-varma commented Feb 1, 2021

mrshenli left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 5, 2021

rohan-varma commented Jan 23, 2021 •

edited

facebook-github-bot commented Jan 23, 2021 •

edited

rohan-varma Jan 25, 2021 •

edited

rohan-varma commented Jan 26, 2021 •

edited

codecov bot commented Jan 29, 2021 •

edited