experimental_index_url can only be used in one pip.parse(), subsequent calls fail

[chrisirhc]

🐞 bug report

Affected Rule

pip.parse()

Is this a regression?

I'm not sure.

Description

I have 4 lock files for 2 different Python versions over 2 platforms. When I try to use experimental_index_url on both pip.parse calls, it fails.

🔬 Minimal Reproduction

• I'll put together a minimum repro repo when I get a chance

Here's an excerpt from MODULE.bazel:

pip = use_extension("@rules_python//python/extensions:pip.bzl", "pip")
pip.parse(
    experimental_extra_index_urls = PYPI_EXTRA_INDEX_URLS,
    experimental_index_url = PYPI_INDEX_URL,
    experimental_requirement_cycles = base_requirement_cycles,
    extra_pip_args = pip_parse_extra_pip_args,
    hub_name = "third_party_python_base",
    python_version = "3.9",
    requirements_darwin = "//third_party/python/base:requirements_3_9_darwin.txt",
    requirements_linux = "//third_party/python/base:requirements_3_9.txt",
)
pip.parse(
    experimental_extra_index_urls = PYPI_EXTRA_INDEX_URLS,
    experimental_index_url = PYPI_INDEX_URL,
    experimental_requirement_cycles = base_requirement_cycles,
    extra_pip_args = pip_parse_extra_pip_args,
    hub_name = "third_party_python_base",
    python_version = "3.11",
    requirements_darwin = "//third_party/python/base:requirements_3_11_darwin.txt",
    requirements_linux = "//third_party/python/base:requirements_3_11.txt",
)

🔥 Exception or Error

ERROR: /home/user/.cache/bazel/…/6cbf57f86cd7ffd2869796107fcd24ff/external/rules_python~/python/private/pypi/extension.bzl:247:25: Traceback (most recent call last):
        File "/home/user/.cache/bazel/…/6cbf57f86cd7ffd2869796107fcd24ff/external/rules_python~/python/private/pypi/extension.bzl", line 610, column 25, in _pip_impl
                mods = parse_modules(module_ctx)
        File "/home/user/.cache/bazel/…/6cbf57f86cd7ffd2869796107fcd24ff/external/rules_python~/python/private/pypi/extension.bzl", line 485, column 36, in parse_modules
                out = _create_whl_repos(
        File "/home/user/.cache/bazel/…/6cbf57f86cd7ffd2869796107fcd24ff/external/rules_python~/python/private/pypi/extension.bzl", line 247, column 25, in _create_whl_repos
                fail("Attempting to creating a duplicate library {} for {}".format(
Error in fail: Attempting to creating a duplicate library third_party_python_base_311_python_bidi_sdist_0665a082 for python_bidi

🌍 Your Environment

Operating System:

  
Linux
  

Output of bazel version:

  
Build label: 7.4.1
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Mon Nov 11 21:24:53 2024 (1731360293)
Build timestamp: 1731360293
Build timestamp as int: 1731360293
  

Rules_python version:

  
1.2.0
  

Anything else relevant?

[aignas]

Ah yes, the sdist repos need to be prefixed/suffixed with the python version. The fix should be reasonably easy - only the repo name should be updated.

PRs are welcome here if someone needs this before I can take a look at it.

[chrisirhc]

Thanks for the quick response. Making a reproduction was more difficult than I expected.

I'm assuming you mean somewhere in the following lines, is that right?
https://github.com/bazelbuild/rules_python/blob/a816962e509311c23230730b4b28f9d52a229949/python/private/pypi/whl_repo_name.bzl#L36

[aignas]

Yes, exactly. I think passing python_version to that function would be enough. Using something like python_version.replace(".", "_") would be a sufficient suffix to not cause clashes, but still work (beware, there should not be any . in the name or the python importing machinery gets angry).

[aignas]

For reference this comment was wrong, it is unclear yet why this is happening.

[chrisirhc]

More debug information on the 2nd sdist, logged on the line:https://github.com/bazelbuild/rules_python/blob/e7d2f09394dd14816310c4c661d2fefab33b2b1b/python/private/pypi/extension.bzl#L246

 repo_name:  third_party_python_base_311_python_bidi_sdist_0665a082
 whl_name:  python_bidi
 args:  {"repo": "third_party_python_base_311", "dep_template": "@third_party_python_base//{name}:{target}", "python_interpreter_target": Label("@@rules_python~~python~python_3_11_host//:python"), ..., "requirement": "python-bidi==0.6.0", "urls": ["…pi/packages/packages/42/72/798318217bf0ee8e68af67e6e23c5e1017b9ab83b96019bf240da7b7a72f/python_bidi-0.6.0.tar.gz"], "sha256": "0665a0826074a9ff8d29640c0c405a2710b671db14fcc8b1c3ee6615ff10b837", "filename": "python_bidi-0.6.0.tar.gz", "experimental_target_platforms": ["cp311_osx_aarch64", "cp311_osx_x86_64"]}
 config_setting:  struct(config_setting = None, filename = "python_bidi-0.6.0.tar.gz", target_platforms = ("cp311_osx_aarch64", "cp311_osx_x86_64"), version = "3.11")

 repo_name:  third_party_python_base_311_python_bidi_sdist_0665a082
 whl_name:  python_bidi
 args:  {"repo": "third_party_python_base_311", "dep_template": "@third_party_python_base//{name}:{target}", "python_interpreter_target": Label("@@rules_python~~python~python_3_11_host//:python"), ..., "requirement": "python-bidi==0.6.0", "urls": ["…pi/packages/packages/42/72/798318217bf0ee8e68af67e6e23c5e1017b9ab83b96019bf240da7b7a72f/python_bidi-0.6.0.tar.gz"], "sha256": "0665a0826074a9ff8d29640c0c405a2710b671db14fcc8b1c3ee6615ff10b837", "filename": "python_bidi-0.6.0.tar.gz", "experimental_target_platforms": ["cp311_linux_aarch64", "cp311_linux_arm", "cp311_linux_ppc", "cp311_linux_s390x", "cp311_linux_x86_64"]}
 config_setting:  struct(config_setting = None, filename = "python_bidi-0.6.0.tar.gz", target_platforms = ("cp311_linux_aarch64", "cp311_linux_arm", "cp311_linux_ppc", "cp311_linux_s390x", "cp311_linux_x86_64"), version = "3.11")

Key part is that the experimental_target_platforms from args and target_platforms from config_setting differ for the same sdist file.

[aignas]

Is there a bug in parse_requirements.bzl? Hmmm. I think I may know what is happening.

1. The requirements.txt files for the darwin and linux have different lines for the bidi package. Could you paste them here?
2. Because they are different, the logic in requirements.txt will think that all of the shas for each platform are unique, but in sdist they are not.

I think we need to fix the parse_requirements.bzl logic to not make assumptions and do the merging of the requirements.txt sources in a more clever way. This has unit tests, so should be doable.

Another option is to handle this at the extension level where the extension is not blindly creating sdists, but that option is less appealing to me because the former is easier to test.

[chrisirhc]

1. The requirements.txt files for the darwin and linux have different lines for the bidi package. Could you paste them here?

Yes, this was exactly the issue. The lines are slightly different, with one hash appearing on darwin lock file but not linux linux lock file, and one hash appearing on linux but not the darwin.

When I initially tried to create the reproduction, I wasn't using the exact same set of hashes as my main repo.
I've updated the reproduction now (requirements_3_11.txt and requirements_3_11_darwin.txt are now different in terms of hashes) and it's showing the same error, https://github.com/chrisirhc/rules_python-repro-multiple-exp/

[chrisirhc]

Took a stab at fixing it via #2658 but looks like it changes some behavior on mac/universal, and any whls. Potentially due to extension.bzl's handling. Let me know what you think.

[tbarrella]

Heads up this can be reproduced with a single pip.parse with something very similar to https://github.com/tbarrella/ambiguous:

• Make both requirements files the same
• To one of them, for one package, add

      --hash=sha256:0000000000000000000000000000000000000000000000000000000000000000 \
  

Workaround is to add/subtract hashes to make hashes the same across requirements files

[aignas]

OK, I have spent some time thinking about this and I will write down my analysis here.

The problem right now is that the output of parse_requirements may have the same wheels present multiple times and this causes the later code to try and create the whl_library instances multiple times.

The output of parse_requirements is structured as:

requirements = {
    "distro": [
        struct(
            whls = list[struct] | None,
            target_platforms = list[str],
            sdist = struct | None,
            extra_pip_args = list[str],
            line = str,
            distro = str (the same as the key in the dict),
            is_exposed = boolean,
        )
    ]
}

For further analysis we can look at each value of the requirements dict and try and understand the problem more. Some of the fields can differ by target platforms, so I'll denote them with letter p in there:

struct(
    whls = list[struct[p]],
    sdist = struct[p],
    extra_args = list[str][p],
    req_line = str[p],
    distro = str,
    is_exposed = boolean,
)

---

Putting that aside for a little, the whl_library instances should be created for each unique:

• (whl, req_line) - because the req_line has the requested extras, which influence the dependencies calculated in the whl_library. The extra_args to pip here are ignored.
• (sdist, req_line, extra_args) - because extra_args will be used by pip to build the sdist.
• (None, req_line, extra_args) - because extra_args will be used by pip to download the wheel for a particular platform or build the sdist. This is only created if there are no wheels or sdists.

Because extra_args and req_line can be different depending on the target_platform, it makes sense to use platform as we currently do in the generation of the name for the whl_library.

---

So ideally, we probably want to change the type that the parse_requirements returns so that we are correctly mapping things here to something that maps 1:1 to the created whl_library instances.

Maybe something like

requirements = {
    ("distro", is_exposed): [
        struct(
            dist = whl or sdist or None,
            req_line = req_line,
            extra_args = extra_args,
            target_platforms = [],
        )
     ]
}

This structure can be created by something like:

requirements = {
    (d, is_exposed): values.values()
    for (d, is_exposed), values in requirements.items()
}

Where the requirements value before this struct is:

requirements = {
    ("distro", is_exposed): {
        (dist, req_line, tuple(extra_pip_args): struct(
            dist = dist,
            target_platforms,
            extra_pip_args = extra_pip_args,
            req_line = req_line,
        )
    }
}

And this in turn is created by something like (the current_requirements is the current return value from parse_requirements):

requirements = {}
for distro, distro_requirements in current_requirements.items():
    for req in distro_requirements:
        all_dists = req.whls + (req.sdist if req.sdist else [])
        for dist in all_dists or [None]: 
            requirements.setdefault((distro, req.is_exposed), {})
                        .setdefault((dist, req.req_line, tuple(req.extra_pip_args)), struct(
                                        dist = dist,
                                        req_line = req.req_line,
                                        target_platforms = {},
                                        is_exposed = req.is_exposed # do we need it here
                                        extra_pip_args = req.extra_pip_args,
                                    ))
                        .target_platforms[target_platforms] = None

To actually validate if this model works, we should do the refactor first and then stack the second PR on top of the refactor and see how it works. I am somewhat confident that it should work like this just fine.