TF2.16 support: hermetic python shim, TF 2.16.2 pin, legacy Keras path, setup/packaging fixes #913
Conversation
mhucka
added
area/dependencies
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thank you for the work, and I'm sorry about the number of changes requested here …
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
Co-authored-by: Michael Hucka <mhucka@google.com>
|
About the version used here (0.7.5): I put 0.7.5 because in the master branch in releases/setup.py: Line 58 in c6a90ac It was 0.7.4 As you mentioned the current version on PyPi is 0.7.3... I could patch releases/setup.py and tensorflow_quantum/init.py to match 0.7.4 (init has 0.7.2): quantum/tensorflow_quantum/__init__.py Line 67 in c6a90ac And maybe this new version could be the tag 0.7.4 right? Or maybe the current version that is in master is the unofficial version 0.7.4 and I should update it to 0.7.5? (to avoid losing 0.7.4 that is in master). |
I know we discussed this offline, but just to follow up here too for future reference: The 0.7.4 in main is intended to be the next version. I'm not sure if this convention is different from what TensorFlow does, but the convention for TFQ has been that For TFQ, since the last release was for 0.7.3, the version was bumped in main to 0.7.4 after the release, and no releases have happened since that one. So the next release is 0.7.4. Based on the PR the version number was bumped to 0.7.4 after the last release, it looks like the version number was updated to 0.7.4 only in At some point in the future, I think we should change the numbering scheme to include |
Options `--cxxopt=-D_GLIBCXX_USE_CXX11_ABI=1` and `--cxxopt=-std=c++17` are written to the `.bazelrc` file by `configure.sh`. Consequently, they are always in effect and don't need to be repeated in the various shell scripts in `scripts/.`
`third_party/python_legacy` is generated by configure.sh and not meant to be committed to git.
Using `set -e` ends up masking failures in command invocations inside `$(...)` constructs.
All the files are autogenerated. To make that clear, it's probably best to put a comment about "# AUTOGENERATED by configure.sh" in all of them and not only in `defs.bzl`.
I've always the phrasing of that question confusing and misleading. (The question is _really_ about GPU vs CPU, but it only asks about CPU and does not say anything about the implications of replying with `n`.) We may as well take this opportunity to try to make it more clear.
The fact that running `configure.sh` has always been another gotcha for people. Since the file contents are being changed substantially, we may as well take this opportunity to add a comment warning users the file will be overwritten.
Some of the variables referenced in the file lacked the curly braces that the style guides recommend.
The use of `readlink` led to getting the pyenv shim, which led to failures during `bazel test` runs. Always asking Python for the path seems to be a more robust strategy.
Running in an environment where the system Python interpreter is not the desired one continued to be a problem in my tests. This updates WORKSPACE to use a slightly newer version of the rules_python library than what is installed by TensorFlow's various workspace files, and in addition, to make it read the `requirements.txt` file.
The Python environment that Bazel sets up for `bazel test` lacked all the dependencies needed. In addition, it's generally considered a good practice to pin the versions of dependencies in order to get more reproducible builds. TensorFlow has been using `pip-compile` and `requirements_lock_X.Y.txt` files for this, where `X.Y` is the Python version. This commit changes the TensorFlow Quantum scheme to be similar: a `requirements.in` lists the essential requirements, and this file is then processed by `pip-compile` to produce the final `requirements.txt` by following the transitive dependencies.
This is necessary for the changes in WORKSPACE to work.
`.gitignore` should not ignore `.txt` files.
The `.tf_configure.bazelrc` file is another one generated at run time and needs to be ignored by git.
After testing different combos on a couple of systems, I found that some of the flags made no difference. (Guess: maybe the Bazel rules don't propagate them down to external dependencies.) This removes those flags as a matter of good practices.
Calling `setup.py` is deprecated these days. The simplest replacement is to use the `build` package.
I must have left a `set -x` in here at some point.
Turns out that `pip show a b c` will return 0 if any of the packages are found. This is not what we needed to happen. Switching instead to simply calling `pip install -qq` is probably as fast as anything else that involves testing and possibly installing a missing dependency.
Turns out that, while `common` does work for both `build` and `test`, it can be problematic for other Bazel commands.
The version 2 of wrapt led to a pip error on at least one platform. Downgrading to 1.x solved it.
Changes: * Adjust the way it extracts the shared library name to account for changes in what's written into the .bazelrc file. * Avoid using `find / ...` in favor of finding out the location of the policy file more directly. * Adjust quotes around values in the sed command. * Use `|` as the sed pattern separator to make the command a little bit more easily readable, because I had some trouble parsing it due to the combination of many forward and backward slashes and quote marks.
3 participants
Summary
Key changes
--python=flag; generate .tf_configure.bazelrc + .bazelrc + the python shim${PYTHON_BIN_PATH:-python3}and ensure setuptools presentTesting
bazel build ... release:build_pip_packagesucceedsNotes