add cgo wrapper for recordio, make go_cmake automatically download go… by helinwang · Pull Request #2294 · PaddlePaddle/Paddle

helinwang · 2017-05-26T23:54:41Z

… dependency

cd Paddle/paddle/go/crecordio
mkdir build && cd build && cmake .. && make
./test/recordio_test

… dependency

wangkuiyi · 2017-05-27T00:51:30Z

paddle/go/cmake/golang.cmake

-  install(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/${NAME} DESTINATION bin)
-endfunction(add_go_executable)
-
+set(PADDLE_IN_GOPATH "${GOPATH}/src/github.com/PaddlePaddle")


Why don't we put these .cmake files /cmake, but in /go/cmake?

I think they are not ready yet, this cmake's purpose is for not blocking the development. I was planning to move to /cmake with the help from @gangliao in another PR.

wangkuiyi · 2017-05-27T00:52:52Z

paddle/go/cmake/golang.cmake

+set(PADDLE_IN_GOPATH "${GOPATH}/src/github.com/PaddlePaddle")
+file(MAKE_DIRECTORY ${PADDLE_IN_GOPATH})

 function(ADD_GO_LIBRARY NAME BUILD_TYPE)


Is this add_go_library actually the go_library in our design?

Yes, will rename.

wangkuiyi · 2017-05-27T00:56:34Z

paddle/go/crecordio/crecordio.go

+}
+
+//export paddle_new_writer
+func paddle_new_writer(path *C.char) C.writer {


Why not create_recordio_writer? I mean this seems not specific for paddle.

wangkuiyi

I think we can add recordio.Multi{Reader/Writer} to the Go code, before we wrap the functionality for C/C++.

helinwang · 2017-05-27T14:52:51Z

@wangkuiyi Added recordio.MultiReader to Go code. We probably don't need MultiWriter currently.

wangkuiyi · 2017-05-30T19:05:56Z

paddle/go/recordio/multi_reader.go

+)
+
+// MultiScanner is a scanner for multiple recordio files.
+type MultiScanner struct {


We'd rename this high-level API to Scanner. And we rename the existing Scanner to RangeScanner. We can do this in a future PR.

wangkuiyi · 2017-05-30T19:06:47Z

paddle/go/recordio/multi_reader.go

+}
+
+// NewMultiScanner creates a new MultiScanner.
+func NewMultiScanner(paths []string) (*MultiScanner, error) {


[]string to ...string, so that it can accept only one filename pattern easily?

Good point! Done.

wangkuiyi · 2017-05-30T19:07:35Z

paddle/go/recordio/reader.go

+// Err returns the first non-EOF error that was encountered by the
+// Scanner.
+func (s *Scanner) Err() error {
+	if s.err == io.EOF {


Good point!

wangkuiyi · 2017-05-30T19:09:08Z

paddle/go/crecordio/crecordio.go

+	scanner *recordio.MultiScanner
+}
+
+func cArrayToSlice(p unsafe.Pointer, len int) []byte {


It seems that this function duplicates with the one in cclient.go?

We can create a package to hold a few commonly used functions. Since they are simple, and probably will never change, I duplicated the code when different package uses them. I can create a package to hold them if you think it's better.

wangkuiyi · 2017-05-30T19:09:54Z

paddle/go/crecordio/crecordio.go

+}
+
+//export write_recordio
+func write_recordio(writer C.writer, buf *C.uchar, size C.int) int {


It seems that this function should be named write_records or recordio_write. I prefer the later.

Should we make this function return the number of bytes like C's write function?

Good point! Done.

wangkuiyi · 2017-05-30T19:12:52Z

paddle/go/crecordio/crecordio.go

+}
+
+//export read_next_item
+func read_next_item(reader C.reader, size *C.int) *C.uchar {


It seems that this function should be defined:

func recordio_read(reader C.reader, record *C.uchar) int

so to return the number of read bytes as C's read does?

Good point! Done.

wangkuiyi · 2017-05-30T19:14:09Z

paddle/go/crecordio/crecordio.go

+}
+
+//export release_recordio
+func release_recordio(writer C.writer) {


Rename this function to release_recordio_writer?

wangkuiyi · 2017-05-30T22:03:19Z

Let 's move go/cclient to `go/pserver/cclient'.

wangkuiyi · 2017-05-30T22:03:33Z

Let's move paddle/go to go.

helinwang · 2017-05-30T22:09:29Z

Thanks! Will fix the issues with a follow up PR.

add cgo wrapper for recordio, make go_cmake automatically download go…

a868d01

… dependency

helinwang requested review from gangliao, gongweibao and wangkuiyi May 26, 2017 23:54

fix comment

9e8503b

helinwang mentioned this pull request May 27, 2017

implement pserver RPC part, and simple parameter partition. #2243

Merged

helinwang added 3 commits May 27, 2017 00:31

clang format

f074198

do not include paddle/go into cmake yet.

cab5076

release reader in c example

0e80dad

wangkuiyi reviewed May 27, 2017

View reviewed changes

wangkuiyi requested changes May 27, 2017

View reviewed changes

helinwang added 2 commits May 27, 2017 14:49

fix according to comments

633171c

fix according to comments

ec5db38

wangkuiyi reviewed May 30, 2017

View reviewed changes

fix according to comments

2fa274c

wangkuiyi approved these changes May 30, 2017

View reviewed changes

This was referenced May 30, 2017

Move package go/cclient to go/pserver/cclient #2309

Closed

Move paddle/go to go #2310

Closed

helinwang merged commit 1d5f3a9 into PaddlePaddle:develop May 30, 2017

helinwang deleted the recordio_c branch May 30, 2017 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cgo wrapper for recordio, make go_cmake automatically download go…#2294

add cgo wrapper for recordio, make go_cmake automatically download go…#2294
helinwang merged 8 commits intoPaddlePaddle:developfrom
helinwang:recordio_c

helinwang commented May 26, 2017 •

edited

Loading

wangkuiyi May 27, 2017

helinwang May 27, 2017 •

edited

Loading

wangkuiyi May 27, 2017

helinwang May 27, 2017

helinwang May 27, 2017

wangkuiyi May 27, 2017

wangkuiyi left a comment

helinwang commented May 27, 2017

wangkuiyi May 30, 2017

helinwang May 30, 2017

wangkuiyi May 30, 2017

helinwang May 30, 2017

wangkuiyi May 30, 2017

wangkuiyi May 30, 2017

helinwang May 30, 2017

wangkuiyi May 30, 2017

wangkuiyi May 30, 2017

wangkuiyi May 30, 2017

helinwang May 30, 2017

wangkuiyi May 30, 2017

helinwang May 30, 2017

wangkuiyi May 30, 2017

helinwang May 30, 2017 •

edited

Loading

wangkuiyi commented May 30, 2017

wangkuiyi commented May 30, 2017

helinwang commented May 30, 2017

Labels

2 participants

Conversation

helinwang commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

helinwang May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

wangkuiyi left a comment

Choose a reason for hiding this comment

helinwang commented May 27, 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

helinwang May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

wangkuiyi commented May 30, 2017

wangkuiyi commented May 30, 2017

helinwang commented May 30, 2017

Labels

2 participants

helinwang commented May 26, 2017 •

edited

Loading

helinwang May 27, 2017 •

edited

Loading

helinwang May 30, 2017 •

edited

Loading