Skip to content

Consider moving to bindgen for tensorflow-sys #57

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

Closed
daschl opened this issue Feb 26, 2017 · 3 comments
Closed

Consider moving to bindgen for tensorflow-sys #57

daschl opened this issue Feb 26, 2017 · 3 comments
Labels

Comments

@daschl
Copy link
Contributor

daschl commented Feb 26, 2017

Hi,

I've used bindgen with great success for other c-bindings I maintain (couchbase-sys) and I was wondering if it could make sense to migrate tensorflow-sys to it as well (https://crates.io/crates/bindgen). It would allow us with very little effort to pull in the c headers and just generate the low level binding, and stay up to date.

Here is basically all it takes to generate the API from a header file: https://github.com/couchbaselabs/couchbase-rs/blob/master/couchbase-sys/build.rs#L31

If that sounds like a good path to investigate I could take it on and see if I can make it work.

@adamcrume
Copy link
Contributor

We were using bindgen originally, but it broke at one point, so we ripped it out because the amount of generated code was small anyway. When it broke, there was no working version of bindgen on crates.io for a couple of months (https://webcache.googleusercontent.com/search?q=cache:d-V7reDJfwYJ:https://github.com/Yamakaky/rust-bindgen/issues/251+&cd=1&hl=en&ct=clnk&gl=us&client=ubuntu) and we were concerned about having an unreliable dependency.

Since the size of the C API is increasing, it might be worth it to revisit bindgen if it is more actively maintained now.

There might be other complications, though, like finding the header file if we're trying to build with an existing TensorFlow installation. Not insurmountable, of course, but it adds complexity.

@daschl
Copy link
Contributor Author

daschl commented Feb 27, 2017

Makes sense - the good news is mozilla took it over and is now actively maintaining it again. Another option I've been considering for my crates is to just include a script for contributors to refresh the API based on a source code and then just check in the generated files for the C lib versions. Then, based on a compile time flag giving it the tag it could include the proper FFI code, making it possible to support more than one version at the same time. I'm not sure if this is perfect either, but it would also remove the dependencies needed to build the library, since you need clang and all kinds of other dependencies installed to make it work, which I don't really like but there is no other option around at the moment if you want it fully integrated.

@nbigaouette-eai
Copy link
Contributor

I've attempted to use bindgen to generate the binding. See #67.

ramon-garcia pushed a commit to ramon-garcia/tensorflow-rust that referenced this issue May 20, 2023
* Optimize page reading by not create array from bytes owner

* - fixes misaligned data from dictionary-encoded columns with null values (tensorflow#57)

- added test and test data file

Co-authored-by: Chirag Gupta (AZURE) <[email protected]>
Co-authored-by: Carlos Orrego <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants