rules_python icon indicating copy to clipboard operation
rules_python copied to clipboard

feat(bzlmod): support multi-platform wheel hubs [WIP]

Open aignas opened this issue 1 year ago • 2 comments

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.
  • Different requirements files per (os, arch) tuples.
  • Uses os, arch specific dependency closures (leveraging the experimental_target_platforms).
  • Use string_flags to configure how to build things (no transition support for now).

aignas avatar Apr 08 '24 14:04 aignas

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

  • [x] refactor groups to be created inside the hub repo. #1856.
  • [x] 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. #1854.
  • [x] use skylib for selects in the alias repo. #1855.
  • [x] Add multi platform requirement support with all of the available wheels registered.

aignas avatar Apr 14 '24 14:04 aignas

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

aignas avatar May 18 '24 05:05 aignas

@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.

aignas avatar May 31 '24 05:05 aignas

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.

dougthor42 avatar Jun 01 '24 03:06 dougthor42

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 avatar Jun 01 '24 05:06 aignas

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.

aignas avatar Jun 02 '24 15:06 aignas

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.

aignas avatar Jun 02 '24 16:06 aignas

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 avatar Jun 03 '24 04:06 aignas

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

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

dougthor42 avatar Jun 04 '24 02:06 dougthor42

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

aignas avatar Jun 04 '24 05:06 aignas

Yup! 445337e worked for me :+1:

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?

dougthor42 avatar Jun 04 '24 14:06 dougthor42

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

aignas avatar Jun 05 '24 23:06 aignas