[RPC] add C++ RPC infrastructure and distributed sampler #408

aksnzhy · 2019-02-22T09:21:47Z

Description

This PR add the C++ RPC infrastructure and distributed sampler API.

Now we need to add Windows wrapper for the c socket API for tcp_socket.cc. Anyone can help me do this job? I don't have the windows development environment.

Also, this code has been tested on multiple machines outside the unittest. If we need to add unittest test for the network code?

Checklist

Please feel free to remove inapplicable items for your PR.

The PR title starts with [$CATEGORY] (such as [Model], [Doc], [Feature]])
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage
Code is well-documented
To the my best knowledge, examples are either not affected by this change,
or have been fixed to be compatible with this change
Related issue is referred in this PR

Changes

BarclayII · 2019-02-22T20:55:50Z

I assume that I only need to port TCPSocket to Windows? The implementation would be a little bit different between WinSock and UNIX sockets.

Re unit test: I think it's better to have some sort of tests, otherwise I wouldn't know whether Windows can work. IMO they don't have to be integrated into CI (like unit tests), but I need some scripts to set up a simulated, or connect to an existing, distributed environment.

BarclayII · 2019-02-22T21:42:01Z

Also, what about PR #325 ?

aksnzhy · 2019-02-23T00:30:27Z

Also, what about PR #325 ?

I closed this PR. It is out of date.

aksnzhy · 2019-02-23T00:32:27Z

I assume that I only need to port TCPSocket to Windows? The implementation would be a little bit different between WinSock and UNIX sockets.

Re unit test: I think it's better to have some sort of tests, otherwise I wouldn't know whether Windows can work. IMO they don't have to be integrated into CI (like unit tests), but I need some scripts to set up a simulated, or connect to an existing, distributed environment.

Yes, only the TCPSocket class need to be changed. The other components are all platform-agnostic.

I will add some unittests for distributed sampler.

python/dgl/graph_index.py

src/graph/graph_apis.cc

src/graph/network/serialize.cc

zheng-da · 2019-02-26T00:26:01Z

src/graph/graph_apis.cc

+DGL_REGISTER_GLOBAL("graph_index._CAPI_ReceiverRecvSubgraph")
+.set_body([] (DGLArgs args, DGLRetValue* rv) {
+    // The data buffer will be allocated just once 
+    static char* data_buffer = new char[kMaxBufferSize];


The memory is allocated when the process starts regardless of how people use DGL. Maybe allocate memory more explicitly. e.g., only when people starts to use the distributed module.

Changed it by using a global pointer, which will only be allocated by CreateReceiver API

zheng-da · 2019-02-26T00:34:10Z

src/graph/network/socket_communicator.cc

+bool SocketCommunicator::InitSender(const char* ip, int port) {
+  // Sender only has a client socket
+  socket_.resize(1);
+  socket_[0] = new TCPSocket();


why not use shared_ptr?

I think it is better to control the pointer by handle in Finalize() method.

zheng-da · 2019-02-26T00:37:19Z

src/graph/network/msg_queue.cc

+  }
+}
+
+int MessageQueue::Add(const char* src, int size, bool is_blocking) {


this is a large memory copy.

It's not very easy to implement zero-copy here

how much does the memory copy affect the performance?

Let me evaluate it by removing the message queue for a test.

src/graph/network/msg_queue.cc

jermainewang · 2019-02-28T02:26:46Z

First of all, @aksnzhy @zheng-da do we agree that this will not merged into master at the moment because this feature is not part of the release plan?

BarclayII · 2019-03-06T05:52:07Z

I have updated tcp_socket.cc with Windows socket implementation.

aksnzhy · 2019-03-06T08:46:28Z

It seems that the NodeFlow and Sampler API has been changed. I need to update the distributed sampler code.

aksnzhy · 2019-03-07T07:14:42Z

@jermainewang Could we change the branch from dgl:gaint to dgl:master? The windows support has been completed and I have updated my code with the new NodeFlow code in master branch.

I have also pushed another PR to Da's local dgl branch.

* refactor. * accelerate update_all in nodeflow. * fix. * refactor. * fix lint. * fix lint. * reorganize. * reorg. * remove. * add doc. * impl block_incidence_matrix * fix lint. * fix. * simple fix. * fix test. * fix interface. * fix eid. * fix comments.

zheng-da · 2019-03-08T06:48:38Z

src/graph/network.cc

@@ -105,18 +82,18 @@ DGL_REGISTER_GLOBAL("network._CAPI_ReceiverRecvSubgraph")
    if (size <= 0) {
      LOG(ERROR) << "Receive error: (size: " << size << ")";
    }
-    NodeFlow nf;
+    NodeFlow* nf = new NodeFlow();


how is the memory free'd?

@zheng-da
I do the same allocation like this:

dgl/src/graph/sampler.cc

Line 836 in eb1acec

nflows[i] = new NodeFlow();

How does the sampler free the memory?

They are manually freed from Python code after instantiation of NodeFlow objects (python/dgl/nodeflow.py:59)

* update version * update news

* fix cython bug * fix * fix

* update README performance number * news

…mutable graph. (#437) * fix rgcn tutorial * small fix * upd * findedge/s * upd * upd * upd * upd * add test * remove redundancy * upd * upd * upd * upd * add edge_subgraph * explicit cast * add test immutable subg * reformat * reformat * fix bug * upd * hotfix * subgraph docs * fix adj mat * fix edges() order * add test * revert * upd * fix * upd

…449) * fix send with builtin bug * test cases * remove todo comment

* refactored sampler code * docstring * fix tutorial

* fix performance bug in EdgeSoftmax * minor

* fix bug in pickling * fix lint

…#457) * forgot prefetch switch * yet another stupid bug

BarclayII · 2019-03-26T05:59:22Z

src/graph/network/serialize.cc

+                                 const IdArray& node_mapping,
+                                 const IdArray& edge_mapping,
+                                 const IdArray& layer_offsets,
+                                 const IdArray& flow_offsets) {


This interface makes changing the nodeflow structure subtle; one needs to also change the interface here (and the deserialization, and all the calling statements).

Is it possible to change this to something like size_t SerializeNodeFlow(char *buffer, const NodeFlow &nf)?

EDIT: or maybe make these methods of NodeFlow instead (e.g. size_t NodeFlow::Serialize(char *buffer) and void NodeFlow::Deserialize(const char *buffer, size_t len))

No problem. I'll do this change after new NodeFlow structure merged.

If "new NodeFlow structure" refers to the one with optional tensor data (which is the one I'm using) then it won't be merged until after PR #453. It's your call but I would prefer changing the interface now so later on NodeFlow could be extended further more easily.

add C++ rpc infrastructure and distributed sampler

7b96a2e

aksnzhy requested a review from zheng-da February 22, 2019 09:21

jermainewang changed the title ~~add C++ RPC infrastructure and distributed sampler~~ [RPC] add C++ RPC infrastructure and distributed sampler Feb 22, 2019

jermainewang self-requested a review February 22, 2019 19:51

BarclayII self-assigned this Feb 22, 2019

aksnzhy added 6 commits February 25, 2019 17:25

update

a289b72

update lint

a76b349

update lint

63bd6ff

update lint

d1f66ae

update

e007826

update

a088ab6

zheng-da reviewed Feb 26, 2019

View reviewed changes

src/graph/network/msg_queue.cc Show resolved Hide resolved

aksnzhy added 12 commits February 26, 2019 17:43

update

255f51d

updare

0715da3

update

faf6a5b

update

49e0b2c

update

3e53c43

update serialize and unittest

49380e9

update serialize

a9c5184

lint

53df492

update

a7d2878

update

4c3f500

update

7181033

update

49b16c1

update API

22ee39c

zheng-da reviewed Mar 8, 2019

View reviewed changes

aksnzhy and others added 20 commits March 8, 2019 16:48

fix bug in serialize

82863ff

[Release] update version (#440)

cd4f0a2

* update version * update news

[Hotfix] fix cython bug (#442)

2ade0ef

* fix cython bug * fix * fix

[Release] Update readme performance number (#441)

32e0f74

* update README performance number * news

[Build] fix setup script to allow cython ext (#444)

d3cd601

[Doc] added index [0] to torch.max (#443)

1327262

fix bug in serialzie

7a1d43f

set parent graph

8074ece

update

6c1f9bb

update

18ec651

switching to msbuild for conda windows (#448)

8f42625

[BugFix] Handle case when calling send with builtin message function (#…

eb1acec

…449) * fix send with builtin bug * test cases * remove todo comment

update

f263149

update

42ce501

[Refactor] Sampler code refactor (#454)

681e521

* refactored sampler code * docstring * fix tutorial

[BugFix] Fix performance bug in EdgeSoftmax (#452)

6f603bb

* fix performance bug in EdgeSoftmax * minor

[Bugfix] fix bug in pickling (#456)

33b9c38

* fix bug in pickling * fix lint

[Bug] Fix unused prefetch switch for NeighborSampler and LayerSampler (…

341dbea

…#457) * forgot prefetch switch * yet another stupid bug

[News] Add GTC 2019 Tutorial as news in readme (#462)

5033d94

BarclayII reviewed Mar 26, 2019

View reviewed changes

aksnzhy added 2 commits March 27, 2019 09:48

update

4176276

update

7aad7b0

aksnzhy closed this Mar 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RPC] add C++ RPC infrastructure and distributed sampler #408

[RPC] add C++ RPC infrastructure and distributed sampler #408

aksnzhy commented Feb 22, 2019 •

edited

Loading

BarclayII commented Feb 22, 2019 •

edited

Loading

BarclayII commented Feb 22, 2019

aksnzhy commented Feb 23, 2019

aksnzhy commented Feb 23, 2019

zheng-da Feb 26, 2019

aksnzhy Feb 26, 2019

zheng-da Feb 26, 2019

aksnzhy Feb 26, 2019

zheng-da Feb 26, 2019

aksnzhy Feb 26, 2019

zheng-da Mar 3, 2019

aksnzhy Mar 4, 2019

jermainewang commented Feb 28, 2019 •

edited

Loading

BarclayII commented Mar 6, 2019

aksnzhy commented Mar 6, 2019

aksnzhy commented Mar 7, 2019 •

edited

Loading

zheng-da Mar 8, 2019

aksnzhy Mar 15, 2019

aksnzhy Mar 15, 2019

BarclayII Mar 18, 2019

BarclayII Mar 26, 2019 •

edited

Loading

aksnzhy Mar 26, 2019 •

edited

Loading

BarclayII Mar 26, 2019

[RPC] add C++ RPC infrastructure and distributed sampler #408

[RPC] add C++ RPC infrastructure and distributed sampler #408

Conversation

aksnzhy commented Feb 22, 2019 • edited Loading

Description

Checklist

Changes

BarclayII commented Feb 22, 2019 • edited Loading

BarclayII commented Feb 22, 2019

aksnzhy commented Feb 23, 2019

aksnzhy commented Feb 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jermainewang commented Feb 28, 2019 • edited Loading

BarclayII commented Mar 6, 2019

aksnzhy commented Mar 6, 2019

aksnzhy commented Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BarclayII Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

aksnzhy Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aksnzhy commented Feb 22, 2019 •

edited

Loading

BarclayII commented Feb 22, 2019 •

edited

Loading

jermainewang commented Feb 28, 2019 •

edited

Loading

aksnzhy commented Mar 7, 2019 •

edited

Loading

BarclayII Mar 26, 2019 •

edited

Loading

aksnzhy Mar 26, 2019 •

edited

Loading