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

Fix grpc bugs #7435

Merged
merged 12 commits into from
Jan 15, 2018
Merged

Fix grpc bugs #7435

merged 12 commits into from
Jan 15, 2018

Conversation

gongweibao
Copy link
Contributor

@gongweibao gongweibao commented Jan 11, 2018 •
edited
Loading

typhoonzero
typhoonzero previously approved these changes Jan 11, 2018
View reviewed changes
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++

typhoonzero
typhoonzero reviewed Jan 15, 2018
View reviewed changes
paddle/operators/detail/grpc_client.cc Outdated

GPR_ASSERT(ok);
PADDLE_ENFORCE(tag);

// TODO(gongwb): add more retries.
ClientBase* c = static_cast<ClientBase*>(tag);
if (!c->status_.ok()) {
LOG(ERROR) << "proc param error:" << c->var_h_.String();
Copy link
Contributor

Choose a reason for hiding this comment

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

Only log one time for the error.

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.

paddle/operators/send_op.cc Outdated
client_.wait();
if (!client_.wait()) {
LOG(ERROR) << "send op exit";
exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use exit in operators, use PADDLE_ENFORCE

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.

python/paddle/v2/fluid/tests/book_distribute/test_dist_image_classification.py Outdated
@@ -0,0 +1,169 @@
# Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserved
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please split the fix and the book dist sample in two PRs?

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.

paddle/operators/detail/grpc_server.cc Outdated
@@ -184,15 +183,20 @@ void AsyncGRPCServer::HandleRequest(bool wait, grpc::ServerCompletionQueue* cq,
break;
}

assert(tag);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use PADDLE_ENFORCE

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.

paddle/operators/recv_op.cc Outdated
@@ -100,6 +100,8 @@ class RecvOp : public framework::OperatorBase {
// TODO(gognwb): simply this loop.
// Get from multiple trainers, we don't care about order in which
// the gradient arrives, just add suffix 0~n then average the gradient.
VLOG(4) << "param_count:" << param_count
Copy link
Contributor

Choose a reason for hiding this comment

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

Reduce VLOG appearances.

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.

paddle/operators/detail/grpc_server.cc Outdated
@@ -28,12 +28,15 @@ class RequestBase {
public:
explicit RequestBase(sendrecv::SendRecvService::AsyncService* service,
grpc::ServerCompletionQueue* cq)
: service_(service), cq_(cq), status_(PROCESS) {}
: service_(service), cq_(cq), status_(PROCESS) {
assert(cq_);
Copy link
Contributor

Choose a reason for hiding this comment

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

PADDLE_ENFORCE

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.

paddle/operators/detail/grpc_server.cc Outdated
TryToRegisterNewOne();
delete base;
Copy link
Contributor

Choose a reason for hiding this comment

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

No other places to release this memory then.

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'm not sure it's a grpc bug or it's our application bug.
When delete base often, I met an error

paddle/operators/detail/grpc_server.cc Outdated
VLOG(4) << cq_name << " recv no regular event";
LOG(WARNING) << cq_name << " recv no regular event:argument name"
<< base->GetReqName();
// FIXME(gongwb): delete the old one?
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment does not make things clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't get more context when ok != true.

paddle/operators/send_op.cc Outdated
@@ -48,7 +48,10 @@ class SendOp : public framework::OperatorBase {
client_.AsyncGetVariable(epmap[i], ctx, scope, outs[i]);
}

client_.wait();
if (!client_.wait()) {
LOG(ERROR) << "send op exit";
Copy link
Contributor

Choose a reason for hiding this comment

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

This log is too simple.

Copy link
Contributor Author

@gongweibao gongweibao Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Detail logs had been logged in functions it calls.

@gongweibao gongweibao changed the title Add distribution image book Fix grpc bugs Jan 15, 2018
typhoonzero
typhoonzero approved these changes Jan 15, 2018
View reviewed changes
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++

@gongweibao gongweibao merged commit 535fefb into PaddlePaddle:develop Jan 15, 2018
@gongweibao gongweibao deleted the imagebook2 branch January 16, 2018 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@typhoonzero typhoonzero typhoonzero approved these changes

@Yancey1989 Yancey1989 Awaiting requested review from Yancey1989

@helinwang helinwang Awaiting requested review from helinwang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gongweibao @typhoonzero