feat(bzlmod): support cross-platform whl setups within the main hub repo #1837

aignas · 2024-04-08T14:29:37Z

With this change we add support for platform-specific wheel registration and
doing the selection of which wheel is used at build time.

This supports:

Different package versions for different platforms.
Use string_flags to configure what to fetch/select:
- only whls, only sdist or auto mode.
- libc version and musl vs glibc selection.
- universal2 vs arch wheels for mac.
- target osx version selection.

Summary of changes:

The uv pip install would only warn the user of yanked packages but
would not refuse to install them. Update our implementation to better
match the same behaviour.
A previous PR has added the support for passing it in the
requirements_by_platform and this just add the necessary code to make
sure that we can also do the dependency management when parsing the
whl METADATA files.
Only configure dev_pip deps for linux and osx platforms to not raise
issues later.
Add a function for generating a whl_library name from a filename.
Add a macro for generating all config settings for a particular set of parameters.
Update render_pkg_aliases to also use those config settings.
Update the toolchain selection target_settings to use the py_linux_libc
config setting. With this the user can register a musl linux toolchain if
needed. We can also use similar flag_values to resolve Consider supporting armv7-unknown-linux-gnueabi/hf toolchain #1876.
Integrate everything in the pip extension and setup cross-platform
select statements.

Work towards #1357, #260

Stacked on #1937

aignas · 2024-04-14T14:32:06Z

I think I've identified at least multiple changes that can be separate PRs:

refactor groups to be created inside the hub repo. feat(pip_parse): support referencing dependencies to packages via hub #1856.
add tests for the index downloader and add a flag to control wether the downloading is happening in parallel. I've noticed it to be much easier to debug. feat(bzlmod): add a flag to control if the parallel downloading is used #1854.
use skylib for selects in the alias repo. refactor: use bazel-skylib for creating hub repo aliases #1855.
Add multi platform requirement support with all of the available wheels registered.

aignas · 2024-05-18T05:18:43Z

This now needs to be reimplemented/rebased on #1875 and #1885.

aignas · 2024-05-31T05:28:53Z

@dougthor42, if you have time, I'd be curious if you could try this PR, It may have the same issue as #1917, but if does not, then it would be a really interesting data point.

dougthor42 · 2024-06-01T03:45:04Z

A different error than previously seen. This one is:

ERROR: Traceback (most recent call last):
        File "/usr/local/google/home/dthor/.cache/bazel/_bazel_dthor/dbe74c4144b5c9a438d84a119652bef9/external/rules_python~/python/private/bzlmod/pip.bzl", line 481, column 30, in _pip_impl
                _create_whl_repos(module_ctx, pip_attr, hub_whl_map, whl_overrides, hub_group_map, simpleapi_cache)
        File "/usr/local/google/home/dthor/.cache/bazel/_bazel_dthor/dbe74c4144b5c9a438d84a119652bef9/external/rules_python~/python/private/bzlmod/pip.bzl", line 276, column 32, in _create_whl_repos
                whl_library(name = repo_name, **dict(sorted(whl_library_args.items())))
Error in repository_rule: invalid user-provided repo name 'pypi_311__REDACTED_0_1_0+7_g9c9466d_py3_none_any': valid names may contain only A-Z, a-z, 0-9, '-', '_', '.', and must start with a letter
ERROR: Analysis of target '//src/pyle/dataking/system_optimization/snake_optimizer:snake_metrics_test' failed; build aborted: error evaluating module extension pip in @@rules_python~//python/extensions:pip.bzl

Some of our internal packages include the short git commit hash in their wheel name.

aignas · 2024-06-01T05:15:44Z

This still needs an updated git commit message and more docs, but the functionality is there and is covered by tests, so it should be ready to test/review.

I'll add more docs later.

Sorry for the big PR, but I am not sure if it is possible to split it without loosing the context that is happening all over the pip.parse extension.

This should be portable to the WORKSPACE world, but I would like to leave this out of scope for now.

aignas · 2024-06-02T15:52:24Z

Right now the only tests that fail are on Windows and they fail with a weird:

(15:03:02) WARNING: C:/b/bk-windows-nznf/bazel/rules-python-python/docs/BUILD.bazel:47:6: target '//docs:requirements_parser_bzl' is deprecated: Use //python/pip_install:pip_repository_bzl instead; Both the requirements parser and targets under //docs are internal
(15:03:14) ERROR: C:/b/bk-windows-nznf/bazel/rules-python-python/tests/config_settings/transition/BUILD.bazel:6:25: in _transition_py_binary rule //tests/config_settings/transition:test_py_binary_with_transition_subject:
Traceback (most recent call last):
	File "C:/b/bk-windows-nznf/bazel/rules-python-python/python/config_settings/transition.bzl", line 78, column 13, in _transition_py_impl
		fail("target {} does not have rules_python PyRuntimeInfo or builtin PyRuntimeInfo".format(target))
Error in fail: target <target //tests/config_settings/transition:_test_py_binary_with_transition_subject> does not have rules_python PyRuntimeInfo or builtin PyRuntimeInfo
(15:03:14) ERROR: C:/b/bk-windows-nznf/bazel/rules-python-python/tests/config_settings/transition/BUILD.bazel:6:25: in _transition_py_test rule //tests/config_settings/transition:test_py_test_with_transition_subject:
Traceback (most recent call last):
	File "C:/b/bk-windows-nznf/bazel/rules-python-python/python/config_settings/transition.bzl", line 78, column 13, in _transition_py_impl
		fail("target {} does not have rules_python PyRuntimeInfo or builtin PyRuntimeInfo".format(target))

I'll finish here for the day. I might split out the last commit out as a separate PR since that is a small distinct change that is related to the big multi-platform change, but we can land it separately.

…lled With this PR we can finally have fewer lock file entries in setups which do not use the `experimental_index_url` feature yet. This is fully backwards compatible change as it relies on `bazel` doing the right thing and regenerating the lock file. Fixes bazelbuild#1643.

aignas · 2024-06-02T16:04:22Z

Done, split out the lock-file related work into #1937, so once that one is merged, the diff here will shrink a tiny bit, but not by much.

…yPI by default

The `uv pip install` would only warn the user of yanked packages but would not refuse to install them. Update our implementation to better match the same behaviour.

A previous PR has added the support for passing it in the `requirements_by_platform` and this just add the necessary code to make sure that we can also do the dependency management when parsing the `whl` `METADATA` files.

…ndows This fixes a small issue with our dependency configuration for the documentation building. It is missing `colorama` and probably a few other dependencies to work on windows, but I am not sure if the other `sphinx` machinery supports non UNIX.

With this we can ensure that the name is unique (we use `sha256` and prefix it with `pip_hub` repo), valid (we use `sha256` instead of the version or the build tag of the whl filename) and easy to understand (we add the python tag, abi tag and platform tags to the end of the name to ensure that users have an easy way to navigate the files).

This also changes `is_python_config_setting` so that it can accept extra `flag_values` so that we can reuse it for the cases where we want something that is more specific than `is_python_config_setting` with a `python_version` alone.

…ames This code will use the config settings generated by `pip_config_settings` in all cases and when the `whl_alias` instances are generated by passing `filename` values, then we will also use the platform-specific config settings from the `//_config` package in the hub repo.

With this we start using the `py_linux_libc` flag value in toolchains as well and ensure that no hermetic toolchain will be matched when we specify to pull the `musl` wheels. In that case the user will need to supply their own toolchain that is the expected behaviour. However, I am not sure how we could fail due to no available toolchain in that case because the autodetecting toolchain will just use the host interpreter.

This feature leverages all of the foundational work from previous commits and updates the tests and documentation.

aignas · 2024-06-03T04:57:03Z

Merged the main since I saw some auto-detecting toolchain changelog present, so just to make sure that I am not missing out on anything.

aignas · 2024-06-03T23:56:58Z

tests/private/pip_repo_name/pip_repo_name_tests.bzl

+
+def _test_simple(env):
+    got = pip_repo_name("prefix", "foo-1.2.3-py3-none-any.whl", "deadbeef")
+    env.expect.that_str(got).equals("prefix_foo_py3_none_any_deadbeef")


Using sha is good enough for the first pass, but PR welcome for including the version (and optionally the build tag) in the repo name.

We will need to keep this experimental until we settle on the names. Or at least remember to update if we change it and it breaks for users.

aignas · 2024-06-04T00:10:46Z

python/private/pip_config_settings.bzl

+    }
+)
+
+def pip_config_settings(


An idea from rules_pycross: https://github.com/jvolkman/rules_pycross/blob/531c8d21ff859e8b6df7f267eef066150723ec04/pycross/private/interpreter_version.bzl

This could be a good alternative to the current is_python_config_setting implementation as it would reduce the number of flags that we generate.

python/private/toolchains_repo.bzl

aignas · 2024-06-04T02:35:40Z

tests/config_settings/transition/multi_version_tests.bzl

+#
+# If the toolchain is not resolved then you will have a weird message telling
+# you that your transition target does not have a PyRuntime provider, which is
+# caused by there not being a toolchain detected for the target.


The tests were working previously because even though the test is setting a specific python_version via a transition, the toolchain that we were getting was the default version. So in this case the actual version would be returned would be different from the one requested in the transition.

This could explain some of the issues we've seen recently about people saying that the toolchain is mismatching their configuration as it could happen if they do not configure the default toolchain version in their MODULE.bazel file.

I wonder if this should be mentioned in the CHANGELOG.md. I'll do it for the sake of posterity.

dougthor42 · 2024-06-04T02:52:01Z

Still failing on the foobar package that has a md5 for both py311 and py312 🫤

It's a different failure, though, so that's good!

$ bazel clean; bazel build //...
INFO: Invocation ID: cefb7077-a468-4283-b711-06897aff5e1d
INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.
INFO: Invocation ID: 84fe841f-1ab2-4990-9261-165044c971ac
ERROR: /usr/local/google/home/dthor/.cache/bazel/_bazel_dthor/dbe74c4144b5c9a438d84a119652bef9/external/rules_python~~pip~pypi/_config/BUILD.bazel: no such target '@@rules_python~~pip~pypi//_config:is_py311_none_manylinux_x86_64': target 'is_py311_none_manylinux_x86_64' not declared in package '_config' defined by /usr/local/google/home/dthor/.cache/bazel/_bazel_dthor/dbe74c4144b5c9a438d84a119652bef9/external/rules_python~~pip~pypi/_config/BUILD.bazel (did you mean is_py3_none_manylinux_x86_64, _is_py3_none_manylinux_x86_64, is_py_none_manylinux_x86_64, _is_py_none_manylinux_x86_64, is_cp3x_none_manylinux_x86_64, or is_py3_none_any_linux_x86_64?)
ERROR: /usr/local/google/home/dthor/.cache/bazel/_bazel_dthor/dbe74c4144b5c9a438d84a119652bef9/external/rules_python~~pip~pypi/foobar/BUILD.bazel:53:6: errors encountered resolving select() keys for @@rules_python~~pip~pypi//foobar:whl
Use --verbose_failures to see the command lines of failed build steps.
ERROR: Analysis of target '//:modules_map' failed; build aborted: Analysis failed
INFO: Elapsed time: 1.091s, Critical Path: 0.63s
INFO: 18 processes: 18 internal.
ERROR: Build did NOT complete successfully

… all whls

…t filenames

…rom dist filenames

aignas · 2024-06-04T05:24:37Z

@dougthor42, sorry for that one, now should be fixed

dougthor42 · 2024-06-04T14:06:17Z

Yup! 445337e worked for me 👍

I noticed that the runfiles directory now has more info in the path name:

the wheel name
the head of the wheel sha

-test.runfiles/rules_python~~pip~pypi_311_foobar/
+test.runfiles/rules_python~~pip~pypi_311_foobar_py311_none_manylinux_2_17_x86_64_b7d2f441/

These are great changes! Super useful. Should such a change be included in the changelog?

groodt

LGTM

As mentioned at the maintainers review, we might need to keep exploring alternatives for the explosion of config flags, but I don't think it needs to be blocking while we iterate.

aignas · 2024-06-05T23:28:28Z

@groodt, thanks for the stamp, could you also stamp #1937 please?

aignas · 2024-06-14T05:48:00Z

python/private/py_toolchain_suite.bzl

-    elif set_python_version_constraint == "False":
-        target_settings = []
+        target_settings = [name]


The absence of this branch is the root cause of #1967.

…ames Initially the repo name generate was very conservative and we had sha256 as the endings of the repo names. With this change we have more human readable repository names. In order to still comply with the bazel label rules, we are normalizing the versions to only have `_` and `.` characters. This is an alternative fix to the error message seen in bazelbuild#1837 (comment)

aignas force-pushed the feat/multi-platform-whls branch 2 times, most recently from 6c19b7b to 6ad9f4a Compare May 30, 2024 05:36

aignas self-assigned this May 30, 2024

aignas mentioned this pull request Jun 1, 2024

pip.parse(python_version=) should not be mandatory #1708

Open

aignas mentioned this pull request Jun 1, 2024

refactor/fix: store dists in parse_requirements output #1917

Merged

aignas force-pushed the feat/multi-platform-whls branch from a28c081 to 1c87068 Compare June 1, 2024 05:04

aignas marked this pull request as ready for review June 1, 2024 05:13

aignas requested a review from rickeylev as a code owner June 1, 2024 05:13

aignas changed the title ~~feat(bzlmod): support multi-platform wheel hubs [WIP]~~ feat(bzlmod): support cross-platform whl setups within the main hub repo Jun 1, 2024

aignas requested a review from f0rmiga as a code owner June 2, 2024 10:57

aignas force-pushed the feat/multi-platform-whls branch from 411ac31 to da00b0a Compare June 2, 2024 15:53

aignas added 9 commits June 3, 2024 12:50

comment: address comments and set the internal pip extension to use P…

3fd6821

…yPI by default

fix(bzlmod): do not throw away yanked PyPI packages

4c4b46f

The `uv pip install` would only warn the user of yanked packages but would not refuse to install them. Update our implementation to better match the same behaviour.

fix(whl_library): add arm to the list of known architectures

0587c37

A previous PR has added the support for passing it in the `requirements_by_platform` and this just add the necessary code to make sure that we can also do the dependency management when parsing the `whl` `METADATA` files.

feat(bzlmod): fetch and configure platform specific wheels

bf4c35c

This feature leverages all of the foundational work from previous commits and updates the tests and documentation.

aignas force-pushed the feat/multi-platform-whls branch from da00b0a to bf4c35c Compare June 3, 2024 04:31

Merge branch 'main' into feat/multi-platform-whls

4e2d7df

aignas commented Jun 3, 2024

View reviewed changes

aignas commented Jun 4, 2024

View reviewed changes

python/private/toolchains_repo.bzl Show resolved Hide resolved

fixup! feat(toolchain): use py_linux_libc flag value

8a6bda5

aignas commented Jun 4, 2024

View reviewed changes

fixup! fixup! feat(toolchain): use py_linux_libc flag value

319409d

aignas added 4 commits June 4, 2024 13:51

fixup! feat(internal): add a function to generate config settings for…

65d8278

… all whls

fixup! feat(internal): add a function to generate config settings for…

bc1cdef

… all whls

fixup! feat(internal): support inferring the config settings from dis…

6cdb534

…t filenames

fixup! fixup! feat(internal): support inferring the config settings f…

445337e

…rom dist filenames

groodt approved these changes Jun 5, 2024

View reviewed changes

aignas added 3 commits June 6, 2024 16:49

Merge branch 'main' into feat/multi-platform-whls

57d7a3f

doc: add a not to CHANGELOG on the naming of the repos

495758c

test: add an additional test to ensure that the aliases are the same

6dde587

aignas mentioned this pull request Jun 6, 2024

fix(pip.parse): make the pip extension reproducible if PyPI is not called #1937

Merged

aignas added this pull request to the merge queue Jun 6, 2024

Merged via the queue into bazelbuild:main with commit 7de43d1 Jun 6, 2024
4 checks passed

This was referenced Jun 6, 2024

Downloaded python toolchain has shared library dependencies #1211

Closed

pip.parse crashes with StopIteration if platform restriction prevents package from install #1868

Closed

aignas mentioned this pull request Jun 14, 2024

(bzlmod): users have no way to opt out of hermetic toolchain downloads due to precompiling toolchain #1967

Closed

aignas commented Jun 14, 2024

View reviewed changes

aignas mentioned this pull request Sep 21, 2024

Consider supporting armv7-unknown-linux-gnueabi/hf toolchain #1876

Open

aignas mentioned this pull request Nov 1, 2024

change(pypi): use the python package version instead of sha in repo names #2366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bzlmod): support cross-platform whl setups within the main hub repo #1837

feat(bzlmod): support cross-platform whl setups within the main hub repo #1837

aignas commented Apr 8, 2024 •

edited

Loading

aignas commented Apr 14, 2024 •

edited

Loading

aignas commented May 18, 2024

aignas commented May 31, 2024

dougthor42 commented Jun 1, 2024

aignas commented Jun 1, 2024

aignas commented Jun 2, 2024

aignas commented Jun 2, 2024 •

edited

Loading

aignas commented Jun 3, 2024

aignas Jun 3, 2024

groodt Jun 5, 2024

aignas Jun 4, 2024

aignas Jun 4, 2024

aignas Jun 4, 2024

dougthor42 commented Jun 4, 2024

aignas commented Jun 4, 2024

dougthor42 commented Jun 4, 2024

groodt left a comment

aignas commented Jun 5, 2024

aignas Jun 14, 2024

feat(bzlmod): support cross-platform whl setups within the main hub repo #1837

feat(bzlmod): support cross-platform whl setups within the main hub repo #1837

Conversation

aignas commented Apr 8, 2024 • edited Loading

aignas commented Apr 14, 2024 • edited Loading

aignas commented May 18, 2024

aignas commented May 31, 2024

dougthor42 commented Jun 1, 2024

aignas commented Jun 1, 2024

aignas commented Jun 2, 2024

aignas commented Jun 2, 2024 • edited Loading

aignas commented Jun 3, 2024

aignas Jun 3, 2024

Choose a reason for hiding this comment

groodt Jun 5, 2024

Choose a reason for hiding this comment

aignas Jun 4, 2024

Choose a reason for hiding this comment

aignas Jun 4, 2024

Choose a reason for hiding this comment

aignas Jun 4, 2024

Choose a reason for hiding this comment

dougthor42 commented Jun 4, 2024

aignas commented Jun 4, 2024

dougthor42 commented Jun 4, 2024

groodt left a comment

Choose a reason for hiding this comment

aignas commented Jun 5, 2024

aignas Jun 14, 2024

Choose a reason for hiding this comment

aignas commented Apr 8, 2024 •

edited

Loading

aignas commented Apr 14, 2024 •

edited

Loading

aignas commented Jun 2, 2024 •

edited

Loading