Repository: bazelbuild/rules_python (language: python)

Issue:
bazel deps query with platform-specific pypi deps fails
# 🐞 bug report

### Affected Rule

Somethinng in the pypi integration

### Is this a regression?

Maybe? I used to be able to do `ibazel build //docs:docs` to have docs automatically
rebuilt while working on them. Now bazel gives an error during the query for deps ibazel
performs.

### Description

`bazel query 'deps(//docs:docs)` fails with an error. I think what's happening is a windows-specific dependency gets evaluated, which triggers a repo rule, and then wheel_installer fails.

## 🔬 Minimal Reproduction

Run `bazel query 'deps(//docs:docs)'`

## 🔥 Exception or Error

Full error: https://gist.github.com/rickeylev/d32a94e6823cb1541babc8565d88ccf8

I suspect the most relevant parts are
```
repo.execute: whl_library.BuildWheelFromSource(_main~pip_internal~dev_pip_311_colorama_sdist_08695f5c, colorama==0.4.6 ;sys_platform == 'win32'): ...
----
Ignoring colorama: markers 'sys_platform == "win32"' don't match your environment
----
  File "/home/user/p/rickeylev/rules_python/python/private/pypi/whl_installer/wheel_installer.py", line 198, in main
    whl = Path(next(iter(glob.glob("*.whl"))))
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
StopIteration
```

Hm, maybe the issue is wheel_installer isn't expecting an "empty" response? 

cc @aignas 


Discussion / hints:
I think there are a few things here:
* Bazel query will fetch the sdists, that you technically don't need for the build even if the wheels are available. It would be better to use cquery, but I am not sure it is possible to tell that to ibazel.
* We could trim the requirements line to not fail in this particular case though, it would only be a problem for platform specific distributions, but that is already a problem, so maybe that should be done here.

On 16 September 2024 03:08:27 GMT+09:00, Richard Levasseur ***@***.***> wrote:
># 🐞 bug report
>
>### Affected Rule
>
>Somethinng in the pypi integration
>
>### Is this a regression?
>
>Maybe? I used to be able to do `ibazel build //docs:docs` to have docs automatically
>rebuilt while working on them. Now bazel gives an error during the query for deps ibazel
>performs.
>
>### Description
>
>`bazel query 'deps(//docs:docs)` fails with an error. I think what's happening is a windows-specific dependency gets evaluated, which triggers a repo rule, and then wheel_installer fails.
>
>## 🔬 Minimal Reproduction
>
>Run `bazel query 'deps(//docs:docs)'`
>
>## 🔥 Exception or Error
>
>Full error: https://gist.github.com/rickeylev/d32a94e6823cb1541babc8565d88ccf8
>
>I suspect the most relevant parts are
>```
>repo.execute: whl_library.BuildWheelFromSource(_main~pip_internal~dev_pip_311_colorama_sdist_08695f5c, colorama==0.4.6 ;sys_platform == 'win32'): ...
>----
>Ignoring colorama: markers 'sys_platform == "win32"' don't match your environment
>----
>  File "/home/user/p/rickeylev/rules_python/python/private/pypi/whl_installer/wheel_installer.py", line 198, in main
>    whl = Path(next(iter(glob.glob("*.whl"))))
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>StopIteration
>```
>
>Hm, maybe the issue is wheel_installer isn't expecting an "empty" response? 
>
>cc @aignas 
>
>-- 
>Reply to this email directly or view it on GitHub:
>https://github.com/bazelbuild/rules_python/issues/2223
>You are receiving this because you were mentioned.
>
>Message ID: ***@***.***>
I see that there is a related issue here: https://github.com/bazelbuild/bazel-watcher/issues/594
A few more notes to add to @aignas commentary above:
* rules_python inherits the common, but unfortunate install-or-build-then-install behaviour of python packaging
* to remove the edge from `//docs:docs` to any sdist won't be easy

Some ideas (not sure on practicality): 
* ensure a wheel-only third party dependency graph
* move installation of wheels to build actions and repositories only reference archives (sdist or wheels)
I would love to go with 
> move installation of wheels to build actions and repositories _to_ only reference archive

But there are complications - we need to know the dependency graph before downloading the wheels and that is tricky. I was thinking about this for some time and the only way I can see it is to actually use @philsc's idea to introduce a custom lockfile format for `rules_python` (the implementation could be different here) so that the python toolchain is only used at `lock` time and during fetching we are only wiring everything up without any need for a python interpreter.

Right now python interpreter is used for:
* building from sdist - should be moved to a build action.
* patching the wheel - if we used `http_archive` this step would not be needed anymore.
* parsing the METADATA within the wheel - could be done at lock time in a build action.
* extracting the wheel - could be dropped and we could instead used an extracted `whl` or `sdist` (by using `http_archive`) and use build rules to lay them out correctly in the sandbox.

The implementation for all this could be:
* When running `uv lock` we also write a file named `rules_python.lock`, which would be a JSON file that the `pip.parse(experimental_index_url="foo")` understands. This could be just a JSON file with the parsed deps based on the target platforms.
* The `pip.parse(experimental_index_url="foo")` then would not need to query the PyPI index, because the `rules_python.lock` would have all of the information. Thus we could mark the extension as reproducible and the entries within the `MODULE.bazel.lock` would disappear, making the `rules_python.lock` the source of truth for what gets installed via the `pip` machinery.

Whilst this is a departure from
> a regular python lock file should just work with `rules_python`
I do think that this closes the main design issues we have right now that prevent us from stabilizing the feature in #260:
* We need to have a separate extension if we want to stabilize the new code.
* We need to have a Python interpreter during repo init time thus duplicating the toolchain selection logic. It works as long as the toolchains are unique per (os, arch) tuple, but if `musl` or `glibc` selection or #1876 comes into play, everything becomes tricky.
* We need to access the internet when evaluating the `bzlmod` extension - we should not need to. We should shift left and create the lock file when we are running `lock`.

I think we can come back to this topic once 1.0 is released and introduce it in a backwards compatible way over the next minor versions.
> to remove the edge from //docs:docs to any sdist won't be easy

Oh? Why? Did you see a particularly thorny one in the deps? I'd be +1 on moving it to a wheel-only build. Also, I thought there was a flag or something you could set to force things to be wheel-only?

If we put `platform=linux or platform=mac` in the requirements, will that work around the issue? For the docs building, we don't care about windows.
> building from sdist - should be moved to a build action.

Hm. What if we just moved it into a build action "as-is"? i.e. it's still only going to work when target == host, but that's fine; that's how it is today. But at least it's a build action and out of the repo phase. Which would probably avoid this bug? (it'd download the sdist, but not build anything)

> patching wheel, parsing metadata, extracting wheel

I just want to note that these don't require any toolchain-equivalent wiring. The interpreter the tools use isn't coupled to the processing they do. So we could, alternatively, simply hard-code particular interpreters to be used with our tools in the repo phase.

Fix patch:
diff --git a/.bazelrc b/.bazelrc
index 1ca469cd75..b484751c3c 100644
--- a/.bazelrc
+++ b/.bazelrc
@@ -4,8 +4,8 @@
 # (Note, we cannot use `common --deleted_packages` because the bazel version command doesn't support it)
 # To update these lines, execute
 # `bazel run @rules_bazel_integration_test//tools:update_deleted_packages`
-build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,gazelle/python/private,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered
-query --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,gazelle/python/private,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered
+build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/python/private,gazelle/pythonconfig,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered
+query --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/python/private,gazelle/pythonconfig,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered
 
 test --test_output=errors
 
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 67c3477443..44333a5eb2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -65,6 +65,8 @@ A brief description of the categories of changes:
     * `3.12 -> 3.12.7`
 [20241008]: https://github.com/indygreg/python-build-standalone/releases/tag/20241008
 * (coverage) Add support for python 3.13 and bump `coverage.py` to 7.6.1.
+* (bzlmod) Add support for `download_only` flag to disable usage of `sdists`
+  when {bzl:attr}`pip.parse.experimental_index_url` is set.
 
 
 ### Removed
diff --git a/MODULE.bazel b/MODULE.bazel
index 58c7ae229b..780d7f8666 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -91,6 +91,7 @@ dev_pip = use_extension(
     dev_dependency = True,
 )
 dev_pip.parse(
+    download_only = True,  # this will not add the `sdist` values to the transitive closures at all.
     hub_name = "dev_pip",
     python_version = "3.11",
     requirements_lock = "//docs:requirements.txt",
diff --git a/examples/bzlmod/MODULE.bazel.lock b/examples/bzlmod/MODULE.bazel.lock
index 2ddbe40bc3..8ef6e97588 100644
--- a/examples/bzlmod/MODULE.bazel.lock
+++ b/examples/bzlmod/MODULE.bazel.lock
@@ -1231,7 +1231,7 @@
     },
     "@@rules_python~//python/extensions:pip.bzl%pip": {
       "general": {
-        "bzlTransitiveDigest": "kPvx0u6SR68H+8BqqEfxd3NpVZjuMOyzI9Xml01rdrQ=",
+        "bzlTransitiveDigest": "Rc0r+OwZ3f57lwygvIXLejgTsJKr8ezIcd136SpMyDo=",
         "usagesDigest": "MChlcSw99EuW3K7OOoMcXQIdcJnEh6YmfyjJm+9mxIg=",
         "recordedFileInputs": {
           "@@other_module~//requirements_lock_3_11.txt": "a7d0061366569043d5efcf80e34a32c732679367cb3c831c4cdc606adc36d314",
@@ -6138,7 +6138,7 @@
     },
     "@@rules_python~//python/private/pypi:pip.bzl%pip_internal": {
       "general": {
-        "bzlTransitiveDigest": "c3OA6iewVGq8nz0o3iI2AtIQhsRZIg/E/PDq2vuAQTw=",
+        "bzlTransitiveDigest": "s4J1aKQE20A+qzGkeB4XwzitVphP4H3vYmeLg6pMZxA=",
         "usagesDigest": "Y8ihY+R57BAFhalrVLVdJFrpwlbsiKz9JPJ99ljF7HA=",
         "recordedFileInputs": {
           "@@rules_python~//tools/publish/requirements.txt": "031e35d03dde03ae6305fe4b3d1f58ad7bdad857379752deede0f93649991b8a",
diff --git a/python/private/pypi/extension.bzl b/python/private/pypi/extension.bzl
index 200aa4327e..36fb20e030 100644
--- a/python/private/pypi/extension.bzl
+++ b/python/private/pypi/extension.bzl
@@ -151,9 +151,6 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
 
     get_index_urls = None
     if pip_attr.experimental_index_url:
-        if pip_attr.download_only:
-            fail("Currently unsupported to use `download_only` and `experimental_index_url`")
-
         get_index_urls = lambda ctx, distributions: simpleapi_download(
             ctx,
             attr = struct(
@@ -263,11 +260,11 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
             is_exposed = False
             for requirement in requirements:
                 is_exposed = is_exposed or requirement.is_exposed
-                for distribution in requirement.whls + [requirement.sdist]:
-                    if not distribution:
-                        # sdist may be None
-                        continue
+                dists = requirement.whls
+                if not pip_attr.download_only and requirement.sdist:
+                    dists = dists + [requirement.sdist]
 
+                for distribution in dists:
                     found_something = True
                     is_hub_reproducible = False
 
@@ -564,6 +561,11 @@ In the future this could be defaulted to `https://pypi.org` when this feature be
 stable.
 
 This is equivalent to `--index-url` `pip` option.
+
+:::{versionchanged} 0.37.0
+If {attr}`download_only` is set, then `sdist` archives will be discarded and `pip.parse` will
+operate in wheel-only mode.
+:::
 """,
         ),
         "experimental_index_url_overrides": attr.string_dict(


Test patch:
diff --git a/tests/pypi/integration/BUILD.bazel b/tests/pypi/integration/BUILD.bazel
index f846bfb124..9ea8dcebe4 100644
--- a/tests/pypi/integration/BUILD.bazel
+++ b/tests/pypi/integration/BUILD.bazel
@@ -1,5 +1,5 @@
 load("@bazel_skylib//rules:build_test.bzl", "build_test")
-load("@dev_pip//:requirements.bzl", "all_requirements")
+load("@rules_python_publish_deps//:requirements.bzl", "all_requirements")
 load(":transitions.bzl", "transition_rule")
 
 build_test(