Add gtest as c++ testsuite #21
Conversation
|
I managed to configure bazel, run the CC tests with bazel, build and install the package with bazel on the azure machine but for some reason when I run the pytest with bazel it can not find the tensorlfow package!!!! doing exact same steps on my local container works!! |
it turned out the problem is due to a bug in bazel: bazelbuild/bazel#4815 |
| @@ -8,19 +8,25 @@ jobs: | |||
| - job: "Test" | |||
| pool: | |||
| vmImage: "ubuntu-latest" | |||
| container: tensorflow/tensorflow:custom-op-ubuntu16 | |||
| container: tensorflow/tensorflow:custom-op-ubuntu14 | |||
There was a problem hiding this comment.
Why this change? I think ubuntu14 doesn't ship with Python 3.6 by default, so this might lead to problems.
There was a problem hiding this comment.
Because tensorflow is built with gcc 4.8 which is present in the ubuntu14 container. If you build the kernels with gcc 5.x (present in ubuntu16) and try to link with the tensorflow library then there are issues because the ABI (for example the meaning of different processor registers like which one gets the return value of a function) changed between those versions of gcc. You can get in-explainable crashes, so therefore I'd prefer to stick with gcc 4.8 for now. One such crash has already happened when I tried to implement the sign operation.
The issue is known by the tensorflow team:
tensorflow/tensorflow#29643
There was a problem hiding this comment.
registering ops in tensorflow has issues with ubuntu16: tensorflow/tensorflow#29643
thats why we need to use ubuntu14 until they get fixed and as you mentioned ubuntu14 is not shiped with python 3.6 so downgraded our python requirements to python 3.4 for now (see below)
There was a problem hiding this comment.
Once the tensorflow pip package is updated and built with gcc 5.x then we can switch back to the ubuntu16 container.
| @@ -13,6 +13,10 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| # ============================================================================== | |||
| shopt -s expand_aliases | |||
| alias pip='pip3' | |||
| alias python='python3' | |||
There was a problem hiding this comment.
I either need to replace pip everywhere with pip3 or just alias pip with pip3. aliasing is cleaner IMO
| displayName: "Build PIP Package" | ||
|
|
||
| - script: pip3 install artifacts/*.whl --user | ||
| displayName: "Install PIP Package" | ||
|
|
||
| - script: python3 -m pytest . | ||
| displayName: "Run unit tests" | ||
| - script: bazel test larq_compute_engine:py_tests --python_top=//larq_compute_engine:pyruntime |
There was a problem hiding this comment.
Is pytest automatically installed by bazel?
There was a problem hiding this comment.
to be honest, I am not sure how bazel works under the hood but this is the suggested workflow in bazel for python testing and it works without to need to install pytest :)
| name = "pyruntime", | ||
| interpreter_path = select({ | ||
| # Update paths as appropriate for your system. | ||
| "@bazel_tools//tools/python:PY2": "/usr/bin/python2", |
There was a problem hiding this comment.
I wouldn't test against Python 2.
| "@bazel_tools//tools/python:PY2": "/usr/bin/python2", |
There was a problem hiding this comment.
this is not testing against python 2. this just tell bazel where to look for python 2 if any build rules requires python 2
| interpreter_path = select({ | ||
| # Update paths as appropriate for your system. | ||
| "@bazel_tools//tools/python:PY2": "/usr/bin/python2", | ||
| "@bazel_tools//tools/python:PY3": "/usr/bin/python3", |
There was a problem hiding this comment.
Would the following work? /usr/bin/python3 would link to the wrong Python version for me locally.
| "@bazel_tools//tools/python:PY3": "/usr/bin/python3", | |
| "@bazel_tools//tools/python:PY3": "python3", |
There was a problem hiding this comment.
this should be the absolute path to the python binary which bazel is supposed to use. bc of the bug in bazel it can not find the right python and as a workaround i have to hardcode it here for now (see bazelbuild/bazel#4815 (comment))
There was a problem hiding this comment.
How about /usr/bin/env python3, just like the shebang in scripts. That should be more compatible. But maybe bazel doesn't support that because then its no longer a path to a file but a command that it has to run.
| @@ -13,7 +13,7 @@ def has_ext_modules(self): | |||
| setup( | |||
| name="larq-compute-engine", | |||
| version="0.0.1", | |||
| python_requires=">=3.6", | |||
| python_requires=">=3.4", | |||
There was a problem hiding this comment.
Switching to Ubuntu 16 container, shouldn't require this change.
| python_requires=">=3.4", | |
| python_requires=">=3.6", |
There was a problem hiding this comment.
see my comments about ununtu14 container above
fixes #3