Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

avoid cast_storage in dist-kvstore-server #174

Merged
merged 3 commits into from
Aug 18, 2017
Merged

avoid cast_storage in dist-kvstore-server #174

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
694b6ab
avoid cast_storage in dist-kvstore-server
eric-haibin-lin Aug 17, 2017
87a0eb3
add stream arg to mshadow;;copy
eric-haibin-lin Aug 18, 2017
74aa70f
fix copy order
eric-haibin-lin Aug 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/kvstore/kvstore_dist_server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "ps/ps.h"
#include "mxnet/kvstore.h"
#include "../operator/tensor/elemwise_binary_op.h"
#include "../operator/tensor/init_op.h"

namespace mxnet {
namespace kvstore {
Expand Down Expand Up @@ -228,11 +229,16 @@ class KVStoreDistServer {
CHECK_EQ(req_data.vals.size(), num_rows * unit_len);
TBlob recv_blob(data, dshape, cpu::kDevMask); // NOLINT(*)
NDArray recved = NDArray(recv_blob, 0);
// TODO(haibin) temporarily initialized as dense NDArray. We need inplace operator
// support for rowsparse ndarrays. And after that `stored` should be initialized as
// RowSparse NDArray
stored = NDArray(kRowSparseStorage, dshape, Context());
CopyFromTo(recved, &stored, 0);
Engine::Get()->PushSync([recved, stored](RunContext ctx) {
NDArray rsp = stored;
stored.CheckAndAlloc({mshadow::Shape1(recved.shape()[0])});
mshadow::Stream<cpu> *s = ctx.get_stream<cpu>();
op::PopulateFullIdxRspImpl(s, &rsp);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the content of rsp.data()? Seems it's allocated but never assigned values to its data blob.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's empty. We want to copy recved.data() to rsp.data()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, should it be Copy(rsp, recved)?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oop terrible mistake.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now. Passing unit test. I must have forgotten to run the ones for dist-kvstore.

mshadow::Copy(rsp.data().FlatTo1D<cpu, float>(),
recved.data().FlatTo1D<cpu, float>(), s);
}, recved.ctx(), {recved.var()}, {stored.var()},
FnProperty::kNormal, 0, PROFILER_MESSAGE_FUNCNAME);
stored.WaitToRead();
server->Response(req_meta);
return;
Expand Down