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 …
| EXTRA_FLAGS="$2" | ||
|
|
||
| if [[ -z ${DEST} ]]; then | ||
| if [[ -z "$DEST" ]]; then |
There was a problem hiding this comment.
Here and elsewhere that variables are referenced, could we keep the recommended Google style for shell scripts? TensorFlow's general code style guidance points to this, and we also use it in Quantum AI's software in the Quantumlib repositories. For shell variable references like the one above, the section on variable expansion is relevant. The preference is for brace-delimiting variables: "${DEST}". If there is not a technical reason for the change away from using braces like was done in the original script code, could you please put them back? (Not just this variable, but the others as well.)
(The addition of quote marks is a different matter, and that's a change worth keeping.)
| # Order: explicit env -> 3.11 -> python3 | ||
| PY="${PYTHON_BIN_PATH:-}" | ||
| if [[ -z "$PY" ]]; then | ||
| if command -v python3.11 >/dev/null 2>&1; then |
There was a problem hiding this comment.
The minimum Python version for TFQ is 3.10 (although looking now, I can't find anywhere where that is defined or enforced – I'll open an issue), so defaulting to 3.11 would be a conflicting requirement. I checked TensorFlow 2.16 and the minimum there is 3.9, but that's too low for Cirq, so 3.10 should be the minimum.
Regarding the logic in this block of code, the order be as follows:
- If PYTHON_BIN_PATH is set, use that and set PY to that (as the code does now).
- Else, test the version of
python3. If it is less than 3.10, error out. - Else, set PY to
python3and use it.
| echo "$(date) : === Using tmpdir: $TMPDIR" | ||
| echo "=== Copy TFQ files" | ||
|
|
||
| # Copy over files necessary to run setup.py |
There was a problem hiding this comment.
This comment and the next one in this file were not carried over. If nothing has changed in the new script that makes these comments obsolete, it would be better to keep them. They add value by orienting readers and explaining the reasons for what the script is doing.
| "$PY" - <<'PY' || { "$PY" -m pip install --upgrade pip setuptools wheel; } | ||
| import importlib | ||
| for m in ["setuptools","wheel"]: | ||
| importlib.import_module(m) | ||
| PY |
There was a problem hiding this comment.
This is an unconventional way of testing that certain packages have been installed. Would testing the return value of pip show work here? For example, something like
if ! "$PY" -m pip show -q setuptools wheel > /dev/null 2>&1; then
"$PY" -m pip install --upgrade pip setuptools wheel
fi| return True | ||
|
|
||
|
|
||
| nightly = False |
There was a problem hiding this comment.
Although the original code was this way (i.e., not following the style guide), I'm inclined to recommend going ahead and changing the names to conform to the style. It's a fairly small change and should be done at some point anyway. So, nightly → NIGHTLY and so on for the other names flagged by the linter.
| write_to_bazelrc "build --action_env $1=\"$2\"" | ||
| } | ||
| # --- helpers --------------------------------------------------------------- | ||
| write_bazelrc() { echo "$1" >> .bazelrc; } |
There was a problem hiding this comment.
Although I personally don't mind short functions as one-liners, we had better follow the Google style guide and change these back to multiline function definitions.
| elif [[ -n "${CONDA_PREFIX:-}" && -x "${CONDA_PREFIX}/bin/python" ]]; then | ||
| PY="${CONDA_PREFIX}/bin/python" | ||
| elif command -v python3.11 >/dev/null 2>&1; then | ||
| PY="$(command -v python3.11)" | ||
| elif command -v python3 >/dev/null 2>&1; then | ||
| PY="$(command -v python3)" |
There was a problem hiding this comment.
This should mirror the logic discussed for the build script above. In other words, use the user-provided value, else try the conda path, else use python3 (checking the minimum).
| PY_ABS="$("$PY" - <<'PY' | ||
| import os,sys | ||
| print(os.path.abspath(sys.executable)) | ||
| PY |
There was a problem hiding this comment.
This kind of thing does not need a heredoc. It can be done with the -c option to Python:
PY_ABS="$("$PY" -c 'import os, sys; print(os.path.abspath(sys.executable))')"| done | ||
|
|
||
| # For TF >= 2.1 this value isn’t actually consulted by TFQ, | ||
| # but we keep a compatible prompt/flag. |
There was a problem hiding this comment.
I'm sorry to be dense, but I don't actually understand what this is about. A compatible prompt/flag for what, exactly?
| load("@org_tensorflow//tensorflow:workspace3.bzl", "tf_workspace3"); tf_workspace3() | ||
| load("@org_tensorflow//tensorflow:workspace2.bzl", "tf_workspace2"); tf_workspace2() | ||
| load("@org_tensorflow//tensorflow:workspace1.bzl", "tf_workspace1"); tf_workspace1() | ||
| load("@org_tensorflow//tensorflow:workspace0.bzl", "tf_workspace0"); tf_workspace0() |
There was a problem hiding this comment.
It's best to limit each PR to a specific purpose and avoid unrelated formatting changes. Also, although the TFQ contributor guidelines don't document this, we should follow the Bazel style generated by buildifier. That style uses the format of separate lines that was present in the original WORKSPACE file.
So, could you please undo the unrelated formatting changes?
2 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