-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Tensorflow to 2.16.1 #805
Conversation
Uses hermetic Python
Disable read-only cache on PR
@@ -43,8 +43,7 @@ def : Pat<(TFL_Conv2DOp:$conv_output | |||
[(HasOneUse $pad_output), | |||
(NoBatchAndChannelPadding $paddings), | |||
(SamePaddingHeight $paddings, $input, $conv_output, $stride_h), | |||
(SamePaddingWidth $paddings, $input, $conv_output, $stride_w)], | |||
(addBenefit 100)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now an extra argument before the benefit-argument. So we could either insert a [],
here before addBenefit
, or just remove the benefit thing alltogether. @lgeiger do you know if we depend on these benefit values? A lot of them seem arbitrary to me, but some seem to be chosen specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through the patterns again, and I added back one of the addBenefit
s and left all the others out.
The one I added back was in order to give priority to GreaterEqual(x, 0) -> Quantize
pattern over the more generic GreaterEqual(x, threshold) -> Quantize(x - threshold)
pattern. This might avoid a 'subtract 0' operation.
For all other patterns, I don't think the benefit value really mattered: you could first convert a ste.sign(x)
to Quantize(Dequantize(x))
or first convert a Conv to a Bconv; that order does not matter. (Besides, it only applies to patterns within one pass)
Add `TF_PYTHON_VERSION`
Remove `--distinct_host_configuration` flags
This is an attempt to fix the path length build issue
This should get the long path under the length limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for going through all this effort @Tombana 🙏 Looks ready to merge to me (I can't approve since I'm the original author of the PR) 👍 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you so much for upgrading 🙏
echo -e 'build --google_default_credentials' >> .bazelrc.user | ||
fi | ||
|
||
bazelisk build :build_pip_pkg --copt=-fvisibility=hidden --copt=-mavx --linkopt=-dead_strip --distinct_host_configuration=false | ||
bazel-bin/build_pip_pkg artifacts --plat-name macosx_10_14_x86_64 | ||
bazelisk build :build_pip_pkg --config=release_macos_x86 --config=release_cpu_macos --copt=-fvisibility=hidden --linkopt=-dead_strip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still keep --distinct_host_configuration=false
? Though happy to keep it as is if it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That flag was removed in this bazel version :)
I think it would still be nice to have the same effect to avoid a few double builds, but in order to achieve that with the new bazel I think you have to do add some complex toolchain files.
What do these changes do?
Very much a draft, only updated TF itself, CI, and Bazel so far. Still to be done: update the actual C++ code as needed.
The main reason for updating TF is that it will add support for Python 3.12 builds.
TODO
addBenefit
arguments from the TableGen rules. We should check if this doesn't introduce problems.TF_PYTHON_VERSION
environment variable in CIattempt1attempt2The linux jobs are running out of disk space.attempt3attempt4attempt5macOS builds now fixed, linux still out of disk spaceattempt6(used hacky action that removes installed software from github runner in order to maximize disk space, but doesn't work in combination with docker)attempt7Linux builds fixed, by using/mnt
folder of github runner to store bazel outputhttps://github.com/larq/compute-engine/actions/runs/9586972348 Use workspace name
"lce"
on Windows build for shorter paths.It tries to access
C:/build_output/execroot/larq-compute-engine/bazel-out/x64_windows-opt/bin/external/org_tensorflow/tensorflow/compiler/mlir/quantization/tensorflow/calibrator/_objs/calibration_statistics_collector_average_min_max/calibration_statistics_collector_average_min_max.obj.params
which is 274 characters, so we need to remove at least 14 (or 15?) characters from this.C:/build_output
toC:/bzl
workspace(name = "larq_compute_engine")
toworkspace(name = "lce")