Skip to content

Conversation

@Yancey0623
Copy link
Contributor

Fixed #6959

@Yancey0623 Yancey0623 requested a review from typhoonzero January 3, 2018 07:53
using namespace paddle::framework::proto;
std::istringstream iss(msg.serialized());
switch (msg.type()) {
case sendrecv::VarType::LOD_TENSOR: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Each case can have no {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

msg->set_serialized(oss.str());
}

inline void DeserializeFromMessage(const VariableMessage &msg,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be inline, this function is too long.

std::unique_ptr<SendRecvService::Stub> stub_;
};

inline void SerializeToMessage(const std::string &name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be inline, this function is too long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think inline is a simple way, otherwise, we need a library for SerializeToMessage/DeserializeFromMessage.

platform::DeviceContextPool::Instance();
auto &dev_ctx = *pool.Get(dev_place);
detail::DeserializeFromMessage(v.second, dev_ctx, var);
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove commented lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Yancey0623 Yancey0623 requested a review from dzhwinter January 3, 2018 10:20
Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

Need unitest for dealing SelectedRows

@Yancey0623
Copy link
Contributor Author

Yancey0623 commented Jan 5, 2018

@typhoonzero
Done for the selectedrows unit test.

add_functor(ctx, *x0, *x1, expect.get());

EXPECT_EQ(actual->numel(), expect_value->numel());

Copy link
Contributor

Choose a reason for hiding this comment

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

Also should expect new return_rows().size() == rows1 + rows2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM++

@Yancey0623 Yancey0623 merged commit e5fe893 into PaddlePaddle:develop Jan 5, 2018
@Yancey0623 Yancey0623 deleted the serialize_functor branch January 5, 2018 05:41
@Yancey0623 Yancey0623 mentioned this pull request Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants