-
Notifications
You must be signed in to change notification settings - Fork 747
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
Update for tensorflow 0.10.0rc0 #266
Conversation
import org.bytedeco.javacpp.annotation.Cast; | ||
import org.bytedeco.javacpp.annotation.Platform; | ||
import org.bytedeco.javacpp.annotation.Properties; | ||
import org.bytedeco.javacpp.tools.Info; | ||
import org.bytedeco.javacpp.tools.InfoMap; | ||
import org.bytedeco.javacpp.tools.InfoMapper; | ||
|
||
import java.lang.annotation.*; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I this is preferable to have all the imports explicit.
Since TF project added the *.so to the targets the patch is no longer necessary
I applied the suggested changes, but it turns out that there are compilation problems at a later stage. I'm really not sure how to deal with them.
|
Another issue is that there seem to be two ops I tried:
but both finished with an error. It either can not be transformed to java in tensorflow.java, because Roughly the same problem is with |
It works for me with the following, but it looks like a lot has changed since version 0.9.0. The C++ compiler complains about of bunch of unrelated errors that we'll need to fix. .put(new Info("tensorflow::ops::Cast").pointerTypes("CastOp"))
.put(new Info("tensorflow::ops::Const").pointerTypes("ConstOp")) Also, building the "//tensorflow:libtensorflow.so" doesn't get us the C++ ops API, but "//tensorflow:libtensorflow_cc.so" does so we should probably be using that. |
I have been able to compile it if I skip or remove all of the offending C++ types. I can live without the Ops, but I need the I have tried:
but this results in:
If I manually edit
with:
then it compiles, but I have no idea either how to achieve this with the How can I represent unique pointers? |
…o support `unique_ptr` containers (issue bytedeco/javacpp-presets#266)
Just now, I added support for |
What's the status on this? |
Waiting for Tensorflow 0.10 to be released. |
But your current version (this PR) is fully working with the current RC? |
It works for me™. However I had to remove some ops in order to get it to work. But if your goal is to open a session, load a model and then run predictions, then that has worked for me. |
Do you have a branch where you have removed those ops / a branch that is compiling? |
# Conflicts: # tensorflow/src/main/java/org/bytedeco/javacpp/presets/tensorflow.java
@andreas-eberle I merged master and resolved the conflicts. Now it compiles for me with |
Hi, I tried to compile it using "mvn install --projects .,tensorflow" on a Mac, however I got this (during linking stage): ld: library not found for -ltensorflow This step works fine on previous javacpp-presets commits (with tensorflow 0.9). |
That has to be something trivial, like a botched tensorflow install. |
@saudet I don't know why google is not tagging TF final release on GitHub, but on the website they have r0.10 as the default when installing. Also there have been no substantial commits in the past weeks to r0.10. So I have switched to the head of the r0.10 branch and built from there. It seems fairly stable now, so maybe you can merge this... |
I was able to build @mirosval's code with javacpp 1.2.4. But got the exact same error of |
It should work if you check out at the above mentioned commit. Except you won't have access to the ops. |
Ah, here's the issue. We need to build the "libtensorflow_cc.so" target, not "libtensorflow.so". The patch in the presets for old versions of TensorFlow included the ops in "libtensorflow.so", but they've since made them available as part of "libtensorflow_cc.so" instead. By building "libtensorflow_cc.so" we also get all the header files for the ops. We don't need to include, parse, and compile the Similarly, the |
What is the status on this? @mirosval: Does this version fully work for you now? If yes, is there something else preventing this from being merged? |
It does work for me. However I only use a subset of the functionality (create session, load protobuf, run prediction), so take it with a grain of salt. I think generally it could be merged, but that's @saudet's call. |
I tried to build this PR and had some issues:
I was able to fix this by updating the version of the parent pom in all the other poms to the same one used by the tensorflow module. Is there another way than setting all these versions manually?
Do you have any idea what I'm doing wrong? Is it working for you with a clean setup? |
I think you need to do |
@mirosval: Sorry, that was a copy and paste failure. I actually ran it without the skip option (I updated my question). I just also tried to first manually run the cppbuild.sh script and skip that step in the maven process. Unfortunately that gave the same result. Any other ideas? EDIT: I just retried it again to make sure I didn't use the wrong command line arguments. Still I have the same problems. The complete output can be found here: https://gist.github.com/andreas-eberle/51c0bb15641f3198ca00860981363452 |
I run both, Clearly it looks like your tensorflow was not built, or it can not be found in that path. I did come across a similar problem on Linux, where I had to change the path where bazel put header files. The best I can do for you is share this gist that contains the Dockerfile I used to build the linux version of the project. |
"tensorflow/cc/framework/ops.h", | ||
"tensorflow/cc/framework/cc_op_gen.h", | ||
"tensorflow_adapters.h"}, | ||
link = "tensorflow"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mirosval: I think I found it. The build worked when I changed to link agains tensorflow_cc
. I guess this is because you had to switch to build libtensorflow_cc instead of libtensorflow which it was in version 0.9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that needs to be changed. Thanks for pointing this out!
We need to change the versions in the pom.xml files, yes, sorry about that. I need to figure out a better way of dealing with this. I think I'll just start using a major version number for all pom.xml files on all master branches (like 1.3-SNAPSHOT) and when I need to do some minor release of a past version (say 1.2.x), I'll just do it as a branch. Anyway, the build also works for me, but there are still some things to fix... The ops header files are missing, at least these:
And we probably don't need this: .put(new Info("const_op.h").skip())
.put(new Info("math_ops.h").skip())
.put(new Info("array_ops.h").skip()) This doesn't appear to be used:
This is normal behavior for JavaCPP: // For some reason the #define this was in got parsed even though its #ifdef guard failed
// see "tensorflow/core/framework/selective_registration.h" for more details
.put(new Info("SHOULD_REGISTER_OP_GRADIENT").skip()) It will try to parse everything by default, because it usually works better that way. This is the way to fix this: .put(new Info("SELECTIVE_REGISTRATION").define(false)) I'm afraid this will also skip the .put(new Info("tensorflow::OpShapeInferenceFn").skip()) Something like this would be better: .put(new Info("tensorflow::OpRegistrationData::shape_inference_fn")
.javaText("@MemberSetter public native OpRegistrationData shape_inference_fn(@ByVal ShapeInferenceFn shape_inference_fn);")) The rest seems good :) Thanks! |
Seems that the ops are back and it compiled for me. So assuming you update the javacpp version in all your poms to 1.2.5-SNAPSHOT it should compile for you as well. However all your other points, like the skipped headers and iterators, etc don't seem to work for me, I always run into issues trying to remove them. |
Looks like |
Everything works after including your patch, so I think from my side we're good to go. I have allowed edits from maintainers, so in case there are some other changes necessary you should be able to push directly into my master branch. |
The Const, Cast and few other static import operation does not work anymore. The version of the tensorflow in the Readme should be updated to 0.10.0. Somewhere we have to enforce that the javacpp version MUST be 1.2.5 or greater. |
… work around name clashes (pull bytedeco/javacpp-presets#266)
@mirosval Actually the Const functions are used in the .put(new Info("tensorflow::ops::Cast").cppTypes("class tensorflow::ops::Cast").pointerTypes("CastOp"))
.put(new Info("tensorflow::ops::Const").cppTypes("class tensorflow::ops::Const").pointerTypes("ConstOp")) I didn't notice I had push access to your repo. Anyway, I'd like to get all those commits squashed, and it might as well be under your name. The following commands seem to work, but feel free to do it your way! Thanks
@Mistobaan Once this and a couple of other things are in, I'll be switching all versions to 1.3-SNAPSHOT, and of course update the README.md file after the next release, whenever that happens. Will also be working on the new mapping for Const and Cast later, but help would be welcome. |
@saudet if you point me in the right direction on the work that needs to be done for Const & Cast I might be able to take a look at it. |
@saudet I think you can set up the squashing directly on GitHub (see https://github.com/blog/2141-squash-your-commits) I'm trying to put the Cast & Const back using your snippet, but I have run into an issue with DataType not being defined, despite 800 lines below in another method it gets correctly recognized and cast to int (it's an enum from core/framework/types.pb.h). It seems that with I'm on vacation next week so I won't have time to look into this. |
@mirosval Oh, I didn't realize GitHub had a squash option there now. Good to know. :) Let's try it out. @Mistobaan Thanks, will look into updating |
@Mistobaan Done: dd4ffa7 The ops API got a bit uglier, so we might want to create nice wrappers for that, but anyway it works. Let me know if there is anything missing! |
getting this error now:
|
@Mistobaan That just means you have an old version of JavaCPP somewhere. Please try again with the latest version on the master branch. |
Please if somebody could review this.
For example I had to rename Cast op to CastOp and Const op to ConstOp otherwise it wouldn’t compile.
I’m pretty sure also other things need to be improved/fixed as I don’t really understand all of the ramifications of these changes