Skip to content
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

infra: remove dependency on org_tensorflow #5528

Merged
merged 15 commits into from
Feb 3, 2022
Merged

Conversation

yatbear
Copy link
Member

@yatbear yatbear commented Jan 24, 2022

This PR removes org_tensorflow dependency and adds rules_apple to fix the build with bazel 5.0.0+.

  • Ran //tensorboard/data/server:update_protos and updated the packages.
  • Upgraded rules_rust dep to HEAD (d5ab4143245af8b33d1947813d411a6cae838409).
  • Upgraded Rust to 1.58.1 (release note).
  • Set minimum Bazel version to 4.0.0 for json feature support.

Note that python-dev, python2-dev or python3-dev is required if Python2 is installed, see grpc/grpc#24665 for details.

Tested internally: cl/424877602

Fixes #5513

@yatbear yatbear requested a review from nfelt January 25, 2022 16:04
@yatbear yatbear added the dependencies Pull requests that update a dependency file label Jan 25, 2022
@yatbear yatbear changed the title chore: remove dependency on org_tensorflow infra: remove dependency on org_tensorflow Jan 27, 2022
Copy link
Contributor

@nfelt nfelt left a 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.

WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
"https://github.com/grpc/grpc/archive/2d8546a3c4ed8fa0a801770e58fd284cf4a70d7a.tar.gz",
],
)

load("@com_github_grpc_grpc//bazel:grpc_deps.bzl", "grpc_deps")

grpc_deps()
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

WORKSPACE Outdated Show resolved Hide resolved
"https://github.com/grpc/grpc/archive/2d8546a3c4ed8fa0a801770e58fd284cf4a70d7a.tar.gz",
],
)

load("@com_github_grpc_grpc//bazel:grpc_deps.bzl", "grpc_deps")

grpc_deps()
Copy link
Contributor

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.

WORKSPACE Show resolved Hide resolved
@yatbear yatbear requested a review from nfelt February 2, 2022 21:30
Copy link
Contributor

@nfelt nfelt left a 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! 🎉

@yatbear
Copy link
Member Author

yatbear commented Feb 3, 2022

This looks great. Thank you! 🎉

Thanks for the thorough reviews and investigation!

@yatbear yatbear merged commit 595dd94 into tensorflow:master Feb 3, 2022
@yatbear yatbear deleted the bazel5 branch February 3, 2022 16:58
yatbear added a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
* 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
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support building tensorboard with bazel 5.0.0+.
2 participants