search

finalfusion / finalfusion-tensorflow-ops

Tensorflow Op for finalfusion embeddings
https://finalfusion.github.io/
Other
2 stars 0 forks source link

Gate ctest in setup.py, fix tests. #13

Closed sebpuetz closed 5 years ago

sebpuetz commented 5 years ago

Relevant changes are in setup.py, the rest is fairly trivial.

Fixes #8

sebpuetz commented 5 years ago

Tensorflow 2.0 is upon us. I pinned the version to 1.14 for now since some tests are broken because of some trivial stuff. tf.enable_eager_mode() doesn't exist anymore and shapes are handled differently in 2.0.

Apart from that, the build works and the eager tests pass.

danieldk commented 5 years ago

Tensorflow 2.0 is upon us. I pinned the version to 1.14 for now since some tests are broken because of some trivial stuff. tf.enable_eager_mode() doesn't exist anymore and shapes are handled differently in 2.0.

We'll see that more of that in the coming time. We should start pinning Tensorflow versions. Tobias and I also discussed life post-Tensorflow 1.x a bit today and concluded that it is probably best to start investigating PyTorch and tch-rs for greenfield projects.

sebpuetz commented 5 years ago

Tensorflow 2.0 is upon us. I pinned the version to 1.14 for now since some tests are broken because of some trivial stuff. tf.enable_eager_mode() doesn't exist anymore and shapes are handled differently in 2.0.

We'll see that more of that in the coming time. We should start pinning Tensorflow versions. Tobias and I also discussed life post-Tensorflow 1.x a bit today and concluded that it is probably best to start investigating PyTorch and tch-rs for greenfield projects.

I agree that we should look in that direction, although I still think it'd be nice to support both 1.x and 2.x with this op since only test cases would need to be customized.

danieldk commented 5 years ago

For this op definitely.

I am not sure if I understand the changes. When does it what tests now?

twuebi commented 5 years ago

I agree that we should look in that direction, although I still think it'd be nice to support both 1.x and 2.x with this op since only test cases would need to be customized.

If it's not too much effort it's probably nice to have both a 1.x and a 2.x branch

sebpuetz commented 5 years ago

I am not sure if I understand the changes. When does it what tests now?

setup.py build_ext has a --test flag which optionally runs ctest in the build_temp folder when the extension is built. That essentially tests finalfusion-cxx and finalfusion-ffi.

The finalfusion_tensorflow package is tested after install through pytest. The testcases are called individually since the enable_eager_mode() call spills over into test_graph_mode.

sebpuetz commented 5 years ago

I agree that we should look in that direction, although I still think it'd be nice to support both 1.x and 2.x with this op since only test cases would need to be customized.

If it's not too much effort it's probably nice to have both a 1.x and a 2.x branch

Shouldn't be necessary:

https://github.com/finalfusion/finalfusion-tensorflow-ops/tree/v2-compat passes on both 2.0 and 1.14.

sebpuetz commented 5 years ago

Tensorflow 2.0 is upon us. I pinned the version to 1.14 for now since some tests are broken because of some trivial stuff. tf.enable_eager_mode() doesn't exist anymore and shapes are handled differently in 2.0.

We'll see that more of that in the coming time. We should start pinning Tensorflow versions. Tobias and I also discussed life post-Tensorflow 1.x a bit today and concluded that it is probably best to start investigating PyTorch and tch-rs for greenfield projects.

https://pytorch.org/tutorials/advanced/cpp_extension.html seems to be doable. Going by torch's FAQ, resources are freed once they go out of scope in Python or are del'd. So I'd expect resource management to be a bit simpler for us, too.

I'd first like to wrap up the tensorflow stuff before jumping into a torch extension or if one of you feels inclined to work on a torch extension, just go ahead!

sebpuetz commented 5 years ago

I'm closing this PR in favor of a new one that will move to testing the Python package. The various small changes are merged through other PRs (see #14 #15 #16 #17).