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 description how to generate ops wrapper #7626

Merged
merged 2 commits into from
Mar 7, 2017
Merged

Add description how to generate ops wrapper #7626

merged 2 commits into from
Mar 7, 2017

Conversation

Lewuathe
Copy link
Contributor

Generating instruction of ops wrapper is useful for development of go binding.

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@vrv vrv requested a review from asimshankar February 17, 2017 16:48
@vrv vrv added stat:awaiting tensorflower Status - Awaiting response from tensorflower type:docs-bug Document issues labels Feb 17, 2017
asimshankar
asimshankar reviewed Feb 17, 2017
View reviewed changes
Copy link
Contributor

@asimshankar asimshankar left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Some suggestions below, to link to the protoc and simplify the command.

Out of curiosity (and it doesn't affect this PR), might I ask what led you to have to generate your own wrappers?

tensorflow/go/README.md Outdated
- Downloading Tensorflow repository under GOPATH

```
# Gegerate Go source from protobuf files.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be replaced by a single:

go generate github.com/tensorflow/tensorflow/tensorflow/go/op

(Doing so does the generation needed for the genop tool as well as actually generating wrappers.go)

tensorflow/go/README.md Outdated
@@ -118,6 +118,25 @@ from source.
go test github.com/tensorflow/tensorflow/tensorflow/go
```

### Generate Ops Wrapper

Tensorflow ops wrappers in Go is generated by a script. Here is how to generate Go source files for Tensorflow ops.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We tend to prefer TensorFlow, not Tensorflow :)

tensorflow/go/README.md Outdated
@@ -118,6 +118,25 @@ from source.
go test github.com/tensorflow/tensorflow/tensorflow/go
```

### Generate Ops Wrapper

Tensorflow ops wrappers in Go is generated by a script. Here is how to generate Go source files for Tensorflow ops.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps reword:
Go functions corresponding to TensorFlow operations are generated in op/wrappers.go. To regenerate them:

Prerequisites:

@asimshankar asimshankar added stat:awaiting response Status - Awaiting response from author and removed stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Feb 17, 2017
@Lewuathe
Copy link
Contributor Author

Lewuathe commented Feb 18, 2017 •
edited

@asimshankar Thanks for review. I updated accordingly.

might I ask what led you to have to generate your own wrappers?

Just for development. I tried to find which API is not generated as wrappers. And there seems to be some diffs between current wrappers.go and regenerated one. When is wrappers.go generated usually?

@asimshankar
Copy link
Contributor

We currently update wrappers.go with a cron job, which you'll see in the file history

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

asimshankar
asimshankar approved these changes Feb 18, 2017
View reviewed changes
Copy link
Contributor

@asimshankar asimshankar left a comment

Choose a reason for hiding this comment

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

Thanks for the change. Took the liberty to fix a typo or two.

@asimshankar asimshankar added awaiting testing (then merge) and removed stat:awaiting response Status - Awaiting response from author labels Feb 18, 2017
@vrv
Copy link

vrv commented Feb 18, 2017

@tensorflow-jenkins test this please

@maciekcc
Copy link

Jenkins, test this please.

@asimshankar
Copy link
Contributor

@maciekcc : The CLA failure is just because it's being cautious about my typo fixes. I believe this is good to merge.

@teamdandelion
Copy link
Contributor

We should merge this. Currently I'm not able to do so, due to the red status from CLAbot, even though we've verified that we have CLA consent.

@teamdandelion teamdandelion self-assigned this Mar 7, 2017
@vrv vrv merged commit b7adae6 into tensorflow:master Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asimshankar asimshankar asimshankar approved these changes

Assignees

@asimshankar asimshankar

@teamdandelion teamdandelion

Labels
cla: yes type:docs-bug Document issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Lewuathe @tensorflow-jenkins @asimshankar @googlebot @vrv @maciekcc @teamdandelion