Go optimizer: integrate Go with optimizer library #2610

dzhwinter · 2017-06-26T16:21:01Z

helinwang · 2017-06-27T01:10:42Z

go/pserver/optimizer.go

+// TODO(zhihong): move compile flags to cmake go_library
+#cgo pkg-config: protobuf
+#cgo CFLAGS: -I ../../
+#cgo LDFLAGS: /Users/dzh/.go/src/github.com/PaddlePaddle/Paddle/build/go/pserver/cclient/libpaddle_go_optimizer.a -lstdc++


/Users/dzh/.go/src/github.com need to be a relative path: #cgo LDFLAGS: -L${SRCDIR}/xxx

cgo cannot get cmake predefined variable here. use relative path here
fix done.

helinwang · 2017-06-27T01:14:42Z

go/pserver/optimizer.go

+	c := paramWithConfigs.Config
+	var cbuffer unsafe.Pointer
+	cbuffer = unsafe.Pointer(&p.Content[0])
+	o.opt = C.paddle_create_optimizer((*C.uchar)(&c[0]), C.int(len(c)),


&p.Content[0] is a pointer allocated in Go's stack, which is owned by Go. The optimizer will own the parameter buffer. So we need to either make a copy of &p.Content[0] here, or inside the implementation of paddle_element_type.

helinwang · 2017-06-27T01:16:16Z

go/pserver/optimizer.go

-	if len(p.Content) != len(g.Content) {
-		return fmt.Errorf("Name: %s, parameter and gradient length not match, parameter: %d, gradient: %d", p.Name, len(p.Content), len(g.Content))
+func (o *optimizer) GetWeights(p *Parameter) error {
+	// FIXME: get weigths from optimizer has bug


The optimizer is working before, we should not check in code that has bug and make optimizer not working. I think we need to debug the bug before merge in the code.

helinwang · 2017-06-27T01:18:39Z

go/pserver/optimizer.go

 	}

-	r := C.paddle_update_parameter(o.opt, unsafe.Pointer(&p.Content[0]), C.paddle_element_type(p.ElementType), unsafe.Pointer(&g.Content[0]), C.int(len(g.Content)))
+	// FIXME: do we need a copy? discard g.Content by GC ok


We do not need to make a copy of &g.Content[0], since C.paddle_update_parameter will not use &g.Content[0] anymore after return.

helinwang · 2017-06-27T01:20:08Z

go/pserver/service.go

-	opt      *optimizer
-	paramMap map[string]Parameter
+	mu sync.Mutex
+	// injection from parameter to optimizer


I don't quite understand the comment here, is this necessary?

helinwang · 2017-06-27T01:21:47Z

go/pserver/optimizer.go

-func (o *optimizer) UpdateParameter(p Parameter, g Gradient) error {
-	if len(p.Content) != len(g.Content) {
-		return fmt.Errorf("Name: %s, parameter and gradient length not match, parameter: %d, gradient: %d", p.Name, len(p.Content), len(g.Content))
+func (o *optimizer) GetWeights(p *Parameter) error {


Maybe func (o *optimizer) GetWeights() []byte is easier to read.

typhoonzero · 2017-06-27T04:11:24Z

go/pserver/optimizer_test.go

+		ElementType: Int32,
+	}
+	p.Content = []byte{1, 3}
+	config, err := ioutil.ReadFile("./cclient/test/testdata/optimizer.pb.txt")


This test file is not generated nor pushed to git.

It is pushed under the testdata folder. here it is
https://github.com/dzhwinter/Paddle/blob/242df4f07bda6de94399cc60b31cb22f27a94434/go/pserver/cclient/test/testdata/optimizer.pb.txt
Binary file can not shown in PR reviews. : )

helinwang · 2017-06-27T22:40:28Z

go/pserver/optimizer.go

 var nullPtr = unsafe.Pointer(uintptr(0))

 type optimizer struct {
 	opt *C.struct_paddle_optimizer
+	// used in GetParam, reconstruct Parameter from optimizer
+	ElementType ElementType


ElementType ElementType -> elementType ElementType. We should not expose anything more than necessary.

helinwang · 2017-06-27T22:41:56Z

go/pserver/optimizer.go

 var nullPtr = unsafe.Pointer(uintptr(0))

 type optimizer struct {
 	opt *C.struct_paddle_optimizer
+	// used in GetParam, reconstruct Parameter from optimizer


Perhaps the code ElementType ElementType is already self documenting, if user want to know where ElementType is used, he can just search through the code.

helinwang · 2017-06-27T22:43:50Z

go/pserver/optimizer.go

 	}

-	r := C.paddle_update_parameter(o.opt, unsafe.Pointer(&p.Content[0]), C.paddle_element_type(p.ElementType), unsafe.Pointer(&g.Content[0]), C.int(len(g.Content)))
+	fmt.Println(g)


Maybe this is not necessary.

helinwang · 2017-06-27T22:44:45Z

go/pserver/optimizer.go

+	cbuffer_len := int(unsafe.Sizeof(p.Content[0])) * len(p.Content)
+	cbuffer = C.malloc(C.size_t(cbuffer_len))
+	C.memcpy(cbuffer, unsafe.Pointer(&p.Content[0]), C.size_t(cbuffer_len))
+	o.opt = C.paddle_create_optimizer((*C.uchar)(&c[0]), C.int(len(c)),


Need a functionfunc (o *optimizer) Release() that releases the C++ optimizer.

we already have a funcition CleanUp

76func (o *optimizer) Cleanup() { 77 if unsafe.Pointer(o.opt) != nullPtr { 78 C.paddle_release_optimizer(o.opt) 79 o.opt = (*C.struct_paddle_optimizer)(nullPtr) 80 } 81}

Thanks! My bad!

helinwang · 2017-06-27T22:48:43Z

go/pserver/optimizer.go

+	p := paramWithConfigs.Param
+	c := paramWithConfigs.Config
+	var cbuffer unsafe.Pointer
+	cbuffer_len := int(unsafe.Sizeof(p.Content[0])) * len(p.Content)


Maybe it's a good idea to log here for Param.ElementType, len(Param.Content), len(Config.Content).

good idea. Done.

helinwang · 2017-06-27T22:49:15Z

go/pserver/optimizer.go

+	C.memcpy(cbuffer, unsafe.Pointer(&p.Content[0]), C.size_t(cbuffer_len))
+	o.opt = C.paddle_create_optimizer((*C.uchar)(&c[0]), C.int(len(c)),
+		C.paddle_element_type(p.ElementType), cbuffer, C.int(len(p.Content)),
+		(*C.char)(nullPtr), 0)


Maybe add a TODO here?

I thought CleanUp is enough

Sorry my bad, I thought (*C.char)(nullPtr), 0 is for parameter config, but it's actually for recover state, which is not necessary here.

helinwang

LGTM++ Maybe need to rebase, because there are merge conflict.

typhoonzero · 2017-06-30T15:06:52Z

go/pserver/optimizer.go

+// #cgo pkg-config: protobuf
+// #cgo CFLAGS: -I ../../
+// //FIXME: ldflags contain "build" path
+// #cgo LDFLAGS: ../../build/go/pserver/cclient/libpaddle_go_optimizer.a -lstdc++


Add -lm after -lstdc++, or build will error at:

# github.com/PaddlePaddle/Paddle/go/pserver /usr/bin/ld: ../../build/go/pserver/cclient/libpaddle_go_optimizer.a(adam_optimizer.cc.o): undefined reference to symbol 'pow@@GLIBC_2.2.5' //lib/x86_64-linux-gnu/libm.so.6: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status

there is no any part use math library, I cannot reproduce it in my computer and docker environment. Will talk with you offline
anyway, add the flags.

typhoonzero · 2017-07-01T03:35:48Z

In client.go:SendGrads:

for err := range errCh {
		if err != nil {
			return err
		}

		recv++
		if recv == len(grads) {
			break
		}
	}

should not return err, or the function will stop waiting for other goroutines to finish, causing unexpected errors, and should log the error.

typhoonzero · 2017-07-01T06:09:45Z

client_test.go create parameter.Content with size n*100, then at optimizer.cc doing Tensor* parameter = new Tensor(reinterpret_cast<float*>(param_buffer), num_bytes);, num_bytes is still the byte array size, but the re-casted data param_buffer will be 4 times less, because float is of 4 bytes.

I tried to use

Tensor* parameter =
      new Tensor(reinterpret_cast<float*>(param_buffer), num_bytes/4);

and tested ok. But better fix should be at optimizer.go:

        C.memcpy(cbuffer, unsafe.Pointer(&p.Content[0]), C.size_t(len(p.Content)))
	o.opt = C.paddle_create_optimizer((*C.uchar)(&c[0]), C.int(len(c)),
		C.paddle_element_type(p.ElementType), cbuffer, C.int(len(p.Content)),
		(*C.char)(nullPtr), 0)

to get proper size from the element type, instead of C.int(len(p.Content))

dzhwinter · 2017-07-03T04:28:37Z

Thanks a lot! fix it @typhoonzero

dzhwinter added 8 commits June 22, 2017 03:59

"integrate go and optimizer library"

5128714

"add c testing, python testing TODO"

1f217f0

"fix typo delete"

cc487c0

"fix optimizer include error"

eaed87b

"add optimizer naive link option"

7386b06

revert parameter with []byte

59b40ec

"fix cmake flags in optimizer"

12749ad

"fix cmake build flags"

c44a94b

dzhwinter mentioned this pull request Jun 26, 2017

integrate Go with optimizer library #2560

Closed

dzhwinter added 2 commits June 27, 2017 01:24

"move proto.txt to testdata folder"

cebfae9

"change log in optimizer"

864386d

helinwang requested changes Jun 27, 2017

View reviewed changes

"copy parameter content with memcpy"

242df4f

typhoonzero reviewed Jun 27, 2017

View reviewed changes

dzhwinter added 4 commits June 28, 2017 00:11

"cgo ldflags config"

7f03817

" add elementtype in optimizer"

80d9150

"remove unuse comment"

b49c9ba

"fix client test"

a2fabcc

helinwang reviewed Jun 27, 2017

View reviewed changes

"add log of create optimizer"

05ddf23

helinwang approved these changes Jun 28, 2017

View reviewed changes

dzhwinter and others added 6 commits June 28, 2017 17:19

"add optimizer full test"

42dcffc

"remove unused debug info"

6bb84b9

Merge branch 'develop' into go_optimizer

36f8b84

Merge branch 'develop' into go_optimizer

c383313

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

5b07a31

"fix protobuf depend"

f712b02

typhoonzero reviewed Jun 30, 2017

View reviewed changes

dzhwinter added 3 commits July 2, 2017 16:14

"polish cgo link option"

7b2bcbe

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

1d4f02c

"remove unused tests"

b9c15b6

dzhwinter added 2 commits July 3, 2017 12:49

test

7228533

"fix pre-commit hook failed"

99e22a8

dzhwinter force-pushed the go_optimizer branch from 2bae0f9 to 99e22a8 Compare July 3, 2017 06:55

"pass style check"

85c4352

dzhwinter merged commit f448edf into PaddlePaddle:develop Jul 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go optimizer: integrate Go with optimizer library #2610

Go optimizer: integrate Go with optimizer library #2610

dzhwinter commented Jun 26, 2017

helinwang Jun 27, 2017

dzhwinter Jun 27, 2017

helinwang Jun 27, 2017

dzhwinter Jun 27, 2017

helinwang Jun 27, 2017

dzhwinter Jun 27, 2017

helinwang Jun 27, 2017

dzhwinter Jun 27, 2017

helinwang Jun 27, 2017

dzhwinter Jun 27, 2017

helinwang Jun 27, 2017

dzhwinter Jun 27, 2017

typhoonzero Jun 27, 2017

dzhwinter Jun 27, 2017

helinwang Jun 27, 2017

dzhwinter Jun 28, 2017

helinwang Jun 27, 2017

dzhwinter Jun 28, 2017

helinwang Jun 27, 2017

dzhwinter Jun 28, 2017

helinwang Jun 27, 2017

dzhwinter Jun 28, 2017

helinwang Jun 28, 2017

helinwang Jun 27, 2017

dzhwinter Jun 28, 2017

helinwang Jun 27, 2017 •

edited

Loading

dzhwinter Jun 28, 2017

helinwang Jun 28, 2017

helinwang left a comment

typhoonzero Jun 30, 2017

dzhwinter Jul 3, 2017

typhoonzero commented Jul 1, 2017

typhoonzero commented Jul 1, 2017 •

edited

Loading

dzhwinter commented Jul 3, 2017

Go optimizer: integrate Go with optimizer library #2610

Go optimizer: integrate Go with optimizer library #2610

Conversation

dzhwinter commented Jun 26, 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

Choose a reason for hiding this comment

helinwang Jun 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typhoonzero commented Jul 1, 2017

typhoonzero commented Jul 1, 2017 • edited Loading

dzhwinter commented Jul 3, 2017

helinwang Jun 27, 2017 •

edited

Loading

typhoonzero commented Jul 1, 2017 •

edited

Loading