Change type of timeoutFutures_ key to time_point instead of duration #31078

xush6528 · 2019-12-10T22:04:38Z

Stack from ghstack:

Tweak pollTimedOutRPCs thread synchronization #30355 Tweak pollTimedOutRPCs thread synchronization
Change type of timeoutFutures_ key to time_point instead of duration #31078 Change type of timeoutFutures_ key to time_point instead of duration

Make ProcessGroupAgent::pollTimedOutRPCs code more conventional.

Use std::chrono::time_point to represent endTime instead of std::chrono::duration.
Replace std::condition_variable::wait_for(lock, endTime) with std::condition_variable::wait_until(lock, endTime).
Remove the unnecessary ::getRPCRemainingTime().

Differential Revision: D5705442

Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, entTime)` with `std::condition_variable::wait_until(lock, entTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. Differential Revision: [D5705442](https://our.internmc.facebook.com/intern/diff/D5705442/) [ghstack-poisoned]

torch/csrc/distributed/rpc/process_group_agent.cpp

rohan-varma · 2019-12-10T22:14:04Z

torch/csrc/distributed/rpc/process_group_agent.h

+  using steady_clock_time_point =
+      std::chrono::time_point<std::chrono::steady_clock>;
+
+  static constexpr steady_clock_time_point kInfiniteTimeoutTimePoint =


Should this go in rpc_agent.h so that other RpcAgents can have access to them?

I don't think so. This watch dog thread and using a const time to represent infinity are all specific to ProcessGroupAgent only.

Notice this is not the "Default RPC timeout".

rohan-varma · 2019-12-10T22:24:01Z

torch/csrc/distributed/rpc/process_group_agent.cpp

      futureTimeoutCV_.wait(lock);
    } else {
-      futureTimeoutCV_.wait_for(lock, sleepTime);
+      futureTimeoutCV_.wait_until(lock, minEndTime);


I think you can get rid of the sleepTime variable now?

Right. Let me update.

mrshenli

LGTM! Please get a stamp from @rohan-varma as well.

There is typo in the PR description entTime -> endTime. Please make sure the final commit message has the correct spelling. Thanks!

…f duration" Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, entTime)` with `std::condition_variable::wait_until(lock, entTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. Differential Revision: [D5705442](https://our.internmc.facebook.com/intern/diff/D5705442/) [ghstack-poisoned]

xush6528 · 2019-12-10T22:40:30Z

@mrshenli Thanks! updated PR description.

…f duration" Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, endTime)` with `std::condition_variable::wait_until(lock, endTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. Differential Revision: [D5705442](https://our.internmc.facebook.com/intern/diff/D5705442/) [ghstack-poisoned]

Pull Request resolved: #31078 Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, endTime)` with `std::condition_variable::wait_until(lock, endTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. ghstack-source-id: 95316304 Differential Revision: [D5705442](https://our.internmc.facebook.com/intern/diff/D5705442/)

rohan-varma

Looks good, please wait for tests to pass.

…f duration" Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, endTime)` with `std::condition_variable::wait_until(lock, endTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. Differential Revision: [D5705442](https://our.internmc.facebook.com/intern/diff/D5705442/) [ghstack-poisoned]

jjlilley · 2019-12-11T20:53:03Z

torch/csrc/distributed/rpc/process_group_agent.h

+using steady_clock_time_point =
+    std::chrono::time_point<std::chrono::steady_clock>;
+
+constexpr steady_clock_time_point kInfiniteTimeoutTimePoint =


why do you need this in the .h?

@jjlilley Wanted to put it in the class declaration.

Then I got this error in macos test
https://circleci.com/gh/pytorch/pytorch/3890383?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Dec 11 01:39:38 + python -c 'import torch' Dec 11 01:39:38 Traceback (most recent call last): Dec 11 01:39:38 File "<string>", line 1, in <module> Dec 11 01:39:38 File "/Users/distiller/project/miniconda/envs/wheel_py36/lib/python3.6/site-packages/torch/__init__.py", line 81, in <module> Dec 11 01:39:38 from torch._C import * Dec 11 01:39:38 ImportError: dlopen(/Users/distiller/project/miniconda/envs/wheel_py36/lib/python3.6/site-packages/torch/_C.cpython-36m-darwin.so, 9): Symbol not found: __ZN5torch11distributed3rpc17ProcessGroupAgent25kInfiniteTimeoutTimePointE Dec 11 01:39:38 Referenced from: /Users/distiller/project/miniconda/envs/wheel_py36/lib/python3.6/site-packages/torch/lib/libtorch_python.dylib Dec 11 01:39:38 Expected in: flat namespace Dec 11 01:39:38 in /Users/distiller/project/miniconda/envs/wheel_py36/lib/python3.6/site-packages/torch/lib/libtorch_python.dylib

Let me put it into process_group_agent.cpp

jjlilley · 2019-12-11T20:53:18Z

torch/csrc/distributed/rpc/process_group_agent.h

 namespace distributed {
 namespace rpc {

+using steady_clock_time_point =


can this be a class-scope using, rather than exposed throughout all distributed::rpc scope?

@jjlilley Let me put it back under ProcessGroupAgent class declaration.

…f duration" Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, endTime)` with `std::condition_variable::wait_until(lock, endTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. Differential Revision: [D5705442](https://our.internmc.facebook.com/intern/diff/D5705442/) [ghstack-poisoned]

jjlilley

LGTM, pending circleci, thanks!

…f duration" Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, endTime)` with `std::condition_variable::wait_until(lock, endTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. Differential Revision: [D5705442](https://our.internmc.facebook.com/intern/diff/D5705442/) [ghstack-poisoned]

Pull Request resolved: #31078 Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, endTime)` with `std::condition_variable::wait_until(lock, endTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. ghstack-source-id: 95408482 Differential Revision: [D5705442](https://our.internmc.facebook.com/intern/diff/D5705442/)

kostmo · 2019-12-12T01:43:11Z

CircleCI build failures summary

As of commit 20c3823:

1/2 broken upstream at merge base e5a550c (see grid view)
- You may want to rebase on the viable/strict branch (see its recency history):
  - If your commit is newer than viable/strict, you can try basing on an older, stable commit:
```
git fetch viable/strict
git rebase --onto viable/strict $(git merge-base origin/master HEAD)
```
  - If your commit is older than viable/strict:
```
git fetch viable/strict
git rebase viable/strict
```
1/2 failures introduced in this PR

Detailed failure analysis

One may explore the probable reasons each build failed interactively on the Dr. CI website.

1 new failure recognized by patterns

The following build failures don't appear to be due to upstream breakage:

pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-build (1/1)

Step: "pytorch android gradle build" (details)

sys	0m0.159s
++ docker run --cap-add=SYS_PTRACE --security-opt seccomp=unconfined -t -d -w /var/lib/jenkins 308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-py3-clang5-android-ndk-r19c:405-20c3823db973579c386ceb6bcab53143cf9ccc00-android-x86_32
+ export id_x86_32=40c9aeb6161cf211c89cfc8d390a3bd6c7f3b436127ab1308d9bf866990f493a
+ id_x86_32=40c9aeb6161cf211c89cfc8d390a3bd6c7f3b436127ab1308d9bf866990f493a
+ export 'COMMAND=((echo "source ./workspace/env" && echo "sudo chown -R jenkins workspace") | docker exec -u jenkins -i "$id_x86_32" bash) 2>&1'
+ COMMAND='((echo "source ./workspace/env" && echo "sudo chown -R jenkins workspace") | docker exec -u jenkins -i "$id_x86_32" bash) 2>&1'
+ echo '((echo' '"source' './workspace/env"' '&&' echo '"sudo' chown -R jenkins 'workspace")' '|' docker exec -u jenkins -i '"$id_x86_32"' 'bash)' '2>&1'
+ unbuffer bash ./command.sh
+ ts
+ docker pull 308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-py3-clang5-android-ndk-r19c:405-20c3823db973579c386ceb6bcab53143cf9ccc00-android-arm-v7a
Error response from daemon: manifest for 308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-py3-clang5-android-ndk-r19c:405-20c3823db973579c386ceb6bcab53143cf9ccc00-android-arm-v7a not found

1 upstream failure recognized by patterns:

These builds matched patterns, but were probably caused by upstream breakages:

binary_macos_libtorch_3_7_cpu_build

This comment was automatically generated by Dr. CI.
Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 1 time.

facebook-github-bot · 2019-12-13T01:46:56Z

This pull request has been merged in a3ed350.

…ytorch#31078) Summary: Pull Request resolved: pytorch#31078 Make `ProcessGroupAgent::pollTimedOutRPCs` code more conventional. - Use `std::chrono::time_point` to represent `endTime` instead of `std::chrono::duration`. - Replace `std::condition_variable::wait_for(lock, endTime)` with `std::condition_variable::wait_until(lock, endTime)`. - Remove the unnecessary `::getRPCRemainingTime()`. ghstack-source-id: 95408482 Test Plan: # Unit tests ``` buck test mode/dev-nosan //caffe2/test:rpc_fork -- test_rpc_timeouts buck-out/gen/caffe2/test/rpc_fork\#binary.par -r test_rpc_timeouts ``` ``` buck test mode/dev-nosan //caffe2/test:rpc_fork_thrift -- test_rpc_timeouts buck-out/gen/caffe2/test/rpc_fork_thrift\#binary.par -r test_rpc_timeouts ``` Differential Revision: D5705442 fbshipit-source-id: ba54b7bdb84bc02d05c22360b01290d044bbfcf5

xush6528 requested review from mrshenli, pritamdamania87 and zhaojuanmao as code owners December 10, 2019 22:04

xush6528 mentioned this pull request Dec 10, 2019

Change startTime_ to endTime_ in FutureInfo #30342

Closed

xush6528 requested a review from rohan-varma December 10, 2019 22:06

rohan-varma reviewed Dec 10, 2019

View reviewed changes

torch/csrc/distributed/rpc/process_group_agent.cpp Show resolved Hide resolved

rohan-varma reviewed Dec 10, 2019

View reviewed changes

mrshenli approved these changes Dec 10, 2019

View reviewed changes

xush6528 mentioned this pull request Dec 10, 2019

Tweak pollTimedOutRPCs thread synchronization #30355

Closed

rohan-varma self-requested a review December 10, 2019 23:15

rohan-varma approved these changes Dec 10, 2019

View reviewed changes

jjlilley reviewed Dec 11, 2019

View reviewed changes

jjlilley approved these changes Dec 11, 2019

View reviewed changes

facebook-github-bot closed this in a3ed350 Dec 12, 2019

facebook-github-bot added the merged label Dec 13, 2019

facebook-github-bot deleted the gh/xush6528/48/head branch December 15, 2019 15:16

mruberry added the Merged label Oct 28, 2020

Change type of timeoutFutures_ key to time_point instead of duration #31078

Change type of timeoutFutures_ key to time_point instead of duration #31078

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

rohan-varma Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

xush6528 Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

rohan-varma Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

xush6528 Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 commented Dec 10, 2019

Uh oh!

rohan-varma left a comment

Choose a reason for hiding this comment

Uh oh!

jjlilley Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

xush6528 Dec 11, 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!

jjlilley Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

xush6528 Dec 11, 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!

jjlilley left a comment

Choose a reason for hiding this comment

Uh oh!

kostmo commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CircleCI build failures summary

Detailed failure analysis

1 new failure recognized by patterns

pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-build (1/1)

1 upstream failure recognized by patterns:

Uh oh!

facebook-github-bot commented Dec 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

xush6528 commented Dec 10, 2019 •

edited

Loading

xush6528 Dec 11, 2019 •

edited

Loading

xush6528 Dec 11, 2019 •

edited

Loading

kostmo commented Dec 12, 2019 •

edited

Loading