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

[bazel] Fix lit tests with python 3.11+ #87022

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

keith
Copy link
Member

@keith keith commented Mar 28, 2024

In python3.11 there is a new environment variable PYTHONSAFEPATH which
stops python from setting the current directory as the first entry in
sys.path. Bazel started setting this to ensure that python targets
don't accidentally access things that aren't in their dependency tree.
This resulted in lit tests breaking because sys.path didn't include the
directory to the lit source files. This is fixed by adding the lit
binary to the dependency tree and propagating the import path from it.

Fixes #75963

@keith keith requested a review from rupprecht as a code owner March 28, 2024 23:30
@llvmbot llvmbot added the bazel "Peripheral" support tier build system: utils/bazel label Mar 28, 2024
@keith keith requested a review from aaronmondal March 28, 2024 23:30
@keith keith force-pushed the ks/bazel-fix-lit-tests-with-python-3.11 branch from a953638 to 1bac938 Compare March 28, 2024 23:35
Copy link
Collaborator

@rupprecht rupprecht left a comment

Choose a reason for hiding this comment

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

Excellent! Fixes #75963.

utils/bazel/llvm-project-overlay/llvm/lit_test.bzl Outdated Show resolved Hide resolved
In python3.11 there is a new environment variable PYTHONSAFEPATH which
stops python from setting the current directory as the first entry in
sys.path. Bazel started setting this to ensure that python targets
don't accidentally access things that aren't in their dependency tree.
This resulted in lit tests breaking because sys.path didn't include the
directory to the lit source files. This is fixed by adding the lit
binary to the dependency tree and propagating the import path from it.
@keith
Copy link
Member Author

keith commented Mar 29, 2024

oh cool, thanks for the link! I realized that I don't think we need to accept deps at all in the macro, so I just removed it. None of the ones in the tree right now are so I assume that's fine. I didn't do that originally just because I wasn't sure what kwargs were being passed, but I doubt it makes sense to pass deps to the underlying py_test in general.

@rupprecht
Copy link
Collaborator

I realized that I don't think we need to accept deps at all in the macro, so I just removed it. None of the ones in the tree right now are so I assume that's fine.

I also don't see anything out of tree. Users will pass their own lit configs/tests via data, nobody should need to touch deps. If someone is doing that, and it's legitimate, we can use what you had before. The error message should be obvious in pointing to this -- something along the lines of "you're passing deps twice".

@keith keith merged commit 89bae85 into llvm:main Mar 29, 2024
4 checks passed
@keith keith deleted the ks/bazel-fix-lit-tests-with-python-3.11 branch March 29, 2024 02:07
@fzakaria
Copy link
Contributor

Awesome.

How did you debug this ? I'm always keen to learn debugging workflows from others.

@keith
Copy link
Member Author

keith commented Apr 1, 2024

I actually wish I had found this issue first since that would have helped a lot.

I was lucky enough to hit the failure on my macOS machine, but had a Linux machine where I wasn't hitting it (but I didn't know why yet). From there I threw a print(sys.path) in the first line of the lit.py script where I spotted a difference. I also happened to know that it's been an issue in bazel before that sys.path changes have broken things. From there I found bazelbuild/bazel#9239 which through some cross links eventually lead me to bazelbuild/bazel#15701 once it was clear it was python version related, and that it was in the name of better hermiticity, I just started looking for a fix.

rupprecht pushed a commit that referenced this pull request Apr 2, 2024
This change checks if there is already a `deps` key in `kwargs` and
concatenate it to avoid multiple values for `deps` key argument.

background:
#87022 recently added explicit
`deps` to the lit_test. This is causing StableHLO bazel build failures
at
https://github.com/openxla/stablehlo/actions/runs/8511888283/job/23312383380?pr=2147

Tested: local build run is successful
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lit] Running lit with bazel fails for Python 3.11
4 participants