Add abortWaitSend/abortWaitRecv to unbound_buffer #228

rohan-varma · 2019-10-30T03:44:12Z

Summary: Closes #227.
Adds 2 methods: abortWaitSend, abortWaitRecv so that we can interrupt a blocking waitRecv or waitSend, per @pietern 's suggestion. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully.

The methods set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. The implementation is both for the tcp and uv transport methods. Unit tests are also added.

Differential Revision: D18210908

facebook-github-bot · 2019-10-30T03:44:34Z

This pull request was exported from Phabricator. Differential Revision: D18210908

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: e38d7b308e501fbeea5ff804bd9fe9f5b207c08a

facebook-github-bot · 2019-10-30T03:47:01Z

This pull request was exported from Phabricator. Differential Revision: D18210908

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: 4f904b8a208a1afe41bb81c4bbdb22fb10bda320

facebook-github-bot · 2019-10-30T03:51:19Z

This pull request was exported from Phabricator. Differential Revision: D18210908

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: d71d5d46b1eb9294bd35ac6b7d5622efd1d71901

facebook-github-bot · 2019-10-30T03:58:20Z

This pull request was exported from Phabricator. Differential Revision: D18210908

xush6528 · 2019-10-30T05:48:34Z

gloo/transport/tcp/unbound_buffer.cc

    }
  }
-
+  auto recvCompleted = recvCompletions_ > 0;


Can't understand this state, recvCompleted. Mind adding comments to explain it a bit?

Shouldn't a user call waitRecv(..) for N times, where N == the recvCompletions_ count?

Sure, I'll add some comments.

The goal here is to return true if the receive completed (i.e. we got data from the remote side) and false if it was aborted. One way to know that we got data from the remote side is if there was at least one recvCompletion, so I used this to determine the return value. We could potentially also use !abortWaitRecv, but we could run into an issue where the receive completes successfully, then a thread sets abortWaitRecv before we return out of waitRecv, in which case returning !abortWaitRecv would not be correct

Will this happen?
Before it returns out of waitRecv, doesn't it lock the mutex?

That makes sense, the lock would be reacquired once the thread is awakened. This should be changed to

if (abortWaitSend_): return false
else, decrement recvCompletions as below and return true

xush6528 · 2019-10-30T05:56:48Z

gloo/test/send_recv_test.cc

+        buf->recv(ranks, slot);
+        recvCompleted = buf->waitRecv(&srcRank);


Not quite understand the concepts from gloo.

What's the purpose of calling buf->recv(..) first?

From what I understand, buf->recv will call the recv function from a particular pair (basically an encapsulation of 2 gloo processes that can communicate), which notifies the sender that there is a receive pending, but does not block. Then, waitRecv blocks until Pair::read indicates that data has been received.

I see, receive operation needs a signal first.
And it's similar code to ProcessGroupGloo::recv and ProcessGroupGloo::recvAnysource.

For normal recv, it's necessary. Otherwise, RecvWork will wait for nothing and block forever.

Since you are aborting anyway, I think these 2 lines could be removed to make the test subject highlighted.

std::vector<int> ranks = {1}; buf->recv(ranks, slot);

like your AbortSend test below, you don't call buf->send(...) before wait.

pietern

Thanks for working on this, @rohan-varma.

pietern · 2019-10-30T15:49:54Z

gloo/transport/tcp/unbound_buffer.cc

    }
  }
-
+  auto recvCompleted = recvCompletions_ > 0;


If it was aborted, it should return here and not try and decrement recvCompletions_. This counter is a tally that counts the number of recv's that completed without an accompanying waitRecv call. If it is non-zero when waitRecv is called, waitRecv can return immediately without blocking (as evidenced by the condition if (recvCompletions_ == 0)).

pietern · 2019-10-30T15:50:09Z

gloo/transport/tcp/unbound_buffer.cc

  if (rank != nullptr) {
    *rank = recvRank_;
  }
+  return recvCompleted;


If we have acknowledged this completion, we must always return true.

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: 588d85eb2ed4c6a0270912bbd3b11298b41f3e37

facebook-github-bot · 2019-10-30T23:14:50Z

This pull request was exported from Phabricator. Differential Revision: D18210908

rohan-varma · 2019-10-30T23:18:51Z

@pietern @xush6528 Updated with the suggestions.

xush6528

Looks good now

xush6528 · 2019-10-31T00:07:32Z

gloo/test/send_recv_test.cc

+      auto buf = context->createUnboundBuffer(&tmp, sizeof(tmp));
+
+      std::thread waitRecvThread([&]() {
+        std::vector<int> ranks = {1};


This is not used.

pietern

Besides the unused variable comments from @xush6528, we also need to determine what to do for subsequent calls. Right now, all future calls to waitSend and waitRecv will return false if abort has been called once. But the intent is more likely to just interrupt a single call, so we should toggle the abort flag back to false if it was true once.

rohan-varma · 2019-10-31T17:48:19Z

Right now, all future calls to waitSend and waitRecv will return false if abort has been called once. But the intent is more likely to just interrupt a single call, so we should toggle the abort flag back to false if it was true once.

That is a good catch. We should flip the flag like you mentioned

xush6528 · 2019-10-31T20:52:14Z

gloo/transport/tcp/unbound_buffer.cc

  }

+  if (abortWaitSend_) {
+    return false;


So flip it back here.

if (abortWaitSend_) { abortWaitSend_ = false; return false; }

Also need a test case for testing an "abort" call only cancles one "wait" call?

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: e0a47edfac536492163fa4baae4d911dcaaaeac4

facebook-github-bot · 2019-10-31T22:59:16Z

This pull request was exported from Phabricator. Differential Revision: D18210908

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: 83a7dc66734d06f517771dcb622c6c5e40b809ff

facebook-github-bot · 2019-10-31T23:26:18Z

This pull request was exported from Phabricator. Differential Revision: D18210908

facebook-github-bot · 2019-10-31T23:41:15Z

This pull request was exported from Phabricator. Differential Revision: D18210908

xush6528

Latest version looks good.

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: fa3ba49e582e97929441fccb31f54ee137b64b39

facebook-github-bot · 2019-11-01T05:25:06Z

This pull request was exported from Phabricator. Differential Revision: D18210908

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: b8f3bba514c556d6fdf512ec488a26e5c179b2e9

facebook-github-bot · 2019-11-01T17:06:25Z

This pull request was exported from Phabricator. Differential Revision: D18210908

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: 22f586d913783f589922ce131a6ef1359b310561

facebook-github-bot · 2019-11-01T23:28:58Z

This pull request was exported from Phabricator. Differential Revision: D18210908

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Differential Revision: D18210908 fbshipit-source-id: d5c1c6a2776469fc943b88ea592aa48dcf1738a6

facebook-github-bot · 2019-11-01T23:35:44Z

This pull request was exported from Phabricator. Differential Revision: D18210908

Summary: Pull Request resolved: pytorch#228 Adds abortWaitSend, abortWaitRecv so that we can interrupt a blocking `waitRecv` or `waitSend`. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The APIs set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. Unit tests are also added. Reviewed By: pietern Differential Revision: D18210908 fbshipit-source-id: 0ed05d1abff2c656299542637cb18b918c9a9893

facebook-github-bot · 2019-11-04T21:38:02Z

This pull request was exported from Phabricator. Differential Revision: D18210908

facebook-github-bot

@rohan-varma is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Closes pytorch#227. Adds 2 methods: `abortWaitSend`, `abortWaitRecv` so that we can interrupt a blocking `waitRecv` or `waitSend`, per pietern 's suggestion. This is useful if a gloo process is waiting for a message in one thread, but another thread decides to shutdown and wants to stop it gracefully. The methods set a boolean variable and notify a waiting condition variable. The condition variable predicate is changed to check this condition. The implementation is both for the tcp and uv transport methods. Unit tests are also added. Pull Request resolved: pytorch#228 Test Plan: Added relevant unit tests in `send_recv_test.cc` Reviewed By: pietern Differential Revision: D18210908 Pulled By: pietern fbshipit-source-id: 013206a77edd7122c9ff6a120f3fbf41d5e75a56

facebook-github-bot · 2019-11-05T01:09:21Z

This pull request was exported from Phabricator. Differential Revision: D18210908

facebook-github-bot · 2019-11-05T05:01:44Z

@rohan-varma merged this pull request in d3554fb.

pietern · 2019-11-05T10:33:51Z

@rohan-varma What was the issue in the libuv code?

rohan-varma · 2019-11-05T17:56:53Z

@rohan-varma What was the issue in the libuv code?

In uv/unbound_buffer.cc, std::lock_guard<std::mutex> guard(m_); should be std::lock_guard<std::mutex> guard(mutex_). This results in the uv code not building (sadly wasn't caught by any of the build systems )

xush6528 · 2019-11-05T19:41:16Z

Redo in #232.

facebook-github-bot added the CLA Signed label Oct 30, 2019

rohan-varma force-pushed the export-D18210908 branch from 8d3fb83 to b5c9dcc Compare October 30, 2019 03:46

rohan-varma force-pushed the export-D18210908 branch from b5c9dcc to c341929 Compare October 30, 2019 03:51

rohan-varma force-pushed the export-D18210908 branch from c341929 to 8a7e42d Compare October 30, 2019 03:58

rohan-varma requested review from mrshenli and pietern October 30, 2019 04:05

xush6528 reviewed Oct 30, 2019

View reviewed changes

pietern reviewed Oct 30, 2019

View reviewed changes

rohan-varma force-pushed the export-D18210908 branch from 8a7e42d to 48480d7 Compare October 30, 2019 23:14

xush6528 approved these changes Oct 31, 2019

View reviewed changes

pietern reviewed Oct 31, 2019

View reviewed changes

xush6528 reviewed Oct 31, 2019

View reviewed changes

rohan-varma force-pushed the export-D18210908 branch from 48480d7 to 3507b19 Compare October 31, 2019 22:59

rohan-varma force-pushed the export-D18210908 branch from 3507b19 to 75241f9 Compare October 31, 2019 23:26

xush6528 approved these changes Nov 1, 2019

View reviewed changes

rohan-varma force-pushed the export-D18210908 branch from 7649c04 to 105d763 Compare November 1, 2019 05:25

rohan-varma force-pushed the export-D18210908 branch from 105d763 to cc60340 Compare November 1, 2019 17:06

rohan-varma force-pushed the export-D18210908 branch from cc60340 to ca10ed8 Compare November 1, 2019 23:28

rohan-varma force-pushed the export-D18210908 branch from ca10ed8 to 7d44293 Compare November 1, 2019 23:35

rohan-varma mentioned this pull request Nov 4, 2019

Properly shutdown RPC even in the case of clean_shutdown=False. pytorch/pytorch#29148

Closed

rohan-varma force-pushed the export-D18210908 branch from 7d44293 to 8fa59ac Compare November 4, 2019 21:37

facebook-github-bot reviewed Nov 4, 2019

View reviewed changes

rohan-varma force-pushed the export-D18210908 branch from 8fa59ac to 62a2660 Compare November 5, 2019 01:09

facebook-github-bot closed this in d3554fb Nov 5, 2019

rohan-varma mentioned this pull request Nov 5, 2019

[build] update gloo submodule pytorch/pytorch#29184

Closed

facebook-github-bot added the Merged label Nov 5, 2019

		buf->recv(ranks, slot);
		recvCompleted = buf->waitRecv(&srcRank);

Add abortWaitSend/abortWaitRecv to unbound_buffer #228

Add abortWaitSend/abortWaitRecv to unbound_buffer #228

Uh oh!

Conversation

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

Uh oh!

Uh oh!

facebook-github-bot commented Oct 30, 2019

Uh oh!

facebook-github-bot commented Oct 30, 2019

Uh oh!

facebook-github-bot commented Oct 30, 2019

Uh oh!

facebook-github-bot commented Oct 30, 2019

Uh oh!

xush6528 Oct 30, 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!

rohan-varma Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

xush6528 Oct 30, 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!

rohan-varma Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

xush6528 Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

rohan-varma Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

xush6528 Oct 30, 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!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

pietern Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

pietern Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 30, 2019

Uh oh!

rohan-varma commented Oct 30, 2019

Uh oh!

xush6528 left a comment

Choose a reason for hiding this comment

Uh oh!

xush6528 Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

rohan-varma commented Oct 31, 2019

Uh oh!

xush6528 Oct 31, 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!

facebook-github-bot commented Oct 31, 2019

Uh oh!

facebook-github-bot commented Oct 31, 2019

Uh oh!

facebook-github-bot commented Oct 31, 2019

Uh oh!

xush6528 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 1, 2019

Uh oh!

facebook-github-bot commented Nov 1, 2019

Uh oh!

rohan-varma commented Oct 30, 2019 •

edited

Loading

xush6528 Oct 30, 2019 •

edited

Loading

xush6528 Oct 30, 2019 •

edited

Loading

xush6528 Oct 30, 2019 •

edited

Loading

xush6528 Oct 31, 2019 •

edited

Loading

rohan-varma commented Nov 5, 2019 •

edited

Loading