Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
May 30, 2017
Merged

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

merged 8 commits into from
May 30, 2017

Conversation

helinwang
Copy link
Contributor

@helinwang helinwang commented May 26, 2017 •
edited
Loading

… dependency

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

@helinwang helinwang mentioned this pull request May 27, 2017
Merged
wangkuiyi
wangkuiyi reviewed May 27, 2017
View reviewed changes
paddle/go/cmake/golang.cmake Outdated
install(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/${NAME} DESTINATION bin)
endfunction(add_go_executable)

set(PADDLE_IN_GOPATH "${GOPATH}/src/github.com/PaddlePaddle")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@helinwang helinwang May 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

paddle/go/cmake/golang.cmake Outdated
endfunction(add_go_executable)

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

function(ADD_GO_LIBRARY NAME BUILD_TYPE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this add_go_library actually the go_library in our design?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

paddle/go/crecordio/crecordio.go Outdated
}

//export paddle_new_writer
func paddle_new_writer(path *C.char) C.writer {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

wangkuiyi
wangkuiyi requested changes May 27, 2017
View reviewed changes
Copy link
Collaborator

@wangkuiyi wangkuiyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@helinwang
Copy link
Contributor Author

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

wangkuiyi
wangkuiyi reviewed May 30, 2017
View reviewed changes
paddle/go/recordio/multi_reader.go Outdated
)

// MultiScanner is a scanner for multiple recordio files.
type MultiScanner struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

paddle/go/recordio/multi_reader.go Outdated
}

// NewMultiScanner creates a new MultiScanner.
func NewMultiScanner(paths []string) (*MultiScanner, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Done.

paddle/go/recordio/reader.go Outdated
// Err returns the first non-EOF error that was encountered by the
// Scanner.
func (s *Scanner) Err() error {
if s.err == io.EOF {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

paddle/go/crecordio/crecordio.go Outdated
scanner *recordio.MultiScanner
}

func cArrayToSlice(p unsafe.Pointer, len int) []byte {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

paddle/go/crecordio/crecordio.go Outdated
}

//export write_recordio
func write_recordio(writer C.writer, buf *C.uchar, size C.int) int {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Done.

paddle/go/crecordio/crecordio.go Outdated
}

//export read_next_item
func read_next_item(reader C.reader, size *C.int) *C.uchar {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Done.

paddle/go/crecordio/crecordio.go Outdated
}

//export release_recordio
func release_recordio(writer C.writer) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this function to release_recordio_writer?

Copy link
Contributor Author

@helinwang helinwang May 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@wangkuiyi
Copy link
Collaborator

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

@wangkuiyi
Copy link
Collaborator

Let's move paddle/go to go.

This was referenced May 30, 2017
Closed
Closed
@helinwang
Copy link
Contributor Author

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

@helinwang helinwang merged commit 1d5f3a9 into PaddlePaddle:develop May 30, 2017
@helinwang helinwang deleted the recordio_c branch May 30, 2017 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangkuiyi wangkuiyi wangkuiyi approved these changes

@gangliao gangliao Awaiting requested review from gangliao

@gongweibao gongweibao Awaiting requested review from gongweibao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@helinwang @wangkuiyi