Skip to content

fix: Resolve incorrect platform specific dependency #2766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 11, 2025
Merged

fix: Resolve incorrect platform specific dependency #2766

merged 2 commits into from
Apr 11, 2025

Conversation

ewianda
Copy link
Contributor

@ewianda ewianda commented Apr 10, 2025 •
edited
Loading

This change addresses a bug where pip.parse selects the wrong requirement entry when multiple extras are listed with platform-specific markers.

🔍 Problem:

In a requirements.txt generated by tools like uv or poetry, it's valid to have multiple entries for the same package, each with different extras and sys_platform markers, for example:

optimum[onnxruntime]==1.17.1 ; sys_platform == 'darwin'
optimum[onnxruntime-gpu]==1.17.1 ; sys_platform == 'linux'

The current implementation in [parse_requirements.bzl](https://github.com/bazel-contrib/rules_python/blob/032f6aa738a673b13b605dabf55465c6fc1a56eb/python/private/pypi/parse_requirements.bzl#L114-L126) uses a sort-by-length heuristic to select the “best” requirement when there are multiple entries with the same base name. This works well in legacy requirements.txt files where:

my_dep
my_dep[foo]
my_dep[foo,bar]

...would indicate an intent to select the most specific subset of extras (i.e. the longest name).

However, this heuristic breaks in the presence of platform markers, where extras are not subsets, but distinct variants. In the example above, Bazel mistakenly selects optimum[onnxruntime-gpu] on macOS because it's a longer match, even though it is guarded by a Linux-only marker.

✅ Fix:

This PR modifies the behavior to:

  1. Add the requirement marker as part of the sorting key.
  2. Then apply the longest-match logic to drop duplicate requirements with different extras but the same markers.

This ensures that only applicable requirements are considered during resolution, preserving correctness in multi-platform environments.

🧪 Before:

On macOS, the following entry is incorrectly selected:

optimum[onnxruntime-gpu]==1.17.1 ; sys_platform == 'linux'

✅ After:

Correct entry is selected:

optimum[onnxruntime]==1.17.1 ; sys_platform == 'darwin'

close #2690

@ewianda ewianda force-pushed the fix-incorrect-platform-specific-dependency branch from e8ab944 to ab105fd Compare April 10, 2025 06:20
@ewianda ewianda mentioned this pull request Apr 10, 2025
Closed
aignas
aignas reviewed Apr 10, 2025
View reviewed changes
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a CHANGELOG item for future reference?

@ewianda ewianda force-pushed the fix-incorrect-platform-specific-dependency branch 2 times, most recently from e827a01 to d3acfc0 Compare April 10, 2025 09:03
aignas added a commit to aignas/rules_python that referenced this pull request Apr 10, 2025
@aignas
fix(pypi): fixes to the marker evaluation and utils
f260d43 
These are just bugfixes to already merged code:
* Fix nested bracket parsing in PEP508 marker parser.
* Fix the sys_platform constants, which I noticed in bazel-contrib#2629 but they got
  also pointed out in bazel-contrib#2766.
* Port some of python tests for requirement parsing and improve the
  implementation. Those tests will be removed in bazel-contrib#2629.
* Move the platform related code to a separate file.

All of the bug fixes have added tests.

Work towards bazel-contrib#2423.
@aignas aignas mentioned this pull request Apr 10, 2025
Merged
aignas added a commit to aignas/rules_python that referenced this pull request Apr 10, 2025
@aignas
fix(pypi): fixes to the marker evaluation and utils
08f5114 
These are just bugfixes to already merged code:
* Fix nested bracket parsing in PEP508 marker parser.
* Fix the sys_platform constants, which I noticed in bazel-contrib#2629 but they got
  also pointed out in bazel-contrib#2766.
* Port some of python tests for requirement parsing and improve the
  implementation. Those tests will be removed in bazel-contrib#2629.
* Move the platform related code to a separate file.
* Rename `pep508_req.bzl` to `pep508_requirement.bzl` to follow the
  convention.

All of the bug fixes have added tests.

Work towards bazel-contrib#2423.
@ewianda
Copy link
Contributor Author

ewianda commented Apr 10, 2025

@aignas is this good to, merge. It is currently a blocker for us

github-merge-queue bot pushed a commit that referenced this pull request Apr 10, 2025
@aignas
fix(pypi): fixes to the marker evaluation and utils (#2767)
55d6836 
These are just bugfixes to already merged code:
* Fix nested bracket parsing in PEP508 marker parser.
* Fix the sys_platform constants, which I noticed in #2629 but they got
  also pointed out in #2766.
* Port some of python tests for requirement parsing and improve the
  implementation. Those tests will be removed in #2629.
* Move the platform related code to a separate file.
* Rename `pep508_req.bzl` to `pep508_requirement.bzl` to follow the
  convention.


All of the bug fixes have added tests.

Work towards #2423.
aignas
aignas reviewed Apr 11, 2025
View reviewed changes
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also update the PR description to be more descriptive please? The PR template has good suggestions on how to structure it (i.e. the user visible change in behaviour should be explained).

ewianda and others added 2 commits April 11, 2025 05:40
@ewianda
fix: Resolve incorrect platform specific dependency
f148ca6
@ewianda @aignas
Update CHANGELOG.md
b11e59e 
Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>
@ewianda ewianda force-pushed the fix-incorrect-platform-specific-dependency branch from 448d577 to b11e59e Compare April 11, 2025 10:05
@aignas aignas enabled auto-merge April 11, 2025 23:18
@aignas aignas added this pull request to the merge queue Apr 11, 2025
@aignas
Copy link
Collaborator

aignas commented Apr 11, 2025 •
edited
Loading

The improvements to the requirements utility are great, thank you!

Merged via the queue into bazel-contrib:main with commit 84351d4 Apr 11, 2025
3 checks passed
@ewianda ewianda deleted the fix-incorrect-platform-specific-dependency branch April 12, 2025 15:19
@ewianda
Copy link
Contributor Author

ewianda commented Apr 14, 2025

@aignas I am getting

ERROR: /home/ewianda/.cache/bazel/_bazel_ewianda/da4b4cc49e0e621570c9e24d6f1eab95/external/rules_python~/python/private/pypi/extension.bzl:231:25: Traceback (most recent call last):
        File "/home/ewianda/.cache/bazel/_bazel_ewianda/da4b4cc49e0e621570c9e24d6f1eab95/external/rules_python~/python/private/pypi/extension.bzl", line 611, column 25, in _pip_impl
                mods = parse_modules(module_ctx)
        File "/home/ewianda/.cache/bazel/_bazel_ewianda/da4b4cc49e0e621570c9e24d6f1eab95/external/rules_python~/python/private/pypi/extension.bzl", line 487, column 36, in parse_modules
                out = _create_whl_repos(
        File "/home/ewianda/.cache/bazel/_bazel_ewianda/da4b4cc49e0e621570c9e24d6f1eab95/external/rules_python~/python/private/pypi/extension.bzl", line 231, column 25, in _create_whl_repos
                fail("Attempting to creating a duplicate library {} for {}".format(
Error in fail: Attempting to creating a duplicate library py_deps_311_optimum_py3_none_any_508bc55d for optimum

Where should I be looking to fix this

@ewianda
Copy link
Contributor Author

ewianda commented Apr 14, 2025 •
edited
Loading

NVM, it is coming from uv astral-sh/uv#8915 (comment)
optimum[onnxruntime-gpu]==1.17.1 ; sys_platform == 'linux'
optimum[onnxruntime]==1.17.1 ; sys_platform == 'darwin'

have same hashes so the repo name is thesame

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aignas aignas aignas approved these changes

@rickeylev rickeylev Awaiting requested review from rickeylev rickeylev is a code owner

@groodt groodt Awaiting requested review from groodt groodt is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect platform-specific dependency selected from universal requirements.txt using pip.parse
2 participants
@ewianda @aignas