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

Use our own Python for specific py_binaries and py_tests #344

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lalten
Copy link
Contributor

@lalten lalten commented Jun 29, 2024

Split off #238

OK so this one is a slightly bigger change. What's happening is that it changes the rules_ros2 machinery to use its own bundled 3.10 interpreter. This way we can ensure that all the pip requirements we depend on will actually work.
The other reason for this change, including the funky python_register_multi_toolchains that registers only a single version, is that this makes it compatible to the Bzlmod way of pinning the interpreter like this.

See also https://rules-python.readthedocs.io/en/latest/toolchains.html#library-modules-with-version-constraints

@mvukov
Copy link
Owner

mvukov commented Jun 30, 2024

OK, I see the big picture here. Please consider this: one writes a python launch file, uses deps for Python != 3.10 (e.g. numpy) and uses that file to launch nodes. What happens is that ros2_launch implicitly depends on Python 3.10 (if the user paid attention to look at which Python rules_ros2 depends on) and the launch app depends on both deps for Python 3.10 and some other Python. This asks for trouble.

This situation may even fail at build time because multi_toolchains go through Bazel transitions -- never tried this TBH.

WDYT?

@lalten
Copy link
Contributor Author

lalten commented Aug 25, 2024

OK, I see the big picture here. Please consider this: one writes a python launch file, uses deps for Python != 3.10 (e.g. numpy) and uses that file to launch nodes. What happens is that ros2_launch implicitly depends on Python 3.10 (if the user paid attention to look at which Python rules_ros2 depends on) and the launch app depends on both deps for Python 3.10 and some other Python. This asks for trouble.

IMO it's better to expose that rather than silently trying to just go with it?

I always thought Humble required exactly Python 3.10, but it looks like REP2000 doesn't really prescribe a specific Python interpreter.

The alternative would be to not pin any Python at all via ofiuco as I suggested in the Bzlmod PR (although it won't actually work fully until ofiuco can expose numpy headers like rules_python can do now)

This situation may even fail at build time because multi_toolchains go through Bazel transitions -- never tried this TBH.

We can cross that bridge when we get there

@lalten lalten mentioned this pull request Aug 29, 2024
@mvukov
Copy link
Owner

mvukov commented Oct 5, 2024

Sorry for being late on this one. Do you need this PR merged or is it a convenience thing?

@lalten
Copy link
Contributor Author

lalten commented Oct 6, 2024

While from Ros side it seems it doesn't really matter what Python version is used from rules_ros2 side there is one kinda difficult complication: We need to depend on numpy headers in

Label("@com_github_mvukov_rules_ros2//ros2:rules_ros2_pip_deps_numpy_headers"),

which come from
whl_filegroup(
name = "numpy_includes",
pattern = "numpy/core/include/numpy",
whl = "@rules_ros2_pip_deps//numpy:whl",
)
cc_library(
name = "rules_ros2_pip_deps_numpy_headers",
hdrs = [":numpy_includes"],
includes = ["numpy_includes/numpy/core/include"],
visibility = ["//visibility:public"],
deps = ["@rules_python//python/cc:current_py_cc_headers"],
)

These headers are generated code:
https://github.com/numpy/numpy/blob/ab5064cc0166d9c112a88d64634c056f8b3f415b/numpy/_core/include/numpy/npy_common.h#L4-L11
and depend on the Python version

So in effect that means that as long as there is a numpy header dependency you must explicitly depend on a specific Python version.

If this wasn't the case it would be great to use ofiuco (Cc @oxidase) as that makes depending on PyPI packages extremely easy for ruleset library things where you do not know the version of Python that will eventually be used.

@mering suggested depending on a well-known list of the Python versions in
#238 (comment) but I would really like to avoid that as that will cause a warning about re-registering toolchains in every single build.

So finally there is the option which I think is the least bad at the moment, which is this PR. It's making all Ros2 dependencies explicitly depend on their own, bundled interpreter. That solves both the numpy header issue and the PyPI dependency resolution issue. The downside is that if you don't happen to use the same interpreter you'll end up with two hermetic Pythons in the runfiles, which will cost some Megabytes for packaged apps.

This is blocking the Bzlmod migration because rules_ros2_pip_deps has to come from somewhere, and it needs to be decided from where.

@lalten
Copy link
Contributor Author

lalten commented Oct 6, 2024

Please consider this: one writes a python launch file, uses deps for Python != 3.10 (e.g. numpy) and uses that file to launch nodes

Regarding this complication, I'm not really not sure what would happen in practice. Probably should try that out. Worst case you could probably add the launchfile as data dep and launch a subprocess to have the right ™️ Python version run it. That would be an awful user experience.

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

Successfully merging this pull request may close these issues.

2 participants