feat(bzlmod): cross-platform builds without experimental_index_url #2325

aignas · 2024-10-22T02:48:17Z

With this change we finally generate the same lock file within the
legacy code pip.parse code path and it allows to slowly transition to
using the new code path as much as possible without user doing anything.

This moves the selection of the host-compatible lock file from the
extension evaluation to the build phase - note, we will generate extra
repositories here which might not work on the host platform, however, if
the users are consuming the whl_library repos through the hub repo
only, then everything should work. A known issue is that it may break
bazel query and in these usecases it is advisable to use cquery
until we have sdist cross-building from source fully working.

Summary:

feat: reuse the render_pkg_aliases for when filename is not known
but platform is known
feat: support generating the extra config settings required
feat: get_whl_flag_versions now generates extra args for the rules
feat: make lock file generation the same irrespective of the host
platform
test: add an extra test with multiple requirements files
feat: support cross-platform builds using download_only = True in
legacy setups

Note, that users depending on the naming of the whl libraries will need
to start using extra_hub_aliases attribute instead to keep their
setups not relying on this implementation detail.

Fixes #2268
Work towards #260

… platform is known

…tform

aignas · 2024-10-22T02:59:13Z

@keith, could you please check to see if this is fixing the issues you were seeing previously? The CI is not fully green, but the examples and unit tests are passing, so I think this might be ready for a spin.

aignas · 2024-10-22T03:01:42Z

tests/pypi/extension/extension_tests.bzl

+            "repo": "pypi_315",
+            "requirement": "simple==0.0.1 --hash=sha256:deadbeef",
+        },
+    })


I am happy that my selection of the mock ctx.os values was already validating the bug in question.

python/private/pypi/extension.bzl

keith · 2024-10-22T18:13:33Z

it's possible it's just fighting getting the lockfile updated, but with this i see:

% bz mod graph --lockfile_mode=update
ERROR: module extension "pip" from "@@rules_python~//python/extensions:pip.bzl" does not generate repository "pip_deps_311_torch", yet it is imported as "pip_deps_311_torch" in the usage at .../MODULE.bazel:237:20 (did you mean 'pip_deps_311_munch'?). Type 'bazel help mod' for syntax and help.

looks like i hit that with or without experimental_index_url

CHANGELOG.md

rickeylev · 2024-10-22T22:22:06Z

CHANGELOG.md

+  file irrespective if `experimental_index_url` is set by any module or not.
+  Fixes [#2268](https://github.com/bazelbuild/rules_python/issues/2268). A known
+  issue is that it may break `bazel query` and in these usecases it is advisable
+  to use `cquery` until we have `sdist` cross-building from source fully working.


download_only=True is also an option, right? Assuming one is OK with not having sdists

It could be, but I haven't tested with regular requirements files, let me add a unit test to see how that would get wired through.

EDIT: With a few extra modifications, I think we can actually support that. This means that we can resolve #260 and have the stabilization for experimental_index_url done as a separate ticket. I've added extra docs on this, please check. :)

python/private/pypi/extension.bzl

aignas · 2024-10-22T23:46:46Z

it's possible it's just fighting getting the lockfile updated, but with this i see:

% bz mod graph --lockfile_mode=update
ERROR: module extension "pip" from "@@rules_python~//python/extensions:pip.bzl" does not generate repository "pip_deps_311_torch", yet it is imported as "pip_deps_311_torch" in the usage at .../MODULE.bazel:237:20 (did you mean 'pip_deps_311_munch'?). Type 'bazel help mod' for syntax and help.

looks like i hit that with or without experimental_index_url

@keith, I should have explained better in the PR message/changelog, that with this change you will have multiple torch variants registered and they will be pip_deps_311_torch__{'_'.join(target_platforms)}. The spoke repository naming is an implementation and should not be relied on too much. What are you using from the spoke repository? Some headers? In this case the best option would be to use @pip_deps//torch and whl_filegroup or similar to do that, what do you think?

Co-authored-by: Richard Levasseur <richardlev@gmail.com>

…oad_only args

We won't have correct dependencies when parsing the METADATA files because we need to pass the target platforms there. I think I got too happy too soon about the possibility of having correct builds with just 'download_only = True'.

This reverts commit bf4f644.

…ownload_only case

rickeylev · 2024-10-30T17:26:22Z

That is only needed if cc_library is unhappy with an empty file.

cc_library is OK with empty srcs

This adds back the internal extension to ensure that users are not forced into the feature.

…tries host independent

…file entries host independent

…n lock file entries host independent

aignas · 2024-11-04T11:21:43Z

OK, quick status update - once #2369 is merged, I will update this PR to be
backwards compatible (i.e. users will have to opt into the new behaviour on
0.38.0) and then merge this PR. Then we can cut a release. Once that is done,
we can have our 1.0 I think.

…repos (#2369) Before this change, it was impossible for users to use the targets created with `additive_build_content` whl annotation unless they relied on the implementation detail of the naming of the spoke repositories and had `use_repo` statements in their `MODULE.bazel` files importing the spoke repos. With #2325 in the works, users will have to change their `use_repo` statements, which is going to be disruptive. In order to offer them an alternative for not relying on the names of the spokes, there has to be a way to expose the extra targets created and this PR implements a method. Incidentally, the same would have happened if we wanted to stabilize the #260 work and mark `experimental_index_url` as non-experimental anymore. I was hoping we could autodetect them by parsing the build content ourselves in the `pip` extension, but it turned out to be extremely tricky and I figured that it was better to have an API rather than not have it. Whilst at it, also relax the naming requirements for the `whl_modifications` attribute. Fixes #2187

aignas · 2024-11-05T14:03:03Z

@keith, could you please check this PR again? I think it should work for your usecase now.

keith · 2024-11-05T14:19:09Z

Yes sorry I'm OOO but I will as soon as I'm back next week

aignas · 2024-11-07T03:51:28Z

Since the behaviour is opt-in, I'll merge the PR now and we can make further fixes later, but I am still looking forward to feedback next week.

keith · 2024-11-20T00:41:26Z

I think I could probably make whl_filegroup work, but it definitely breaks some use cases we're using today. For example today in my additive_build_content_file I do this:

_LIBS = [
    lib
    for lib in glob(
        [
            "site-packages/torch/lib/*.so*",
            "site-packages/torch.libs/*.so*",
            "site-packages/torch/lib/*.dylib",
        ],
        allow_empty = True,
        exclude = [
            "site-packages/torch/lib/*libgomp*",  # Handled below
            # Intel only, seem to be unnecessary for our use case
            "**/libtorchbind_test*",
            "**/libbackend_with_compiler*",
            "**/libjitbackend_test*",
        ],
    )
]

And then I expand multiple targets based on the _LIBS list below using list comprehensions. I don't think this has a clear 1:1 since if I had these as a filegroup it would not be the same

keith · 2024-11-20T02:16:35Z

seems like one option is this:

    extra_hub_aliases = {
        "torch": [
            "stock-torch",
            "stock-torch-headers",
            "stock-torch-api",
            "torchlib",
        ],
        "torchvision": ["torchvisionpy"],
    },

which pretty much gets me back to what i had before, is that one not recommended?

aignas added 6 commits October 22, 2024 10:51

feat: reuse the render_pkg_aliases for when filename is not known but…

4eb639e

… platform is known

feat: support generating the extra config settings required

ab8b2f0

feat: get_whl_flag_versions now generates extra args for the rules

645885c

feat: make lock file generation the same irrespective of the host pla…

1d33f49

…tform

test: add an extra test with multiple requirements files

9544386

doc: CHANGELOG

b81f86e

aignas requested review from groodt and rickeylev as code owners October 22, 2024 02:48

aignas marked this pull request as draft October 22, 2024 02:49

bring back code used in workspace

9b296d2

aignas marked this pull request as ready for review October 22, 2024 02:56

aignas commented Oct 22, 2024

View reviewed changes

fixup the generation of the config settings

77754c2

aignas commented Oct 22, 2024

View reviewed changes

python/private/pypi/extension.bzl Show resolved Hide resolved

aignas added 3 commits October 22, 2024 12:19

Update python/private/pypi/extension.bzl

d9a0d3b

wip

fdfa1de

wip

f7051ab

rickeylev approved these changes Oct 22, 2024

View reviewed changes

aignas and others added 9 commits October 23, 2024 08:48

Update CHANGELOG.md

0c058a1

Co-authored-by: Richard Levasseur <richardlev@gmail.com>

test: check that parse_requirements handles multiple files with downl…

e5b0190

…oad_only args

ensure that we can support multi-platform setups with the legacy pip

0400154

fix: make the names of the repos more user friendly

c75039d

revert docs about cross-platform building

bf4f644

We won't have correct dependencies when parsing the METADATA files because we need to pass the target platforms there. I think I got too happy too soon about the possibility of having correct builds with just 'download_only = True'.

Revert "revert docs about cross-platform building"

4de5a45

This reverts commit bf4f644.

fix the docs to fully document what is possible today

0d468df

adjust the code so that the experimental_target_platforms is set in d…

8d418f4

…ownload_only case

Merge branch 'main' into fix/lockfile-inconsistency

cfcc8c5

aignas added 3 commits November 1, 2024 19:00

Merge branch 'main' into fix/lockfile-inconsistency

5025f90

fixup! Merge branch 'main' into fix/lockfile-inconsistency

e90d257

use a different scheme inspired by bazelbuild#2366

8835d02

aignas marked this pull request as draft November 1, 2024 14:24

aignas added 10 commits November 1, 2024 23:30

one more try

e2c07ae

fix the tests

74383f3

simplify

4120c15

Merge branch 'main' into fix/lockfile-inconsistency

b4dad40

revert: start making the change in behaviour opt in

7582d5b

This adds back the internal extension to ensure that users are not forced into the feature.

add a flag to use all requirements to make the extension lock file en…

e5d2ff4

…tries host independent

fixup! add a flag to use all requirements to make the extension lock …

5c45b4a

…file entries host independent

fixup! fixup! add a flag to use all requirements to make the extensio…

58e7a6c

…n lock file entries host independent

fixup tests

c0fe195

wip

1ffb52c

aignas mentioned this pull request Nov 4, 2024

fix(bzlmod): allow users to specify extra targets to be added to hub repos #2369

Merged

aignas mentioned this pull request Nov 4, 2024

Release version 1.0 #1361

Closed

10 tasks

aignas added 2 commits November 5, 2024 22:45

Merge branch 'main' into fix/lockfile-inconsistency

b7577ff

wip

a2b8b52

aignas marked this pull request as ready for review November 5, 2024 13:57

aignas added this pull request to the merge queue Nov 7, 2024

Merged via the queue into bazelbuild:main with commit d66e55c Nov 7, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bzlmod): cross-platform builds without experimental_index_url #2325

feat(bzlmod): cross-platform builds without experimental_index_url #2325

aignas commented Oct 22, 2024 •

edited

Loading

aignas commented Oct 22, 2024

aignas Oct 22, 2024

keith commented Oct 22, 2024 •

edited

Loading

rickeylev Oct 22, 2024

aignas Oct 22, 2024 •

edited

Loading

aignas commented Oct 22, 2024 •

edited

Loading

rickeylev commented Oct 30, 2024

aignas commented Nov 4, 2024

aignas commented Nov 5, 2024

keith commented Nov 5, 2024

aignas commented Nov 7, 2024

keith commented Nov 20, 2024

keith commented Nov 20, 2024

feat(bzlmod): cross-platform builds without experimental_index_url #2325

feat(bzlmod): cross-platform builds without experimental_index_url #2325

Conversation

aignas commented Oct 22, 2024 • edited Loading

aignas commented Oct 22, 2024

aignas Oct 22, 2024

Choose a reason for hiding this comment

keith commented Oct 22, 2024 • edited Loading

rickeylev Oct 22, 2024

Choose a reason for hiding this comment

aignas Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

aignas commented Oct 22, 2024 • edited Loading

rickeylev commented Oct 30, 2024

aignas commented Nov 4, 2024

aignas commented Nov 5, 2024

keith commented Nov 5, 2024

aignas commented Nov 7, 2024

keith commented Nov 20, 2024

keith commented Nov 20, 2024

aignas commented Oct 22, 2024 •

edited

Loading

keith commented Oct 22, 2024 •

edited

Loading

aignas Oct 22, 2024 •

edited

Loading

aignas commented Oct 22, 2024 •

edited

Loading