Pserver Save state #2716

dzhwinter · 2017-07-03T15:40:19Z

helinwang · 2017-07-03T16:38:04Z

go/pserver/service_test.go

@@ -79,6 +79,8 @@ func TestServiceFull(t *testing.T) {
 	if !reflect.DeepEqual(param1, p) {
 		t.FailNow()
 	}
+	var dummy int
+	s.Save("", &dummy)


Pass in nil is fine: s.Save("", nil). I used s.Save("", &dummy) before but later realized that it's fine to pass in nil :)

helinwang · 2017-07-03T16:41:15Z

go/pserver/service_test.go

@@ -166,3 +168,7 @@ func TestBlockUntilInitialized(t *testing.T) {

 	wg.Wait()
 }
+
+func TestCheckpointSpeed(t *testing.T) {


Speed can be tested with benchmark. Here is an example: https://dave.cheney.net/2013/06/30/how-to-write-benchmarks-in-go

leave a TODO here, will be tested after reaching an agreement with @Yancey1989 's recover logic.

helinwang · 2017-07-03T16:46:18Z

go/pserver/service.go

@@ -38,6 +52,7 @@ type Parameter struct {
 type ParameterWithConfig struct {
 	Param  Parameter
 	Config []byte // parameter configuration in Proto Buffer format
+	State  []byte // parameter training state


ParameterWithConfig is the data sent from the trainer to the pserver. But State is saved by pserver, loaded by pserver, which is not related to trainer.
So State should not be part of this struct.

Maybe:

type checkpoint struct { Uuid string Md5sum string Timestamp string ParameterWithConfig // this is called embedded field State []byte }

embedded field: https://golang.org/ref/spec#Struct_types

split into info and data part. fix done.

helinwang · 2017-07-03T16:49:55Z

go/pserver/service.go

@@ -142,8 +177,51 @@ func (s *Service) GetParam(name string, parameter *Parameter) error {

 // Save tells the parameter server to save parameters.
 func (s *Service) Save(path string, dummy *int) error {


Save was intended for saving model. But now we no longer use pservers to save model. Can you rename save to checkpoint? Also, I think at least for the first implementation, checkpoint should not be exposed as a RPC method to the trainer, instead, pservers periodically checkpoints, so can you make this a private function: func (s *Service) checkpoint(path string) error? (note that we don't need parameter dummy *int anymore if it's not used for RPC).

helinwang · 2017-07-03T16:59:47Z

go/pserver/service.go

+		log.Infof("parameter checkpoint %s", ckbytes)
+
+		if _, err = os.Stat(ck.Uuid); os.IsNotExist(err) {
+			log.Info("checkpoint not exists.")


checkpoint not exists. -> checkpoint does not exist.

helinwang · 2017-07-03T17:00:19Z

go/pserver/service.go

+			log.Info("checkpoint not exists.")
+		} else {
+			err = os.Remove(ck.Uuid)
+			log.Infof("remove %s", ck.Uuid)


remove %s -> checkpoint %s already exists, removing.

helinwang · 2017-07-03T17:01:43Z

go/pserver/service.go

+			log.Infof("remove %s", ck.Uuid)
+		}
+		f, err := os.Create(ck.Uuid)
+		defer f.Close()


defer f.Close() will close when this function returns, not when the for loop goes to the next loop. And the for loop may be very long. So perhaps call f.Close() at the end of for loop.

helinwang · 2017-07-03T17:03:20Z

go/pserver/service.go

@@ -14,6 +24,10 @@ const (
 	Uninitialized      = "pserver not fully initialized"
 )

+const (
+	checkpoint_path = "./checkpoints/"


Go's naming convention is camelCase, not snake_case.

checkpointPath need to be an argument (flag.String) passed to go/cmd/pserver program. Since the k8s will configure the path.

helinwang · 2017-07-03T17:05:19Z

go/pserver/service.go

+			log.Errorln(err)
+		}
+		// TODO: according design doc, need to save Uuid to etcd in json format
+		// {\"Uuid\": [UUID], \"md5\", \"MD5 sum\", \"Timestamp\": xxxx}


I think the design doc mentioned using etcd to save checkpoint information as well. Maybe add a TODO?

Add etcd saving logic. fix done.

helinwang · 2017-07-03T17:11:58Z

go/pserver/service.go

+}
+
+//serialize ParameterWithConfig to byte stream
+func GetBytes(content ...interface{}) ([]byte, error) {


Making content ...interface{} adds more complexity to the code: since it's interface type that we need to encode, we have to call gob.Register. It's harder to understand the code (people need to search for what does gob.Register do. And it's harder to maintain the code (whenever adds a new type for GetBytes to use, maintainer need to add gob.Register as well, it's hard to track.

Since here we only need to call GetBytes twice, and this function does not have much code. Maybe just put it inline? (and remove gob.Register)

Yancey1989 · 2017-07-04T05:56:58Z

go/pserver/service.go

@@ -52,14 +67,34 @@ type Service struct {
 	optMap map[string]*optimizer
 }

+type checkpoint struct {
+	Uuid      string


Adding json: "uuid" at the end of the line, so we can use Json.marshal to a format JSON.

agree with another PR. fix Done.

Yancey1989 · 2017-07-04T16:54:18Z

go/pserver/service.go

+			err = os.Remove(ck.Uuid)
+			log.Infof("remove %s", ck.Uuid)
+		}
+		f, err := os.Create(ck.Uuid)


It will create so many files for each paramter. Following the design doc, we will only have one checkpoint file named UUID?

typhoonzero

LGTM++ except small comments.

typhoonzero · 2017-07-09T01:36:26Z

go/cmd/pserver/pserver.go

@@ -20,6 +20,8 @@ func main() {
 		"comma separated endpoint string for pserver to connect to etcd")
 	etcdTimeout := flag.Int("etcd-timeout", 5, "timeout for etcd calls")
 	numPservers := flag.Int("num-pservers", 1, "total pserver count in a training job")
+	checkpointPath := flag.String("checkpoint-path", "/checkpoints/", "save checkpoint path")
+	checkpointInterval := flag.Int("checkpoint-interval", 10, "save checkpoint per interval seconds")


Default 10 seconds maybe too quick?

yeah, fix interval is not proper for every training job. Time consumed always determined by training data amount. Round count may be better here.
Change it to 10 min(600seconds)

typhoonzero · 2017-07-09T02:05:48Z

go/pserver/service.go

+}
+
+// Checkpoint is the pserver shard persist in file
+type Checkpoint []parameterCheckPoint


Exported type is an array of unexported type, maybe inconvenience to use.

dzhwinter added 4 commits July 3, 2017 21:13

"init saving model"

5ef1425

"saving checkpoint"

f1330e2

Merge remote-tracking branch 'origin/develop' into save_state

e6c98e4

"fix gob register error"

65afbe1

helinwang reviewed Jul 3, 2017

View reviewed changes

Yancey1989 reviewed Jul 4, 2017

View reviewed changes

Yancey1989 mentioned this pull request Jul 5, 2017

PServer recovery from checkpoint #2741

Merged

dzhwinter added 6 commits July 7, 2017 10:36

Merge remote-tracking branch 'origin/develop' into save_state

bfc3b43

"fix pserver saving etcd"

40295b9

Merge remote-tracking branch 'origin/develop' into save_state

8426beb

"add more NewService argument"

774604c

Merge remote-tracking branch 'origin/develop' into save_state

2f2ffd9

"pserver flags type error"

87e7924

typhoonzero approved these changes Jul 9, 2017

View reviewed changes

dzhwinter and others added 2 commits July 9, 2017 15:12

"make parameterCheckpoint exported"

0ad7053

restart teamcity JOB

e8296ff

dzhwinter merged commit 15f021a into PaddlePaddle:develop Jul 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pserver Save state #2716

Pserver Save state #2716

dzhwinter commented Jul 3, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

helinwang Jul 3, 2017

dzhwinter Jul 7, 2017

Yancey1989 Jul 4, 2017

dzhwinter Jul 7, 2017

Yancey1989 Jul 4, 2017

dzhwinter Jul 7, 2017

typhoonzero left a comment

typhoonzero Jul 9, 2017

dzhwinter Jul 9, 2017

typhoonzero Jul 9, 2017

dzhwinter Jul 9, 2017

		@@ -142,8 +177,51 @@ func (s Service) GetParam(name string, parameter Parameter) error {

		// Save tells the parameter server to save parameters.
		func (s Service) Save(path string, dummy int) error {

Pserver Save state #2716

Pserver Save state #2716

Conversation

dzhwinter commented Jul 3, 2017

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

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

typhoonzero 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