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

Simplify generic parameterization on some operations #176

Closed
karllessard opened this issue Dec 28, 2020 · 10 comments · Fixed by #193
Closed

Simplify generic parameterization on some operations #176

karllessard opened this issue Dec 28, 2020 · 10 comments · Fixed by #193

Comments

@karllessard
Copy link
Collaborator

I'm working on the reified generation for the Kotlin API, and I'm noticing that lots of methods have unnecessary type parameters that makes the reified usage much less nice (since you have to specify all type parameters if you specify one). The best example is probably cast, which has the signiture:

<U extends TType, T extends TType> Cast<U> cast(Operand<T> x, Class<U> DstT, Cast.Options... options)

T is completely unnecessary and could be replaced with ? without issue, but it prevents cast<TInx32>(x) usage from Kotlin. This shows up in a number of Ops, mostly with the unnecessary type parameters on the input. It essentially needs a "is this type param only bounded by TType and only used on inputs" check.

Originally posted by @rnett in #174 (comment)

@karllessard
Copy link
Collaborator Author

karllessard commented Dec 28, 2020

I think it would be worth it to make this simplification yes, but the rule might be a bit more tricky. In fact, a generic parameter should be enforced if any input or output of the method should share the same parameter. In other words:

  • the returned value parameter must match the parameter of an input
  • more than one input must share the same parameter

Even if a single parameter is not bound to TType but to one of its subtype (like TNumber), we can handle it with a wildcard, e.g. for a hypothetic toFloat operation:

Operand<TFloat> toFloat(Operand<? extends TNumber> input)

So this logic must be added to the C++ op generator.

@rnett
Copy link
Contributor

rnett commented Jan 14, 2021

@karllessard do you have any idea when you would get to this? Should we wait for it before merging in the Kotlin API?

@karllessard
Copy link
Collaborator Author

Hey @rnett , I was not necessarily planning to do this work anytime soon, do you want to give it a try?

@rnett
Copy link
Contributor

rnett commented Jan 15, 2021

I can take a look, at least. Do you have any recommendations on IDE setup? I had issues getting bazel to work with CLion when I tried it.

@karllessard
Copy link
Collaborator Author

When I was doing more C/C++ code than Java for TensorFlow, I was using Eclipse which supports well both languages, even in the same project (at that time, I had to edit the project file manually to enable this hybridity, I don't know if that can be done differently now).

@rnett
Copy link
Contributor

rnett commented Jan 22, 2021

@karllessard I've got this mostly working, I think, but Javadocs aren't being generated any more, even on master. I'm running with ./bazel-out/k8-opt/bin/external/org_tensorflow/tensorflow/libtensorflow_cc.so --output_dir=./src/gen/java --api_dirs=./src/bazel/api_def, any idea what would cause this? Using bazel-tensorflow-core-api/external/org_tensorflow/tensorflow/core/api_def doesn't work either, and makes everything a core op.

@karllessard
Copy link
Collaborator Author

karllessard commented Jan 23, 2021

I'm not sure to understand, you are running the libtensorflow_cc.so library? or that's probably a mistake, should be the gen_op_java binary or something like that.

(oh yes I recall now, you need to pass the path to the library as well as an argument, that's something relatively new.)

But you need to pass both dirs in the api_dirs option, separated by commas if I recall correctly. Also the order matters (sorry for not being more clear in my instructions but I'm not in front of my computer right now)

@rnett
Copy link
Contributor

rnett commented Jan 23, 2021

Ok, It looks like it should be bazel-tensorflow-core-api/external/org_tensorflow/tensorflow/core/api_def/base_api,src/bazel/api_def. This doesn't generate a few ops (DummySeedGenerator, TensorScatterMax, and TensorScatterMin), should I be concerned? The other order doesn't generate them either and misses the group for some data ops.

@rnett
Copy link
Contributor

rnett commented Jan 23, 2021

Also, for anyone else trying to use the bazel project, I got it working w/ an old version of CLion (so that I could use the bazel plugin), as long as ran ./configure in a cloned tensorflow project, and copied the resulting .tf_configure.bazelrc to tensorflow-core-api.

@karllessard do you think we should make a CONTRIBUTORS.md style file with build tips like this, the exact command to use for cc op generation, etc?

@karllessard
Copy link
Collaborator Author

@karllessard do you think we should make a CONTRIBUTORS.md style file with build tips like this, the exact command to use for cc op generation, etc?

Yes, we've mentioned that during the last meeting, we definitely need better documentation when it comes to build from sources and guide the users in the process, we have a lot of issues raised by them that would probably be avoided by just having this documentation, please feel free to start it!

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 a pull request may close this issue.

2 participants