-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
"fix client send empty gradients bug" #2654
Conversation
go/pserver/client.go
Outdated
@@ -123,6 +123,10 @@ func (c *Client) FinishInitParams() error { | |||
// SendGrads sends gradients to parameter servers for updating | |||
// parameters. | |||
func (c *Client) SendGrads(grads []Gradient) error { | |||
if len(grads) == 0 { | |||
log.Info("Send Empty Gradient") | |||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe return an error. If we treat it as error, user can be aware of this error and fix it.
Btw, let's not use "Title Case" (基本每个单词都大写) for log message. For log message we can use a normal sentence like: "Sent empty gradient." (or people uses "sent empty gradient" (首单词不大写,不用句号结尾))。
For error message, people typically use "sent empty gradient" (首单词不大写,不用句号结尾), because error messages are often combined somewhere else. E.g.,
func foo() error {
return errors.New("sent empty gradient")
}
func main() {
err := foo()
if err != nil {
log.Errorf("foo returned error: %v", err)
}
}
// Output:
// foo returned error: sent empty gradient
If we use a normal sentence like: "Sent empty gradient.", the output would be: "foo returned error: Sent empty gradient.", Which could be strange, because in English grammar, word typically does not capitalize after ":".
Also, "send empty gradient" may not express this error fully, "no gradient received" could be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for such a detailed comment! 👍👍👍
fix Done.
Thank for noticing this! Good catch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM after fixing the only comment.
In some miss use situation, SendGrads will get empty slice here, then
errCh
will block forever.