Refactor tests to use pytest parametrized

[lgeiger]

This should simplify some of the logic of the tetst, I can change the TFLite test once #88 is merged.

[lgeiger]

I prefer the API of https://docs.pytest.org/en/latest/parametrize.html#parametrize a lot, which makes things like this a lot easier. But I don't think it will work together with tf.test.TestCase.

[lgeiger]

TF 1.x tests still run on Python 3.4 😢 so my attempts to simplify this further failed.

[arashb]

TF 1.x tests still run on Python 3.4 😢 so my attempts to simplify this further failed.

shall we drop the TF 1 support entirely? what is the status with Larq in respect to TF 1 support?

[lgeiger]

shall we drop the TF 1 support entirely?

I'd be in favour. We could also only support 1.15+ which uses the same docker container as TF 2

what is the status with Larq in respect to TF 1 support?

We still support TF 1 and there are no plans to drop it just yet. Though I am happy to re-evaluate if it makes our life difficult.

[Tombana]

For now, the compute engine is mostly meant for tflite. Until we start adding CUDA operations, the TF side of the compute engine only exists to be able to define a model and convert it to TF lite.

And TF lite is kind of independent of TF1 vs TF2, it is just its own version.

I think it will still take a long time before we start having a serious binary CUDA kernel for TF, so I don't think it's worth all the extra effort now.

The main "product" for users is the tflite part.
Let's say a user wants use use our tflite compute engine:

• They want to use one of our models: no problem, everything is TF 2
• They want to use their own models (made with Larq obviously):
  • They use TF 2: great
  • They use TF 1 and don't want to upgrade: they can easily keep doing their training in TF 1. As long as they can save the trained model as a keras file and load it back into TF 2, we can convert it to tflite and everything is great again.
  • They use TF 1 and their trained saved model can not be loaded in TF2: This would be a problem but I think it would be very rare. @lgeiger Do you think this can happen?

Since this last case is so rare (I think), I vote to drop TF2 support in the compute engine.

[lgeiger]

They use TF 1 and their trained saved model can not be loaded in TF2: This would be a problem but I think it would be very rare. @lgeiger Do you think this can happen?

I don't know.

I vote to drop TF2 support in the compute engine.

I'd always vote to drop support for anything that make maintenance difficult. Would upgrading the TF 1.x tests to TF 1.15.0 solve our issues, since it uses the same docker container?

[Tombana]

I'd always vote to drop support for anything that make maintenance difficult. Would upgrading the TF 1.x tests to TF 1.15.0 solve our issues, since it uses the same docker container?

I'm not sure what the current issues are. From your comment I understood that that tf 1.x needs older python versions? Does TF 1.15 work with newer python versions?

[lgeiger]

From your comment I understood that that tf 1.x needs older python versions?

Just the default custom-op container for 1.13 and 1.14 is different and doesn't include a modern version of Python. We run TF 1 with Python 3.7 in larq. If I recall correctly with 1.15 they switched to the tensorflow:custom-op-ubuntu16 image TF 2 uses as well, so it would simplify our build pipeline.

[lgeiger]

I switched the test runner from tf.test to pytest in b3cedd3 which can be still be run via bazel. But feel free to close this PR if we should just stick to what TF is doing and use tf.test.TestCase and absl.testing.parameterized.

[Tombana]

Just the default custom-op container for 1.13 and 1.14 is different and doesn't include a modern version of Python. We run TF 1 with Python 3.7 in larq. If I recall correctly with 1.15 they switched to the tensorflow:custom-op-ubuntu16 image TF 2 uses as well, so it would simplify our build pipeline.

Then we should definitely switch to tf 1.15 with the new container.
The configure.sh script should be updated then. Currently it asks if the tensorflow package is manylinux2010-compatible. (1.14 is not manylinux2010-compatible, 1.15 and 2.0 are).
This question was then used to determine if we should install 1.14 or 2.0.
If we switch to 1.15, then we should just remove that question from the script and always assume manylinux2010-compatible, but replace the question with "Do you want tf 1.15 or tf 2.0".

I think we should do that.

I switched the test runner from tf.test to pytest in b3cedd3 which can be still be run via bazel. But feel free to close this PR if we should just stick to what TF is doing and use tf.test.TestCase and absl.testing.parameterized.

I don't know if there is a good reason to use tf.test.TestCase. Maybe it provides something that pytest doesn't provide, like a Tensorflow session object thats meant for testing or something? (like maybe it automatically clears the tf session for every individual test)

[lgeiger]

If we switch to 1.15, then we should just remove that question from the script and always assume manylinux2010-compatible, but replace the question with "Do you want tf 1.15 or tf 2.0".

Either that or require the user to install the desired TF version beforehand.
See also tensorflow/custom-op#35 (comment) which verifies our assessment.

Maybe it provides something that pytest doesn't provide, like a Tensorflow session object thats meant for testing or something

Yes, it provides Tensor support for self.assert* calls, but nothing vital we couldn't replace with other tooling.

like maybe it automatically clears the tf session for every individual test

We could handle that like we do in the TFLite test with running pytest-xdist plugin.

[Tombana]

Either that or require the user to install the desired TF version beforehand.
See also tensorflow/custom-op#35 (comment) which verifies our assessment.

I like that even more. The user should install the tensorflow version of their choice, this should not be done by the configure script. (This will even speed up the ARM Github actions because they don't require tensorflow to be installed at all).

Yes, it provides Tensor support for self.assert* calls, but nothing vital we couldn't replace with other tooling.

The assert can be replaced by the pytest variants. I think I remember seeing something like with self.TestSession() as sess: or something but I forgot where.

like maybe it automatically clears the tf session for every individual test
We could handle that like we do in the TFLite test with running pytest-xdist plugin.

Sounds good. I'm in favor if this means we can use more recent python tools.

[lgeiger]

I am exploring some ideas in larq/larq#313

[lgeiger]

I just realized that we already use the eval_op so there is no real need to inherit from tf.test.TestCase anyway: e5f1c6e

If we want to run the test in both eager and graph mode, we can reuse the fixtures introduced in larq/larq#313

Keep in mind, that this doesn't switch the CLI command to pytest we will still run it through bazel.

[Tombana]

Just a note: I saw that this --python_top=... bazel commandline argument is deprecated and that in newer bazel versions it is disabled. See for example the "Python" section on this page. They recommend the bazel example at the bottom of this github issue for migrating to the new version.
However, Tensorflow itself does not work with bazel > 0.26 (I tried it), so the docker image that we use will probably keep using bazel 0.26 for a long time. Therefore we don't have to update, but I just wanted to have this written down somewhere.

Another note: it seems we have to increase the python test timeout limit because the unit test is failing. (It makes sense because it is a rather extensive test of all the bconv2d options).
According to (this documentation)[https://docs.bazel.build/versions/master/test-encyclopedia.html] we can do so by labeling the test as "large" instead of "small" in larq_compute_engine/BUILD (search for "bconv2d_tests").

[Tombana]

Looks good. Can be merged after marking bconv2d_tests as "large".