Use Future's then() API to fix RPC profiling by rohan-varma · Pull Request #38352 · pytorch/pytorch

rohan-varma · 2020-05-12T20:11:37Z

Stack from ghstack:

Use Future's then() API to fix RPC profiling #38352 Use Future's then() API to fix RPC profiling

Fixes the RPC profiling by using the then() API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value)

To make this work for RRef, we add a _set_profiling_future to set the profiling future, and _get_profiling_future to retrieve this future and wait on it in the tests.

Re-enabled profiling tests and stress tested them 1000 times to verify the fix

Differential Revision: D21506940

Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/) [ghstack-poisoned]

dr-ci · 2020-05-12T20:22:50Z

💊 CI failures summary and remediations

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

1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

caffe2_onnx_main_py3_6_clang7_ubuntu16_04_test (1/1)

Step: "Test" (full log | diagnosis details | 🔁 rerun)

May 14 07:37:27 E AssertionError: False is not true

May 14 07:37:27      
May 14 07:37:27         def shutdown_fun(): 
May 14 07:37:27             workspace.FeedBlob('data', 'shutdown') 
May 14 07:37:27      
May 14 07:37:27         worker_coordinator = parallel_workers.init_workers( 
May 14 07:37:27             dummy_worker, shutdown_fun=shutdown_fun 
May 14 07:37:27         ) 
May 14 07:37:27         worker_coordinator.start() 
May 14 07:37:27      
May 14 07:37:27 >       self.assertTrue(worker_coordinator.stop()) 
May 14 07:37:27 E       AssertionError: False is not true 
May 14 07:37:27  
May 14 07:37:27 ../.local/lib/python3.6/site-packages/caffe2/python/parallel_workers_test.py:116: AssertionError 
May 14 07:37:27 ----------------------------- Captured stdout call ----------------------------- 
May 14 07:37:27 Wait for workers to die: train 
May 14 07:37:27 Worker <Thread(parallel_workers worker id 4, started daemon 139753781446400)> failed to close while waiting 
May 14 07:37:27 Worker <Thread(parallel_workers worker id 5, started daemon 139753789839104)> failed to close while waiting 
May 14 07:37:27 All workers terminated: False 
May 14 07:37:27 - generated xml file: /var/lib/jenkins/workspace/caffe2_tests/python/result.xml - 
May 14 07:37:27 ---------- onnx coverage: ---------- 
May 14 07:37:27 Operators (passed/loaded/total): 0/0/185

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 on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 21 times.

Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/) [ghstack-poisoned]

Pull Request resolved: #38352 Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix ghstack-source-id: 103957346 Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/)

Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/) [ghstack-poisoned]

Pull Request resolved: #38352 Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix ghstack-source-id: 103983281 Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/)

Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/) [ghstack-poisoned]

Pull Request resolved: #38352 Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix ghstack-source-id: 104042479 Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/)

mrshenli

Thanks for fixing! I added some minor comments.

mrshenli · 2020-05-13T23:19:07Z

        auto& rec = getRecordFunctionFromTensor(handle);
        rec._end();
-      });
+        return fut->constValue();


Let's add a comment to explain why we need to forward the return value here. IIUC, it is because _invoke_rpc actually returns this future?

Yeah, we get the future which has the actual result from _invoke_rpc. Then, if profiling we run this, and we want to ensure that the correct value of the future from _invoke_rpc is propagated. Will add this to the comment

mrshenli · 2020-05-13T23:25:13Z

-          _call_end_callbacks_on_fut(tensor, fut);
+          auto profiledFut = _call_end_callbacks_on_fut(tensor, fut);
+          // return future that completes when profiling callbacks have run.
+          jit::push(stack, profiledFut);


IIUC, push takes rvalue reference. If so, shall we use std::move() on the profilerFut?

mrshenli · 2020-05-13T23:26:09Z

+          .def(
+              "_get_profiling_future",
+              [](const PyRRef& self) {
+                return std::make_shared<jit::PythonFutureWrapper>(
+                    self.getProfilingFuture());
+              },
+              py::call_guard<py::gil_scoped_release>(),
+              R"(
+                  Returns future that completes when the profiling event corresponding
+                  to the creation of this RRef on the remote node has been recorded.
+              )")
+          .def(
+              "_set_profiling_future",
+              [](PyRRef& self,
+                 const std::shared_ptr<jit::PythonFutureWrapper>&
+                     wrappedFuture) {
+                self.setProfilingFuture(wrappedFuture->fut);
+              },
+              py::call_guard<py::gil_scoped_release>(),


These two seems are very light functions, should I hold GIL instead to avoid ctx switch?

mrshenli

Lint failure is real.

  {
    path: 'torch/testing/_internal/distributed/rpc/jit/rpc_test.py',
    start_line: 2,
    end_line: 2,
    start_column: 1,
    end_column: 1,
    annotation_level: 'failure',
    message: "[F401] 'unittest' imported but unused"
  }

Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/) [ghstack-poisoned]

Pull Request resolved: #38352 Fixes the RPC profiling by using the `then()` API added in #37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix ghstack-source-id: 104086114 Differential Revision: [D21506940](https://our.internmc.facebook.com/intern/diff/D21506940/)

mrshenli

LGTM! Let's land!

facebook-github-bot · 2020-05-14T20:12:33Z

This pull request has been merged in 4d4895a.

Summary: Pull Request resolved: pytorch#38352 Fixes the RPC profiling by using the `then()` API added in pytorch#37311. Instead of adding a regular callback, we return a new future that completes when the profiling callback is finished. This is transparent to the user as the future still completes with the value of the original future (i.e. the RPC's return value) To make this work for RRef, we add a `_set_profiling_future` to set the profiling future, and `_get_profiling_future` to retrieve this future and wait on it in the tests. Re-enabled profiling tests and stress tested them 1000 times to verify the fix ghstack-source-id: 104086114 Test Plan: Re-enabled profiling tests Differential Revision: D21506940 fbshipit-source-id: 35cde22f0551c825c9bc98ddc24cca412878a63a

rohan-varma requested review from albanD, apaszke, mrshenli, pritamdamania87 and zhaojuanmao as code owners May 12, 2020 20:11

mrshenli approved these changes May 13, 2020

View reviewed changes

mrshenli requested changes May 13, 2020

View reviewed changes

rohan-varma requested a review from mrshenli May 14, 2020 07:07

mrshenli approved these changes May 14, 2020

View reviewed changes

facebook-github-bot closed this in 4d4895a May 14, 2020

facebook-github-bot added the merged label May 14, 2020

This was referenced May 16, 2020

DISABLED test_profiler_with_async_rpc_builtin (__main__.RpcTestWithSpawn) #37557

Closed

DISABLED test_profiler_with_sync_rpc_udf (__main__.RpcTestWithSpawn) #38008

Closed

DISABLED test_profiler_with_async_rpc_udf (__main__.RpcTestWithSpawn) #37889

Closed

facebook-github-bot deleted the gh/rohan-varma/127/head branch May 18, 2020 14:16

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Future's then() API to fix RPC profiling#38352

Use Future's then() API to fix RPC profiling#38352
rohan-varma wants to merge 6 commits intogh/rohan-varma/127/basefrom
gh/rohan-varma/127/head

rohan-varma commented May 12, 2020 •

edited

Loading

Uh oh!

dr-ci Bot commented May 12, 2020 •

edited

Loading

Uh oh!

mrshenli left a comment

Uh oh!

mrshenli May 13, 2020

Uh oh!

rohan-varma May 14, 2020

Uh oh!

mrshenli May 13, 2020

Uh oh!

mrshenli May 13, 2020

Uh oh!

mrshenli left a comment

Uh oh!

mrshenli left a comment

Uh oh!

facebook-github-bot commented May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Uh oh!

dr-ci Bot commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

caffe2_onnx_main_py3_6_clang7_ubuntu16_04_test (1/1)

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

mrshenli May 13, 2020

Choose a reason for hiding this comment

Uh oh!

rohan-varma May 14, 2020

Choose a reason for hiding this comment

Uh oh!

mrshenli May 13, 2020

Choose a reason for hiding this comment

Uh oh!

mrshenli May 13, 2020

Choose a reason for hiding this comment

Uh oh!

mrshenli left a comment

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 May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rohan-varma commented May 12, 2020 •

edited

Loading

dr-ci Bot commented May 12, 2020 •

edited

Loading