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

[WIP] Build against tf2.14 bazel strategy #2855

Closed

Conversation

seanpmorgan
Copy link
Member

Description

@boring-cyborg boring-cyborg bot added the github label Oct 18, 2023
@bhack
Copy link
Contributor

bhack commented Oct 18, 2023

We have the same issue when I've introduced this in the release PR. It seems that pytest is not installed in the same python env where we are executing tests with bazel.

@trevor-m
Copy link
Contributor

Fixes #2849

@bhack bhack linked an issue Oct 23, 2023 that may be closed by this pull request
@seanpmorgan
Copy link
Member Author

@angerson we (and other SIGs / downstream consumers) have had issues with the the hermetic python bazel rule. Can you or someone in SIG build take a look at this failing test when time allows:

  1. Our workspace install_deps call does not properly install the 3 requirements files that we have specified, since we error on ModuleNotFoundError: No module named 'pytest'
  2. We need to run a py_binary() for configure.py using the hermetic python after the install deps are available instead of using the system python install as is down in the test above.

We tried a few things on the last release... but we want to get ahead of this for upcoming releases when the new workspace strategy may be required (I see that we're still publishing multiple py docker images for future tf releases though).

@georgiyekkert
Copy link
Contributor

@seanpmorgan Why do you pull tensorflow via http_archive and also via pip? You started doing it not so long ago - 1c3a2b2
Looking at the code, you should install it only via pip.

@seanpmorgan
Copy link
Member Author

seanpmorgan commented Oct 25, 2023

@seanpmorgan Why do you pull tensorflow via http_archive and also via pip? You started doing it not so long ago - 1c3a2b2 Looking at the code, you should install it only via pip.

https://github.com/tensorflow/addons/blob/master/WORKSPACE#L18-L32

Because we load the 4 bazel workspace files that are provided since we compile custom CPU and CUDA ops. IIRC when on TF2.9 there was rev in CUDA version and our previous local toolchain broke. We started loading the workspace files from tensorflow to build the same way.

@georgiyekkert
Copy link
Contributor

georgiyekkert commented Oct 25, 2023

@seanpmorgan Why do you pull tensorflow via http_archive and also via pip? You started doing it not so long ago - 1c3a2b2 Looking at the code, you should install it only via pip.

https://github.com/tensorflow/addons/blob/master/WORKSPACE#L18-L32

Because we load the 4 bazel workspace files that are provided since we compile custom CPU and CUDA ops. IIRC when on TF2.9 there was rev in CUDA version and our previous local toolchain broke. We started loading the workspace files from tensorflow to build the same way.

There are 3 ways to fix it:

  1. Remove pulling tensorflow via http_archive - you are bringing a lot more than you need when you load all TF dependencies. Load only what you need. IMO this is the correct way
  2. Since EOL is planned in May 2024, we can patch tensorflow, so it keeps using non-hermetic Python. You can use the same patch as TF-federated - google-parfait/tensorflow-federated@8580528#diff-4d440744ea1e145db1f807bb811e815b7e0526db8cc2dfdbba441ef80ceb9e81. This is the easiest way
  3. Use the hermetic python in your build. Currently you have errors like "ModuleNotFoundError: No module named 'pytest'" because you are not loading "pip" deps correctly, see https://rules-python.readthedocs.io/en/latest/pypi-dependencies.html#using-third-party-packages-as-dependencies. Each "pip" dependency must be specified explicitly in the BUILD file where needed. E.g. in tensorflow https://github.com/tensorflow/tensorflow/blob/a17ef94e5144fb0fe8447d7ae2e1b32f367a584c/tensorflow/python/distribute/failure_handling/BUILD#L50 https://github.com/tensorflow/tensorflow/blob/a17ef94e5144fb0fe8447d7ae2e1b32f367a584c/tensorflow/python/distribute/failure_handling/failure_handling_util.py#L19. Since you are pulling TF via both http_archive and pip, making configure.py work the same way as it is now might be tricky. Also https://github.com/bazelbuild/rules_python/blob/0.26.0/docs/pip.md#pip_parse requires a locked/compiled requirements file

@bhack
Copy link
Contributor

bhack commented Nov 3, 2023

@georgiyekkert Can you send a PR on this repo directly or over this PR's source branch?
Thanks

@seanpmorgan
Copy link
Member Author

#2856

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot build tensorflow addons with TF 2.14
4 participants