Skip to content

[pypi] Support python micro version in env marker based dependency evaluation #2319

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.

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

Open
psalaberria002 opened this issue Oct 21, 2024 · 2 comments
Labels
type: pip pip/pypi integration

Comments

@psalaberria002
Copy link

🚀 feature request

Relevant Rules

The whl_installer Python program needs to be updated.

Description

whl_installer should be aware of the selected full Python version (..) when resiolving dependencies.

Today, it is only aware of the minor version.

See the Redis 5.1.1 wheel. In the METADATA file it has Requires-Dist: async-timeout>=4.0.3; python_full_version < "3.11.3".

If the Python runtime is set to 3.11.10, the async-timeout dependency should be excluded, but it isn't. The repo BUILD file looks like this:

load("@bazel_skylib//rules:copy_file.bzl", "copy_file")
load("@rules_python//python:defs.bzl", "py_library", "py_binary")

package(default_visibility = ["//visibility:public"])

filegroup(
    name = "dist_info",
    srcs = glob(["site-packages/*.dist-info/**"], allow_empty = True),
)

filegroup(
    name = "data",
    srcs = glob(["data/**"], allow_empty = True),
)

filegroup(
    name = "whl",
    srcs = ["redis-5.1.1-py3-none-any.whl"],
    data = ["@python_deps//async_timeout:whl"],
    visibility = ["//visibility:public"],
)

py_library(
    name = "pkg",
    srcs = glob(
        ["site-packages/**/*.py"],
        exclude=[],
        # Empty sources are allowed to support wheels that don't have any
        # pure-Python code, e.g. pymssql, which is written in Cython.
        allow_empty = True,
    ),
    data = [] + glob(
        ["site-packages/**/*"],
        exclude=["**/* *", "**/*.py", "**/*.pyc", "**/*.pyc.*", "**/*.dist-info/RECORD"],
    ),
    # This makes this directory a top-level in the python import
    # search path for anything that depends on this.
    imports = ["site-packages"],
    deps = ["@python_deps//async_timeout:pkg"],
    tags = ["pypi_name=redis", "pypi_version=5.1.1"],
    visibility = ["//visibility:public"],
)

Describe the solution you'd like

@aignas aignas added type: bug type: pip pip/pypi integration labels Oct 22, 2024
github-merge-queue bot pushed a commit that referenced this issue Nov 13, 2024
This just cleans up the code and moves more logic from the
repository_rule
(i.e. generation of `BUILD.bazel` files) to loading time (macro
evaluation).
This makes the unit testing easier and I plan to also move the code that
is
generating config setting names from filenames to this new macro, but
wanted to
submit this PR to reduce the review chunks.

Summary:
- Add a new `pkg_aliases` macro.
- Move logic and tests for creating WORKSPACE aliases.
- Move logic and tests bzlmod aliases.
- Move logic and tests bzlmod aliases with groups.
- Add a test for extra alias creation.
- Use `whl_alias` in `pypi` extension integration tests.
- Improve the serialization of `whl_alias` for passing to the pypi hub
repo.

Related to #260, #2386, #2337, #2319 - hopefully cleaning the code up
will make
it easier to address those feature requests later.

---------

Co-authored-by: Richard Levasseur <[email protected]>
@aignas aignas changed the title Support full Python versions in whl_installer [pypi] Support python micro version in env marker based dependency evaluation Nov 18, 2024
@aignas
Copy link
Collaborator

aignas commented Nov 18, 2024

This is something of a known issue and the current design has this limitation described in the issue. To properly solve this I would like to have the following solution:

  • Implement a PEP508 parser in starlark so that we can evaluate things outside the repo rule context. We can use FeatureFlagInfo to pass the result of the marker evaluation to a config_setting. I tested if it is possible to write a sane parser and have the code working for this, but did not want to create a PR unless I find a good way to use it. [pypi] PEP508 Remove the need for Python in env_marker evaluation in hub repos #2423
  • Use the said method to pass markers from requirements.txt files to the hub_repository for selecting which actual whl_library gets selected. This will simplify the module extension code and will allow us to test the PEP508 parser and the config_setting machinery better. It is a simpler thing than going for the deps straight away. [pypi] PEP508 Remove the need for Python in env_marker evaluation in hub repos #2423
  • Use the said method to replace the whl_installer parsing of the METADATA and create a converter from a list of METADATA Requires-Deps lines to a list of dependency specifiers.

This is a lot of yak shaving for achieving what is asked in the issue, but I do think that moving the evaluation of the markers to the analysis phase is actually the right thing to do here.

Until then, one could possibly patch the piece of code that determines the micro version used in evaluating the specifiers in the wheel_installer code. Let me know if a patch is needed.

aignas added a commit to aignas/rules_python that referenced this issue Nov 18, 2024
Before hand we would pass around `whl_alias` struct instances and it
would go through rounds of starlark functions which would end up
rendered as BUILD.bazel files in the hub repository. This means that
every time the output of the said functions would change, the
`MODULE.bazel.lock` would also possibly change.

Since we now have much better unit testing framework, we start relying
on these functions being evaluated at loading phase instead of repo rule
phase, which means that we can make the `BUILD.bazel` files not change a
lot when the underlying code changes, this should make the code more
maintainable in the long run and unblocks the code to accept extra
things, like env marker values or similar.

Summary:
- Remove the `repo` field from `whl_alias`.
- Rename `whl_alias` to `whl_config_setting`
- Move the tests around
- Simplify the `pkg_aliases` tests to use `contains` instead of exact
  equality to make things less brittle.
- Simplify the `pkg_aliases` tests to use different mocks to make
  expectations easier to understand.
- Make `whl_config_setting` hashable by using tuples instead of lists.

This is definitely needed for bazel-contrib#2319 and bazel-contrib#2423.
aignas added a commit to aignas/rules_python that referenced this issue Nov 18, 2024
Before hand we would pass around `whl_alias` struct instances and it
would go through rounds of starlark functions which would end up
rendered as BUILD.bazel files in the hub repository. This means that
every time the output of the said functions would change, the
`MODULE.bazel.lock` would also possibly change.

Since we now have much better unit testing framework, we start relying
on these functions being evaluated at loading phase instead of repo rule
phase, which means that we can make the `BUILD.bazel` files not change a
lot when the underlying code changes, this should make the code more
maintainable in the long run and unblocks the code to accept extra
things, like env marker values or similar.

Summary:
- Remove the `repo` field from `whl_alias`.
- Rename `whl_alias` to `whl_config_setting`
- Move the tests around
- Simplify the `pkg_aliases` tests to use `contains` instead of exact
  equality to make things less brittle.
- Simplify the `pkg_aliases` tests to use different mocks to make
  expectations easier to understand.
- Make `whl_config_setting` hashable by using tuples instead of lists.

This is definitely needed for bazel-contrib#2319 and bazel-contrib#2423.
github-merge-queue bot pushed a commit that referenced this issue Nov 22, 2024
Before hand we would pass around `whl_alias` struct instances and it
would go through rounds of starlark functions which would end up
rendered as BUILD.bazel files in the hub repository. This means that
every time the output of the said functions would change, the
`MODULE.bazel.lock` would also possibly change.

Since we now have much better unit testing framework, we start relying
on these functions being evaluated at loading phase instead of repo rule
phase, which means that we can make the `BUILD.bazel` files not change a
lot when the underlying code changes, this should make the code more
maintainable in the long run and unblocks the code to accept extra
things, like env marker values or similar.

Summary:
- Remove the `repo` field from `whl_alias`.
- Rename `whl_alias` to `whl_config_setting`
- Move the tests around
- Simplify the `pkg_aliases` tests to use `contains` instead of exact
  equality to make things less brittle.
- Simplify the `pkg_aliases` tests to use different mocks to make
  expectations easier to understand.
- Make `whl_config_setting` hashable by using tuples instead of lists.
- Adjust how we store the `whl_config_setting` in the PyPI extension
  and optimize the passing to the `hub_repository`.

This is needed for #2319 and #2423.

To be in future PRs:
* Remove the need to pass `osx_versions`, etc to the `pkg_aliases`
macro.
ewianda pushed a commit to ewianda/rules_python that referenced this issue Nov 22, 2024
…ntrib#2424)

Before hand we would pass around `whl_alias` struct instances and it
would go through rounds of starlark functions which would end up
rendered as BUILD.bazel files in the hub repository. This means that
every time the output of the said functions would change, the
`MODULE.bazel.lock` would also possibly change.

Since we now have much better unit testing framework, we start relying
on these functions being evaluated at loading phase instead of repo rule
phase, which means that we can make the `BUILD.bazel` files not change a
lot when the underlying code changes, this should make the code more
maintainable in the long run and unblocks the code to accept extra
things, like env marker values or similar.

Summary:
- Remove the `repo` field from `whl_alias`.
- Rename `whl_alias` to `whl_config_setting`
- Move the tests around
- Simplify the `pkg_aliases` tests to use `contains` instead of exact
  equality to make things less brittle.
- Simplify the `pkg_aliases` tests to use different mocks to make
  expectations easier to understand.
- Make `whl_config_setting` hashable by using tuples instead of lists.
- Adjust how we store the `whl_config_setting` in the PyPI extension
  and optimize the passing to the `hub_repository`.

This is needed for bazel-contrib#2319 and bazel-contrib#2423.

To be in future PRs:
* Remove the need to pass `osx_versions`, etc to the `pkg_aliases`
macro.
aignas added a commit to aignas/rules_python that referenced this issue Dec 24, 2024
This is a workaround for bazel-contrib#2319, but I would like to find
a better way to solve it
@aignas
Copy link
Collaborator

aignas commented Dec 24, 2024

@psalaberria002, I have created a draft that may work around the issue you are having, could you please test?

@aignas aignas removed the type: bug label Mar 21, 2025
aignas added a commit to aignas/rules_python that referenced this issue Mar 23, 2025
This implements the PEP508 compliant marker evaluation in starlark and
removes the need for the Python interpreter when evaluating requirements
files passed to `pip.parse`. This makes the evaluation faster and allows
us to fix a few known issues (bazel-contrib#2690).

In the future the intent is to move the `METADATA` parsing to pure
starlark so that the `RequiresDist` could be parsed in starlark at the
macro evaluation or analysis phases. This should make it possible to
more easily solve the design problem that more and more things need to
be passed to `whl_library` as args to have a robust dependency parsing:
* bazel-contrib#2319 needs the full Python version to have correct cross-platform
  compatible METADATA parsing and passing it to `Python` and back makes
  it difficult/annoying to implement.
* Parsing the `METADATA` file requires the precise list of target
  platform or the list of available packages in the `requirements.txt`.
  This means that without it we cannot trim the dependency tree in the
  `whl_library`. Doing this at macro loading phase allows us to depend
  on `.bzl` files in the `hub_repository` and more effectively pass
  information.

Fixes bazel-contrib#2423
github-merge-queue bot pushed a commit that referenced this issue Mar 30, 2025
This implements the PEP508 compliant marker evaluation in starlark and
removes the need for the Python interpreter when evaluating requirements
files passed to `pip.parse`. This makes the evaluation faster and allows
us to fix a few known issues (#2690).

In the future the intent is to move the `METADATA` parsing to pure
starlark so that the `RequiresDist` could be parsed in starlark at the
macro evaluation or analysis phases. This should make it possible to
more easily solve the design problem that more and more things need to
be passed to `whl_library` as args to have a robust dependency parsing:
* #2319 needs the full Python version to have correct cross-platform
compatible `METADATA` parsing and passing it to `Python` and back makes
  it difficult/annoying to implement.
* Parsing the `METADATA` file requires the precise list of target
  platform or the list of available packages in the `requirements.txt`.
  This means that without it we cannot trim the dependency tree in the
  `whl_library`. Doing this at macro loading phase allows us to depend
  on `.bzl` files in the `hub_repository` and more effectively pass
  information.

I can remotely see that this could become useful in `py_wheel` or an
building
wheels from sdists as the environment markers may be present in various
source
metadata as well. What is more `uv.lock` file has the env markers as
part of
the lock file information, so this might be useful there.

Work towards #2423
Work towards #260
Split from #2629
github-merge-queue bot pushed a commit that referenced this issue Apr 13, 2025
This PR starts using the newly introduced (#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards #260, #2319, #2241
Fixes #2423
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423

fix(toolchain) Override coverage rc
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423

fix(toolchain) Override coverage rc
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423
ewianda pushed a commit to ewianda/rules_python that referenced this issue Apr 14, 2025
…ontrib#2629)

This PR starts using the newly introduced (bazel-contrib#2692) PEP508 compliant
requirement marker parser in starlark and moves the dependency
generation from the Python language (`whl_installer`) to the Starlark
in the `whl_library` repository rule.

This PR is (almost) a pure refactor where no bugs are fixed, but this is
foundational work that also adds notes on how things will be moved
to macros (i.e. analysis phase) so that we can fix a few long standing
bugs and prepare for stabilizing the `experimental_index_url` (bazel-contrib#260).

Refactor:
* I have migrated all of the unit tests from Python to starlark for deps
  generation from METADATA `Requires-Dist` fields.
* Read the `METADATA` file itself in Starlark.

Work towards bazel-contrib#260, bazel-contrib#2319, bazel-contrib#2241
Fixes bazel-contrib#2423
aignas added a commit to aignas/rules_python that referenced this issue Apr 16, 2025
This PR moves the parsing of `Requires-Dist` to the analysis phase
within the `whl_library_targets_from_requires` macro. The original
`whl_library_targets` macro has been left unchanged so that I don't have
to reinvent the unit tests - it is well covered under tests.

Before this PR we had to wire the `target_platforms` via the
`experimental_target_platforms` attr in the `whl_library`, which means
that whenever this would change (e.g. the minor Python version changes),
the wheel would be re-extracted even though the final result may be the
same.

This also cleans up the code by removing left over TODO notes or code
that no longer make sense.

Work towards bazel-contrib#260, bazel-contrib#2319
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: pip pip/pypi integration
Projects
None yet
Development

No branches or pull requests

2 participants