-
Notifications
You must be signed in to change notification settings - Fork 123
Bazel rules py #3542
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
base: master
Are you sure you want to change the base?
Bazel rules py #3542
Conversation
|
I need someone to check that this works on ubuntu arm computers as there was something wrong with the pyqt distribution through pip last time we tried something like this. |
Lmh-java
left a comment
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 ran thunderscope on my arm VM and it worked fine (after setup_software). Seems like you have fixed the pyqt mess. Left a few comments.
| host_software_packages+=(pyqt6-dev-tools) | ||
| host_software_packages+=(python3-pyqt6.qtsvg) | ||
|
|
||
| virtualenv_opt_args="--system-site-packages" |
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 am guessing these will contaminate the dependency tree?
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.
These are installations of pyqt6 into the local computer, which we no longer need. I think I need to write an additional line to uninstall an existing version of pyqt6 as well so it doesn't contaminate the tree.
| "QTWEBENGINEPROCESS_PATH": "../.thunderscope_main.venv/lib/python3.12/site-packages/PyQt6/Qt6/libexec/QtWebEngineProcess", | ||
| }, | ||
| package_collisions = "warning", | ||
| 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.
Who is managing this virtual environment? Is it bazel?
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.
Yes, I believe it is Bazel by way of the aspect_rules_py reworking. Basically, the new aspect_rules_py pylibrary/ pybinary rules build the dependencies in a venv, instead of doing whatever Bazel did before, which was less rigorous and safe.
|
I also tested on my arm VM. Set up software and ran thunderscope. Everything looks good to me! |
|
Ok so I'll need to refine the testing a bit further so that I can see exactly what I need to, I'll post a procedure in a sec |
Are you certain that pyqt was uninstalled from the local machine properly? |
| PyOpenGL==3.1.6 | ||
|
|
||
| ruff==0.5.5 | ||
| pyqt-toast-notification==1.3.2 |
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.
pyqt-toast-notification can probably be removed here since it is already in software/thunderscope/requirements.in
| pyqtgraph==0.13.7 | ||
| thefuzz==0.19.0 | ||
| iterfzf==0.5.0.20.0 | ||
| python-Levenshtein==0.25.1 |
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 think python-Levenshtein can be safely removed here, I don't see any usages of Levenshtein in our code
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.
python-Levenshtein is used for thefuzz.
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.
oh interesting, I wonder why it isn't installed as a dependency of thefuzz
| thefuzz==0.19.0 | ||
| iterfzf==0.5.0.20.0 | ||
| python-Levenshtein==0.25.1 | ||
| psutil==5.9.0 |
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.
We can probably remove psutil here since it is in software/thunderscope/requirements.in and we don't need it to run tbots.py
| from PyQt6.QtCore import PYQT_VERSION_STR, QT_VERSION_STR | ||
|
|
||
| print(f"PyQt6 Version: {PYQT_VERSION_STR}") | ||
| print(f"Qt Version (underlying C++ library): {QT_VERSION_STR}") | ||
|
|
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.
It would be better to put these logging statements in a function somewhere (maybe in initialize_application) and avoid running top-level code
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 is true, I did it mostly for testing purposes so that I could assess if the pr was actually doing what it should be. It would be removed in the future, before this gets merged
| env = { | ||
| "LD_LIBRARY_PATH": "../.thunderscope_main.venv/lib/python3.12/site-packages/PyQt6/Qt6/lib", | ||
| "QTWEBENGINEPROCESS_PATH": "../.thunderscope_main.venv/lib/python3.12/site-packages/PyQt6/Qt6/libexec/QtWebEngineProcess", | ||
| }, |
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.
You may want to consider setting these environment variables in Python at runtime so you don't have to copy this for every py_binary and py_test. You'd have to do this before importing PyQt, so maybe at the top of thunderscope.py would be a good place. Something like:
# PyQt6 bundles Qt as native shared libraries and helper binaries that are not
# discoverable when running under Bazel's sandboxed environment. In particular,
# the dynamic linker cannot find Qt's .so files and QtWebEngine cannot locate its
# QtWebEngineProcess helper unless explicitly told where they live.
# Since the aspect_rules_py virtual env lives in the runfiles tree and its
# location is only known at runtime, we derive these paths from $VIRTUAL_ENV
# here and set the required env vars before importing PyQt6.
qt_path = (
Path(os.environ["VIRTUAL_ENV"])
/ "lib"
/ f"python{sys.version_info.major}.{sys.version_info.minor}"
/ "site-packages"
/ "PyQt6"
/ "Qt6"
)
os.environ["LD_LIBRARY_PATH"] = str(qt_path / "lib")
os.environ["QTWEBENGINEPROCESS_PATH"] = str(qt_path / "libexec" / "QtWebEngineProcess")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 seems to work fine as is, without needing to copy paste this to every py_binary and py_test. I think perhaps under the hood aspect_rules is doing the same thing?
| @@ -1,3 +1,4 @@ | |||
| load("@aspect_rules_py//py:defs.bzl", "py_binary", "py_library") | |||
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 you be loading py_binary and py_library from aspect_rules_py in every BUILD file? Some BUILD files don't load them in, so I assume they're using the rules_python versions.
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 think for consistency you are probably correct. I would also assume that if I don't explicitly load it, it uses the rules_python version. I just knew that specifically this was broken, and otherwise I didn't want to touch the other parts that seemed to work fine.

Description
Commit history is cooked again and idk why. This PR switches us over to using a different version of py_binary and py_library in bazel, to fix some of our hermeticity issues and accommodate PyQt in a less cooked manner.
We can probably clean up setup software to remove a few more libraries from the local installation, since they got moved to the hermetic build. I'll look into this further.
https://github.com/aspect-build/rules_py
bazel-contrib/rules_python#508
Testing Done
Thunderscope compiles (finally)
Resolved Issues
Resolves #3533
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue