-
Notifications
You must be signed in to change notification settings - Fork 423
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
Tests for tensorflow-sys fail #66
Comments
We'll probably have to rope in a TensorFlow developer, because that stack trace looks like 100% C++. |
This looks very weird as not much is executed. I suspect it's a rust related setup issue... If I comment out tensorflow-sys/src/lib.rs's line 558 the test pass! Here's the failure:
And here's the success after commenting line 558:
|
There's something going on when linking While the
it passes in release mode:
Probably because of dead code elimination or something like that... |
Running through valgrind reveals more information:
|
Could it be a conflict with jemalloc? tensorflow can be compiled with jemalloc 4.4.0 (see https://github.com/tensorflow/tensorflow/blob/master/tensorflow/workspace.bzl#L447-L457). As suggested from IRC, I've compiled using a nightly and the system allocator (instead of jemalloc) by adding the following at the top of
(see https://doc.rust-lang.org/book/custom-allocators.html). Running the "test" (note that there is none) succeeds:
Using default jemalloc:
I might try to recompile tf with |
Recompiled tensorflow 1.0.1 with jemalloc disabled ( |
Thanks for debugging; that's great detective work! And good idea using valgrind, we should add that as a test (#69). Unfortunately, this is a bit of a pickle. I don't want to require building with a nightly release, so changing the allocator in Rust is probably not an option. On the other hand, I'm trying to add support for prebuilt TensorFlow binaries (#65), and they seem to be built with jemalloc enabled. I'm also still lost on why the tests for the |
I expect Throwing it out there that these issues might also appear because of this. |
It would surprise me that TF's C API would be platform dependent. Its header should be identical on all platform... Or is it not? It seemed more reliable to use bindgen to generate the Rust wrapper and commit the result rather than include a The script to generate the binding is included in the repository so that anybody can either update the binding or verify that the binding generated is the same as the committed one. I invite anybody to run this script to verify the assumption. If I am wrong about the assumption then bindgen will have to be added as a build dependency. I'll have to revisit this issue at some point, the build process changed a little bit since I filled this. |
It looks like running |
I just tested 15a40b0 on both macOS and Linux. Both are using the pre-built tensorflow downloaded build I cannot reproduce the problem I had back in March; all tests (for both crates) are passing without issue. I'm not sure what went wrong back then. Maybe there was something with the pre-built vs compiled integration, the At least now it works! Closing this. |
* dotnet loves parquet! * typo
Running
cargo test
in thetensorflow-sys
directory fails (but tests pass for the main crate).Here's the output:
Here's a backtrace from running
/home/nbigaouette/tensorflow_rust.git/target/debug/deps/tensorflow_sys-b18d6c19e08d67bd
through gdb:The text was updated successfully, but these errors were encountered: