C10D: Added TCPStore to support C10D store interface #7560

teng-li · 2018-05-15T01:51:50Z

Added TCP Store as another Store for the C10D interface.

For more details on the C10D Store interface, refer: #7434

A few things done in the PR:

(1) TCP Store can be initialized either as a master who will launch a daemon thread or a slave who will connect to the master's store daemon thread.
(2) TCP Store supports all the interface functions for Store.h
(3) Refactored some of the TCP helper functions into Utils.hoo/cpp
(4) Refactored test to some common folders.

Test Plan:

~/pytorch/torch/lib/c10d$ bin/test.sh
+ mkdir -p build
+ cd build
+ cmake ../
-- Configuring done
-- Generating done
-- Build files have been written to: /private/home/tengli/pytorch/torch/lib/c10d/build
+ make all test
[ 11%] Building CXX object CMakeFiles/store.dir/Utils.cpp.o
[ 22%] Building CXX object CMakeFiles/store.dir/Store.cpp.o
[ 33%] Building CXX object CMakeFiles/store.dir/FileStore.cpp.o
[ 44%] Building CXX object CMakeFiles/store.dir/TcpStore.cpp.o
[ 55%] Linking CXX static library libstore.a
[ 55%] Built target store
Scanning dependencies of target TcpStoreTest
[ 66%] Building CXX object test/CMakeFiles/TcpStoreTest.dir/TcpStoreTest.cpp.o
[ 77%] Linking CXX executable TcpStoreTest
[ 77%] Built target TcpStoreTest
[ 88%] Building CXX object test/CMakeFiles/FileStoreTest.dir/FileStoreTest.cpp.o
[100%] Linking CXX executable FileStoreTest
[100%] Built target FileStoreTest
Running tests...
Test project /private/home/tengli/pytorch/torch/lib/c10d/build
    Start 1: FileStoreTest
1/2 Test #1: FileStoreTest ....................   Passed    0.03 sec
    Start 2: TcpStoreTest
2/2 Test #2: TcpStoreTest .....................   Passed    0.69 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   0.74 sec

apaszke

It looks like most of the code is coming straight from THD, is that right? Can you please post diffs comparing them to the original files? That would make reviewing much easier.

torch/lib/c10d/Utils.cpp

+int connect(const std::string& address,
+            PortType port,
+            bool wait,
+            int timeout) {


torch/lib/c10d/Utils.hpp

+
+    bytesToReceive -= bytesReceived;
+    currentBytes += bytesReceived;
+  }


torch/lib/c10d/test/TcpStoreTest.cpp

+              c10d::test::set(*slaveStores[i], key, val);
+              c10d::test::check(*slaveStores[i], key, val);
+            }
+          })));


teng-li · 2018-05-15T04:38:26Z

@apaszke Utils.cpp/hpp are mostly from THD with some refactoring. TcpStore.hpp/cpp are based on architecture of Gloo store, but the API implementation needs some changes to support these new functions like wait with timeout, add, check functions that are defined in the new base store class. I sent you a diff file internally on messenger.

pietern

Nice work @teng-li!

General comments:

Can we name it TCPStore instead?
There is inconsistency in style, w.r.t. double newlines, braces on newline vs end of line, function argument indentation, namespace comments.
A few comments below talk about this: I think we should not let the exception propagation determine shutdown of the daemon thread. It will happen one node dies for whatever reason and this will immediately propagate everywhere. Instead it should just go away, be removed from the fd vector, and the daemon should keep running.

torch/lib/c10d/TcpStore.cpp

+      keysAwaited_.push_back(0);
+      fds.push_back({ .fd = sockFd, .events = POLLIN });
+    }
+    for (size_t rank = 0; rank < sockets_.size(); rank++) {


torch/lib/c10d/TcpStore.cpp

+
+    SYSCHECK(::poll(fds.data(), fds.size(), -1));
+
+    if (fds[0].revents != 0) {


torch/lib/c10d/TcpStore.cpp

+
+    if (fds[0].revents != 0) {
+      if (fds[0].revents ^ POLLIN) {
+        throw std::system_error(ECONNABORTED, std::system_category());


torch/lib/c10d/TcpStore.cpp

+         * side has been closed. If the closing was due to normal exit, then the
+         * store should exit too. Otherwise, if it was different exception,
+         * other processes will get an exception once they try to use the store.
+         */


torch/lib/c10d/TcpStore.cpp

+}
+
+void TcpStoreDaemon::join() {
+  daemonThread_.join();


torch/lib/c10d/TcpStore.cpp

+
+{
+  if (isServer_) {
+    // Openning up the listening socket


torch/lib/c10d/TcpStore.cpp

+    return false;
+  } else {
+    throw std::runtime_error("stop_waiting or keep_waiting response expected");
+  }


torch/lib/c10d/TcpStore.cpp

+ * or, in the case of wait
+ * type of query | number of args | size of arg1 | arg1 | ...
+ */
+void TcpStoreDaemon::query(RankType rank) {


torch/lib/c10d/TcpStore.cpp

+void TcpStore::set(const std::string& key, const std::vector<uint8_t>& data) {
+  tcputil::sendValue<QueryType>(storeSocket_, QueryType::SET);
+  tcputil::sendString(storeSocket_, key, true);
+  tcputil::sendVector<uint8_t>(storeSocket_, data);


torch/lib/c10d/test/CMakeLists.txt

@@ -13,4 +14,5 @@ foreach(test_src ${test_srcs})
  target_include_directories(${test_name} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/..)
  target_link_libraries(${test_name} ${test_libraries})
  add_test(NAME ${test_name} COMMAND $<TARGET_FILE:${test_name}>)
+


teng-li · 2018-05-16T01:50:32Z

@pietern Changed the file name to TCPStore, but somehow the new commit cannot see the diff :(, I can revert the name back so that you can see the diff and later rename it.

Anyway, addressed all of your comments, like properly shutdown the daemon using pipes etc.

@apaszke addressed your comments too,

Please see: pytorch#7434

torch/lib/c10d/TcpStore.cpp

+      ret = false;
+      it++;
+    } else {
+      it = keys.erase(it);


torch/lib/c10d/TcpStore.cpp

+    }
+    /* sleep override */
+    std::this_thread::sleep_for(std::chrono::milliseconds(10));
+  }


torch/lib/c10d/Utils.hpp

+}
+
+// send a string's length and data
+inline void sendString(int socket,


torch/lib/c10d/Utils.hpp

+       const std::chrono::milliseconds& timeout = kNoTimeout);
+
+// Helper resource guard class
+class ResourceGuard {


torch/lib/c10d/test/TCPStoreTest.cpp

+              std::string val = "thread_val_" +
+                  std::to_string(numIterations - 1);
+              c10d::test::check(*slaveStores[i], key, val);
+            }


pietern

Added a few instances of the style inconsistencies I was talking about in the previous review. There are functions that start with a newline and ones that don't. Also the mixed use of // and /**/ is odd on the eyes.

torch/lib/c10d/TcpStore.cpp

+}
+
+TCPStoreDaemon::~TCPStoreDaemon() {
+


torch/lib/c10d/TcpStore.cpp

+}
+
+void TCPStoreDaemon::run() {
+


torch/lib/c10d/TcpStore.cpp

+void TCPStoreDaemon::run() {
+
+  // Create the control pipe
+  controlPipeFd_ = std::vector<int>{-1, -1};


torch/lib/c10d/Utils.cpp

+  return listenPort;
+}
+
+}


torch/lib/c10d/Utils.hpp

+  return static_cast<RankType>(rank);
+}
+
+// TCP util namespace


torch/lib/c10d/test/TCPStoreTest.cpp

+  c10d::test::Semaphore sem1, sem2;
+
+  // Each thread will have a slave store to send/recv data
+  std::vector<std::unique_ptr<c10d::TCPStore>> slaveStores;


Reference: pytorch#7434

pietern

Micro nit remaining

torch/lib/c10d/TcpStore.cpp

+}
+
+bool TCPStoreDaemon::
+checkKeys(const std::vector<std::string>& keys) const {


torch/lib/c10d/TCPStore.cpp

+        // we hit an exception here.
+        ::close(fds[fdIdx].fd);
+        fds.erase(fds.begin() + fdIdx);
+        sockets_.erase(sockets_.begin() + fdIdx - 2);


Reference: pytorch#7434

…0D store interface (pytorch/pytorch#7560) pytorch/pytorch@0d27d26

* upstream/master: Makes AccumulateGrad high priority in backwards passes (pytorch#7604) [C++ API] Implement builder style construction (pytorch#7597) C10D: Added TCPStore to support C10D store interface (pytorch#7560) [auto] Update onnx to ba86ec2 - Protobuf typing (onnx/onnx#982) onnx/onnx@ba86ec2 Add LBFGS optimization algorithm to C++ API (pytorch#7596)

Reference: pytorch#7434 * C10D: Added TCPStore to support C10D store interface * Used pipe to terminate the store daemon and addressed all comments * Used notify/wake for wait and addressed all comments * Clean up nits * Clean up all socket states when the socket is closed

C10D: Added TCPStore to support C10D store interface

54cb0b5

teng-li requested review from pietern and apaszke May 15, 2018 01:51

teng-li requested review from colesbury, ezyang, gchanan, soumith and zdevito as code owners May 15, 2018 01:51

onnxbot-worker-2 mentioned this pull request May 15, 2018

[auto] pytorch-pr-7560 onnxbot/onnx-fb-universe#2068

Closed

teng-li added the oncall: distributed Add this issue/PR to distributed oncall triage queue label May 15, 2018

apaszke reviewed May 15, 2018

View reviewed changes

pietern reviewed May 15, 2018

View reviewed changes

teng-li force-pushed the TcpStore branch 6 times, most recently from bbd1059 to e11d975 Compare May 16, 2018 02:09

Used pipe to terminate the store daemon and addressed all comments

50e1362

Please see: pytorch#7434

teng-li force-pushed the TcpStore branch from e11d975 to 50e1362 Compare May 16, 2018 02:11

apaszke reviewed May 16, 2018

View reviewed changes

torch/lib/c10d/test/TCPStoreTest.cpp Outdated

std::string val = "thread_val_" +

std::to_string(numIterations - 1);

c10d::test::check(*slaveStores[i], key, val);

}

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

pietern reviewed May 16, 2018

View reviewed changes

teng-li force-pushed the TcpStore branch from 72645a9 to d27c8ec Compare May 16, 2018 22:20

Used notify/wake for wait and addressed all comments

b9294ee

Reference: pytorch#7434

teng-li force-pushed the TcpStore branch from d27c8ec to b9294ee Compare May 16, 2018 23:44

pietern reviewed May 16, 2018

View reviewed changes

torch/lib/c10d/TcpStore.cpp Outdated

}

bool TCPStoreDaemon::

checkKeys(const std::vector<std::string>& keys) const {

This comment was marked as off-topic.

Sign in to view

Clean up nits

346b6c5

apaszke approved these changes May 17, 2018

View reviewed changes

apaszke suggested changes May 17, 2018

View reviewed changes

teng-li force-pushed the TcpStore branch 2 times, most recently from e5f7c5c to 0171a57 Compare May 17, 2018 18:08

Clean up all socket states when the socket is closed

ba48f05

Reference: pytorch#7434

teng-li force-pushed the TcpStore branch from 0171a57 to ba48f05 Compare May 17, 2018 18:12

apaszke approved these changes May 17, 2018

View reviewed changes

teng-li merged commit 0d27d26 into pytorch:master May 17, 2018

teng-li deleted the TcpStore branch May 17, 2018 20:38

onnxbot added a commit to onnxbot/onnx-fb-universe that referenced this pull request May 17, 2018

[auto] Update pytorch to 0d27d26 - C10D: Added TCPStore to support C1…

39b9fb1

…0D store interface (pytorch/pytorch#7560) pytorch/pytorch@0d27d26


		SYSCHECK(::poll(fds.data(), fds.size(), -1));

		if (fds[0].revents != 0) {

		}

		TCPStoreDaemon::~TCPStoreDaemon() {

C10D: Added TCPStore to support C10D store interface #7560

C10D: Added TCPStore to support C10D store interface #7560

Conversation

teng-li commented May 15, 2018 • edited

apaszke left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

teng-li commented May 15, 2018 • edited

pietern left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

teng-li commented May 16, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

pietern left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

pietern left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

teng-li commented May 15, 2018 •

edited

teng-li commented May 15, 2018 •

edited