[go: up one dir, main page]

Skip to content
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

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

Open
psalaberria002 opened this issue Oct 21, 2024 · 1 comment

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

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 <richardlev@gmail.com>
@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 bazelbuild#2319 and bazelbuild#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 bazelbuild#2319 and bazelbuild#2423.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants