-
Notifications
You must be signed in to change notification settings - Fork 1.7k
infra: remove dependency on org_tensorflow
#5528
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
Conversation
tensorboard/data/server/descriptor.bin out of date, also updated license dates for all packages.
org_tensorfloworg_tensorflow
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.
Thank you very much for taking this on, it's been something I've meant to do for ages but never quite managed to do! And my apologies for the long wait on a review, I wanted to have the time to test it out properly.
The only larger thing is that I think we might be able to reduce the need for gRPC-deps somewhat; otherwise it pretty much LGTM.
|
|
||
| load("@com_github_grpc_grpc//bazel:grpc_deps.bzl", "grpc_deps") | ||
|
|
||
| grpc_deps() |
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.
So without TF, it looks like grpc is requiring that we have python headers installed in order to build - when I checked out this PR and try to build it, I initially got this:
ERROR: /usr/local/google/home/nickfelt/tb/WORKSPACE:129:10: //external:python_headers depends on @local_config_python//:python_headers in repository @local_config_python which failed to fetch. no such package '@local_config_python//': Python Configuration Error: Unable to find Python headers for /usr/bin/python2
Traceback (most recent call last):
File "<string>", line 1, in <module>
AssertionError: /usr/include/python2.7/Python.h does not exist.
Are Python headers installed? Try installing python-dev or python3-dev on Debian-based systems. Try python-devel or python3-devel on Redhat-based systems.
On my workstation I already have python3-dev installed, and I was able to sudo apt-get install python2-dev, which resolved the issue. But that's still a bit of a regression from before when we didn't need python headers installed at all. As far as I could tell, it boils down to our python toolchain configuration - grpc uses a slightly different python_configure() rule from TensorFlow and I guess it's somehow stricter.
(As for the root issue, I looked around and found grpc/grpc#24665, which seems related. It sounds like there's some ambiguity in general about how the grpc header dependency should work.)
Anyway - I guess this is fine, but let's add a mention about the need to have python-dev or python2-dev and python3-dev installed to DEVELOPMENT.md?
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 don't think I have python{1,2,3}-dev or python2 on my workstation, and my python version is 3.9.9. Will investigate the issue further.
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.
Hmm, that's a bit odd. My python is also 3.9.9. I guess there must be some configuration difference?
Maybe we should check w/ one other person just in case, but if nobody else is seeing this error, then I'm fine just proceeding and not mentioning it in DEVELOPMENT.md since the error is at least pretty self-explanatory about what to do to fix it.
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 was able to reproduce the same error after installing Python2, related issue: grpc/grpc#21963.
`rules_apple` (bazelbuild/bazel#13811 (comment)), swift and upb deps (grpc/grpc#25337 (comment)) are included in the the v1.42.0 release of gRPC and are removed.
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.
🎉
|
|
||
| load("@com_github_grpc_grpc//bazel:grpc_deps.bzl", "grpc_deps") | ||
|
|
||
| grpc_deps() |
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.
Hmm, that's a bit odd. My python is also 3.9.9. I guess there must be some configuration difference?
Maybe we should check w/ one other person just in case, but if nobody else is seeing this error, then I'm fine just proceeding and not mentioning it in DEVELOPMENT.md since the error is at least pretty self-explanatory about what to do to fix it.
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.
This looks great. Thank you! 🎉
Thanks for the thorough reviews and investigation! |
* remove dependency on `org_tensorflow` and add necessary dependencies * update rules_rust dep to HEAD and upgrade to Rust 1.58.1 * set mininum bazel version to 4.0.0
* remove dependency on `org_tensorflow` and add necessary dependencies * update rules_rust dep to HEAD and upgrade to Rust 1.58.1 * set mininum bazel version to 4.0.0
This PR removes
org_tensorflowdependency and addsrules_appleto fix the build with bazel 5.0.0+.//tensorboard/data/server:update_protosand updated the packages.rules_rustdep to HEAD (d5ab4143245af8b33d1947813d411a6cae838409).4.0.0for json feature support.Note that
python-dev,python2-devorpython3-devis required if Python2 is installed, see grpc/grpc#24665 for details.Tested internally: cl/424877602
Fixes #5513