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.

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

Import Java operators generator from main repo #2

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

karllessard
Copy link
Collaborator

@karllessard karllessard commented Oct 10, 2019

@sjamesr, can you please review this PR?

I've imported the C++ generator of Java operators (found in the main repository here) in our new repository, so that the SIG can maintain and modify those sources as well. That will be required soon to add support for Tensor NIO. Eventually that code will be migrated to Java but for now we will live with this C++ implementation.

I've also imported all api_def protos for Java (originally found here, almost 1000 files!) because it make sense that we maintain those as well. But I had to patch the TensorFlow external code to be able to access the base definitions, which should remain in the main repo... I did not know how to do it otherwise.

@saudet: please also take a look, you might find it interesting.

@saudet
Copy link
Contributor

saudet commented Oct 10, 2019

All those pbtxt files look very simple. Shouldn't they be generated somehow? Is the idea that we're not going to need them anyway in the future and they are just there temporarily in /src/bazel?

@karllessard
Copy link
Collaborator Author

I don't know what's the plan for the future for the API defs, I raised that issue to Google in a RFC that is actually James's but got no clear respond to it yet. (@sjamesr?)

But as simple as they look, they contain information that is nowhere else to be found, which is the endpoint package used for grouping the operators (e.g. math.Abs), the visibility of the operator in Java and the operator name when it needs to be overridden (e.g. Switch became SwitchCond).

So even if we want to generate those .pbtxt files, we still need to keep this mapping somewhere on our side.

Now for the temporary status, we'll migrate this code to Java at some point but that raised a question in my mind: If we plan to move the generator to tensorflow-core-processor as we were planning, how can it links to the native library? It's a chicken&egg situation. It would have been nice if it could also use the JavaCPP bindings for retrieving the list of ops (and api defs?) but that won't be possible outside tensorflow-core-api right now. Just throwing an idea but would it make sense to move the bazel workspace to tensorflow-core instead and "share" it among all child core artifacts?

@saudet
Copy link
Contributor

saudet commented Oct 11, 2019

Once we're not dependent on Bazel to generate the ops wrappers, we can easily move everything related to those to another module like tensorflow-core-ops that depends on both tensorflow-core-api and tensorflow-core-generator to do its stuff. I don't see any issues with that, do you? For now though, until Bazel is still in the picture, it's probably best to confine it to only one module and not complicate things unnecessarily, yes.

@karllessard
Copy link
Collaborator Author

Yes, I think that should do it as well. In fact that tensorflow-core-ops artifact could contain not only the generated primitive ops but also all wrappers between keras and the core api. So yeah, let’s keep it like that for now.

@karllessard karllessard merged commit 02d0403 into tensorflow:master Oct 15, 2019
@karllessard karllessard deleted the operator-generator branch October 15, 2019 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants