PServer recovery from checkpoint #2741

Yancey1989 · 2017-07-04T16:45:09Z

Related: Pserver save state #2716

helinwang · 2017-07-04T23:57:52Z

go/pserver/etcd_client.go

+// GetCheckpoints gets the checkpoint information by the specified pserver id
+func (e *EtcdClient) GetCheckpointInfo(string idx) (string, error) {
+	key := PsCheckpoint + idx
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*time.Duration(timeout))


Where is timeout defined, does this code compiles? :)

Sorry :), fixed it.

helinwang · 2017-07-05T00:00:15Z

go/pserver/etcd_client.go

@@ -186,3 +188,20 @@ func (e *EtcdClient) registerPserverEtcd(ctx context.Context) (int, error) {

 	return idx, nil
 }
+
+// GetCheckpoints gets the checkpoint information by the specified pserver id
+func (e *EtcdClient) GetCheckpointInfo(string idx) (string, error) {


Maybe rename this function to GetKey, caller can compute and pass in the key.

helinwang · 2017-07-05T00:01:37Z

go/pserver/service.go

+	UUID      string `json:"uuid"`
+	MD5       string `json:"md5"`
+	Timestamp string `json:"timestamp"`
+	State     []byte


Just curious why State and ParameterWithConfig does not have json tag?

Checkpoint will be initialized from the JSON data in etcd, which only the the key uuid, md5 and timestamp, but according with your other comment, separate Checkpoint and CheckpointInfo is better way, thanks for @helinwang .

helinwang · 2017-07-05T00:09:21Z

go/pserver/service.go

+	MD5       string `json:"md5"`
+	Timestamp string `json:"timestamp"`
+	State     []byte
+	ParameterWithConfig


Agree with your comment on the other PR, Checkpoint need to contain checkpoints for all parameters on the pserver.
I was confused before. I think Checkpoint should be renamed to CheckpointInfo, it's actually a reference to the checkpoint file on distributed filesystem. The type of Timestamp should be changed to int64.
Perhaps something like:

type CheckpointInfo struct { UUID string Path string MD5 string Timestamp int64 }

Maybe we can have another type like:

type ParameterCheckpoint struct { ParameterWithConfig State []byte } type Checkpoint []ParameterCheckpoint

How do you think?

Good idea! I think separate Checkpoint and CheckpointInfo is a good idea.I will do that.

Rename ParameterCheckpoint to CheckpointData should be more clear. Then these two struct should be:

type CheckpointMeta struct {} type CheckpointData struct {} type Checkpoint struct { meta CheckpointMeta data CheckpointData }

Cool, CheckpointMeta looks like a appropriate name.
Maybe we don't need the struct Checkpoint including meta and data? We can

Json.Umarshal from etcd to initialize CheckpointMeta.

gob.Decode(...) from the checkpoint file to initialize CheckpointData

I looks like more simple?

Done.

@typhoonzero How do you think about this way?

agreed with @Yancey1989 . Do we need to separate meta and data in checkpoint ?

Agree!

Do we need to separate meta and data in checkpoint ?

Maybe not, It's simple enouph to load meta and data into the struct separately.

typhoonzero · 2017-07-05T01:57:27Z

go/pserver/service.go

+	MD5       string `json:"md5"`
+	Timestamp string `json:"timestamp"`
+	State     []byte
+	ParameterWithConfig


Rename ParameterCheckpoint to CheckpointData should be more clear. Then these two struct should be:

type CheckpointMeta struct {} type CheckpointData struct {} type Checkpoint struct { meta CheckpointMeta data CheckpointData }

typhoonzero · 2017-07-05T02:00:07Z

go/pserver/service.go

+}
+
+// NewCheckpoint creates a new checkpoint.
+func NewCheckpoint(idx int, cpPath string, e *EtcdClient) (*Checkpoint, error) {


Rename to NewCheckpointFromFile

As the offline discuss, I will add Load and Save method for the struct Checkponit:)

typhoonzero · 2017-07-05T02:00:32Z

go/pserver/service.go

+	if err != nil {
+		return nil, err
+	}
+	// TODO: create checkpoint from file


Finish this TODO and then review.

I may not have internet connection in the following few days. Don't want to block this PR from being merged.

…_from_checkpoint

dzhwinter

Some part of the design is quite different.
Please review this PR when you in spare time.
https://github.com/PaddlePaddle/Paddle/pull/2716/files

dzhwinter · 2017-07-07T02:50:31Z

go/pserver/service.go

@@ -49,13 +54,95 @@ type Service struct {
 	optMap map[string]*optimizer
 }

+// CheckpointMeta saves the checkpoint information
+type CheckpointMeta struct {


checkpointMeta is an inner type which only used by pserver, it should not be a exported type.

dzhwinter · 2017-07-07T09:04:18Z

go/cmd/pserver/pserver.go

+		s, err = pserver.NewService(idx)
+		candy.Must(err)
+	} else {
+		s, err = pserver.NewServiceFromCheckpoint(idx, cp)


unify these two interface may be better, we can pass an empty checkpoint in default. then we will have only one create entry and leave out the duplicate code.

s := &Service{ idx: idx, } s.optMap = make(map[string]*optimizer) s.initialized = make(chan struct{})

dzhwinter · 2017-07-07T09:08:18Z

go/pserver/service.go

+}
+
+// ParameterCheckpoint saves parameter checkpoint
+type ParameterCheckpoint struct {


same as Meta type.

dzhwinter · 2017-07-07T10:08:12Z

go/pserver/service.go

+
+// ParameterCheckpoint saves parameter checkpoint
+type ParameterCheckpoint struct {
+	ParameterWithConfig


there in only type here without variable name.

According with: https://golang.org/ref/spec#Struct_types, a field without a explicit field name is an anonymous field, so I can initialize the Param filed of ParameterWithConfig as ParameterCheckpoint.Param = xxx

get it. a brilliant feature of Go.

dzhwinter · 2017-07-07T11:52:42Z

go/pserver/service.go

+
+	dec := gob.NewDecoder(f)
+
+	if err = dec.Decode(&cp.data); err != nil {


need to check md5 value here.

dzhwinter · 2017-07-07T12:25:41Z

go/pserver/service.go

+}
+
+// Checkpoint saves all parameters' checkpoint
+type Checkpoint struct {


maybe these things should not be the member of Checkpoint. They do not persist in permanent storage. I move them into Server's argument.

dzhwinter

LGTM

dzhwinter · 2017-07-11T07:37:04Z

go/pserver/service.go

 	Timestamp string `json:"timestamp"`
 }

 // Checkpoint is the pserver shard persist in file
-type Checkpoint []ParameterCheckpoint
+type Checkpoint []parameterCheckpoint


I am following @typhoonzero 's comment here for possible usage.
#2716 (comment)
But it is true, single parameterCheckpoint is useless to the user. It should be use immediately out of pserver's package.

We can fix this if something error for the later.

dzhwinter · 2017-07-11T07:49:54Z

go/pserver/service.go

+	}
+	md5 := hex.EncodeToString(h.Sum(nil))
+	if md5 != cpMeta.MD5 {
+		return nil, errors.New("md5 does match, load checkpoint failed")


do we need to put the const error type together? see the line of L28.

helinwang · 2017-07-11T17:54:29Z

go/pserver/etcd_client.go

@@ -189,6 +189,22 @@ func (e *EtcdClient) registerPserverEtcd(ctx context.Context) (int, error) {
 	return idx, nil
 }

+// GetKey gets the value by the specified key
+func (e *EtcdClient) GetKey(key string, timeout int) ([]byte, error) {


It is more clear if the type of timeout is time.Duration.

helinwang · 2017-07-11T17:59:59Z

go/pserver/service.go

@@ -26,6 +27,8 @@ const (
 	AlreadyInitialized = "pserver already initialized"
 	// Uninitialized is true if pserver not fully initialized
 	Uninitialized = "pserver not fully initialized"
+	// CheckpointUnmatched is true if the md5 of checkpoint file does matched the older
+	CheckpointUnmatched = "checkpoint file does not matched the older"


Maybe CheckpointMD5Failed = "checkpoint file MD5 validation failed" would better express the error.

helinwang · 2017-07-11T18:04:39Z

go/pserver/service.go

@@ -26,6 +27,8 @@ const (
 	AlreadyInitialized = "pserver already initialized"
 	// Uninitialized is true if pserver not fully initialized
 	Uninitialized = "pserver not fully initialized"
+	// CheckpointUnmatched is true if the md5 of checkpoint file does matched the older


I think "is true" is not very appropriate in English grammar, it's string type, "is true" is for bool type. Since the content of these constants already express the meaning, maybe we don't need the comment?
Perhaps we can put a comment on the top of const keyword:

// RPC error messages. const ( AlreadyInitialized = "pserver already initialized" Uninitialized = "pserver not fully initialized" CheckpointUnmatched = "checkpoint file does not matched the older" )

helinwang · 2017-07-11T18:08:12Z

go/pserver/service.go

 type checkpointMeta struct {
 	UUID      string `json:"uuid"`
-	Md5sum    string `json:"md5sum"`
+	MD5       string `json:"md5"`
 	Timestamp string `json:"timestamp"`


Sorry this is not related to this PR.
Timestamp should have type int64, it's value should be from func (t Time) UnixNano() int64.
Having string here makes it not precise enough (精度不够). time.Now().String() is only accurate to second: 2009-11-10 23:00:00 +0000 UTC.
Can you add a TODO: change type of timestamp to int64 here? (or would be great if you could fix it 👍 ).

helinwang · 2017-07-11T18:10:39Z

go/pserver/service.go

+		return nil, errors.New(CheckpointUnmatched)
+	}
+
+	dec := gob.NewDecoder(f)


Since io.Copy already read through f once, gob.NewDecoder(f) requires to read from the beginning of the file, but f have not been seek to the beginning of the file. Does this actually work?

Sorry, it's a bug...
Done.

helinwang · 2017-07-11T18:11:53Z

go/pserver/service.go

-// endpoints specified.
-func NewService(idx int, seconds int, path string, client *EtcdClient, cp Checkpoint) (*Service, error) {
+// endpoints specified. It will recovery from checkpoint file if a exists a specified checkpoint.
+func NewService(idx int, interval int, path string, client *EtcdClient, cp *Checkpoint) (*Service, error) {


Sorry it's not related to this PR. I know etcd uses int to express the type of timeout, we should really use type time.Duration for interval.

helinwang · 2017-07-11T18:16:52Z

go/pserver/service.go

@@ -186,13 +231,13 @@ func (s *Service) doCheckpoint() error {
 	s.mu.Lock()
 	defer s.mu.Unlock()

-	cp := make([]ParameterCheckpoint, 0, len(s.optMap))
+	cp := make([]parameterCheckpoint, 0, len(s.optMap))


Sorry this is not related to this PR.
This does not work. It creates a slice with length set to 0 and capacity set to len(s.optMap). access cp[0] will crash the program since it's length is 0. Should use cp := make([]parameterCheckpoint, len(s.optMap)) here.

For example:

package main func main() { a := make([]int, 0, 10) for i := 0; i < 10; i++ { a[i] = i } }

Output:

panic: runtime error: index out of range goroutine 1 [running]: main.main() /tmp/sandbox064034874/main.go:6 +0x20

Thanks @helinwang , done.

helinwang · 2017-07-12T06:40:43Z

go/cmd/pserver/pserver.go

 		if err != nil {
-			panic(err)
+			log.Infof("Fetch checkpoint failed, %s\n", err)


Maybe log.Errorf?

helinwang · 2017-07-12T06:42:16Z

go/cmd/pserver/pserver.go

-	if err != nil {
-		panic(err)
-	}
+	s, err := pserver.NewService(idx, time.Duration(*checkpointInterval)*time.Second, *checkpointPath, e, cp)


Can change flag.Int("checkpoint-interval", 600, "save checkpoint per interval seconds") to flag.Duration("checkpoint-interval", 600*time.Second, "save checkpoint interval"), and not do type cast here.

helinwang · 2017-07-12T06:44:35Z

go/pserver/service.go


 	cpMetajson, _ := json.Marshal(cpMeta)
-	err = s.client.PutKey(filepath.Join(PsCheckpoint, strconv.Itoa(s.idx)), cpMetajson, 3)
+	err = s.client.PutKey(filepath.Join(PsCheckpoint, strconv.Itoa(s.idx)), cpMetajson, time.Duration(3)*time.Second)


Maybe change to 3*time.Second. Constant in Go is well designed: https://blog.golang.org/constants

helinwang · 2017-07-12T06:46:03Z

go/pserver/service.go

 		}
 	}
 	return s, nil
 }

 // InitParam initializes a parameter.
-func (s *Service) InitParam(paramWithConfigs ParameterWithConfig, dummy *int) error {
+func (s *Service) InitParam(paramWithConfigs *ParameterWithConfig, dummy *int) error {


Is passing pointer * ParameterWithConfig necessary? Please see: https://github.com/golang/go/wiki/CodeReviewComments#pass-values

Cool, and here is also some useful documents about pass by value of pointer,

http://goinbigdata.com/golang-pass-by-pointer-vs-pass-by-value/

https://docs.google.com/document/d/1CxgUBPlx9iJzkz9JWkb6tIpTe5q32QDmz8l0BouG0Cw/preview

helinwang · 2017-07-12T06:46:27Z

go/pserver/optimizer.go

@@ -35,7 +35,7 @@ func cArrayToSlice(p unsafe.Pointer, len int) []byte {
 	return (*[1 << 30]byte)(p)[:len:len]
 }

-func newOptimizer(paramWithConfigs ParameterWithConfig, State []byte) *optimizer {
+func newOptimizer(paramWithConfigs *ParameterWithConfig, State []byte) *optimizer {


Is passing pointer * ParameterWithConfig necessary? Please see: https://github.com/golang/go/wiki/CodeReviewComments#pass-values

helinwang

LGTM++ Thanks!!!

pserver recovery from checkpoint

5a223a4

Yancey1989 assigned typhoonzero Jul 4, 2017

Yancey1989 requested review from helinwang, jacquesqiao and dzhwinter July 4, 2017 16:45

helinwang previously requested changes Jul 5, 2017

View reviewed changes

typhoonzero reviewed Jul 5, 2017

View reviewed changes

Yancey1989 added 5 commits July 6, 2017 02:13

update

c7d1547

merge from develop

ece97bf

update

64d9df9

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into pserver…

f3f42a2

…_from_checkpoint

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into pserver…

4cd9a42

…_from_checkpoint

Yancey1989 changed the title ~~[WIP] PServer recovery from checkpoint~~ PServer recovery from checkpoint Jul 7, 2017

Merge branch 'develop' of github.com:PaddlePaddle/Paddle into pserver…

bc5acdd

…_from_checkpoint

dzhwinter reviewed Jul 7, 2017

View reviewed changes

Yancey1989 added 4 commits July 11, 2017 10:18

merge from develop

3805634

update

24b4531

update

4cbafae

update

8f00457

dzhwinter approved these changes Jul 11, 2017

View reviewed changes

fix pre-commit

ed3097d

helinwang requested changes Jul 11, 2017

View reviewed changes

Yancey1989 added 2 commits July 12, 2017 12:12

merge and fix some bug

e26dcbf

update comment

01c22ef

helinwang reviewed Jul 12, 2017

View reviewed changes

update

c20c24a

helinwang approved these changes Jul 12, 2017

View reviewed changes

Yancey1989 merged commit 19bfb8a into PaddlePaddle:develop Jul 13, 2017

Yancey1989 deleted the pserver_from_checkpoint branch July 13, 2017 01:52


		dec := gob.NewDecoder(f)

		if err = dec.Decode(&cp.data); err != nil {

PServer recovery from checkpoint #2741

PServer recovery from checkpoint #2741

Conversation

Yancey1989 commented Jul 4, 2017 • edited by typhoonzero 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

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

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

dzhwinter left a 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

Choose a reason for hiding this comment

Yancey1989 Jul 11, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhwinter left a 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

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

helinwang Jul 11, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang Jul 11, 2017 • 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

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

Yancey1989 Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang left a comment

Choose a reason for hiding this comment

Yancey1989 commented Jul 4, 2017 •

edited by typhoonzero

Loading

Yancey1989 Jul 11, 2017 •

edited

Loading

helinwang Jul 11, 2017 •

edited

Loading

helinwang Jul 11, 2017 •

edited

Loading

Yancey1989 Jul 12, 2017 •

edited

Loading