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

feat(py_wheel): deps can provide requirement specifiers for Requires-Dist #1776

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

pcj
Copy link
Member

@pcj pcj commented Feb 23, 2024

This feature adds two new providers (PyRequirementInfo and PyRequirementsInfo) to py_wheel.bzl.

Downstream users can use these Providers to supply additional requirement specifiers to the generated wheel metadata.

The use case is that we'd like wheels to contain files limited to transitive dependencies not including those which are available from pypi directly. If we use py_package out of the box, we get "fat wheels" that contain too many files (all of protobuf, for example). Rather, we'd like to be able to have our own "thinner" implementation of py_package that filters protobuf out, but also include Requires-Dist: protobuf>=4.25.2 in the wheel metadata.

This is similar but more limited in scope than #1182.

@pcj pcj requested a review from rickeylev as a code owner February 23, 2024 02:00
@pcj
Copy link
Member Author

pcj commented Feb 23, 2024

Example py_project rule that demonstrates usage:

"Implementation of py_package rule"

load("@rules_python//python:packaging.bzl", "PyRequrirementInfo", "PyRequrirementsInfo")

PYPI_LOCK_PREFIX = "../pypi/_lock/"

def _py_package_impl(ctx):
    inputs = depset(
        transitive = [dep[DefaultInfo].data_runfiles.files for dep in ctx.attr.deps] +
                     [dep[DefaultInfo].default_runfiles.files for dep in ctx.attr.deps],
    )

    files = []
    requirements = []

    for input_file in inputs.to_list():
        filename = input_file.short_path
        if filename.startswith(PYPI_LOCK_PREFIX):
            filename = filename[len(PYPI_LOCK_PREFIX):]
            name, version = filename.split("@")
            if name and version:
                requirements.append(PyRequrirementInfo(
                    name = name,
                    version = version,
                    specifier = "%s>=%s" % (name, version),
                ))
        else:
            files.append(input_file)

    return [
        DefaultInfo(
            files = depset(direct = files),
        ),
        PyRequrirementsInfo(
            label = ctx.label,
            requirements = requirements,
        ),
    ]

py_package = rule(
    doc = """\
A rule to select all files in transitive dependencies of deps which
belong to given set of Python packages.

This rule is intended to be used as data dependency to py_wheel rule.
""",
    implementation = _py_package_impl,
    attrs = {
        "deps": attr.label_list(
            doc = "",
        ),
    },
)

The _lock/ prefix is specific to https://github.com/jvolkman/rules_pycross

@aignas
Copy link
Collaborator

aignas commented Feb 27, 2024

I like the idea but was wondering if the py_project rule should be part of the rules_python, at least as an example if we are not sure that we want to increase the API surface we are maintaining. What do you think, @pcj?

@pcj
Copy link
Member Author

pcj commented Feb 27, 2024

I'm happy to include it, perhaps under a directory like examples/pycross_project/ since this particular implementation is specific to rules_pycross?

cc @jvolkman

Comment on lines 36 to 37
"specifier": "String: a formatted string, to be used verbatim for 'Requires-Dist' metadata specification (e.g 'protobuf>=4.25.2', see https://pip.pypa.io/en/stable/reference/requirement-specifiers/)",
"version": "String: the concrete version of the pypi requirement (e.g. '4.25.2')",
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't specifier and version redundant? Do you need both?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is redundant. I left version in because I thought someone might want to know the concrete version rather than having to parse the specifier, but since I don't have a specific use for it myself, should probably be removed for the sake of simplicity (could be added later)

"name": "String: the name of the pypi dependency (e.g. 'protobuf')",
"specifier": "String: a formatted string, to be used verbatim for 'Requires-Dist' metadata specification (e.g 'protobuf>=4.25.2', see https://pip.pypa.io/en/stable/reference/requirement-specifiers/)",
"version": "String: the concrete version of the pypi requirement (e.g. '4.25.2')",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to also include markers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably? I'm somewhat of an imposter when it comes to python, but after doing more reading it would make sense to include them (List[String]?)

@jvolkman
Copy link
Contributor

I'm happy to include it, perhaps under a directory like examples/pycross_project/ since this particular implementation is specific to rules_pycross?

cc @jvolkman

Not sure it makes sense to have this example in rules_python as written, with dependencies on _lock (a sort-of-implementation-detail, although it's described in a comment) and pypi (a user-specified repo name).

Could the example be retooled to rely only on stuff in rules_python?

@pcj
Copy link
Member Author

pcj commented Feb 28, 2024

Added examples/custom_py_project/

@@ -316,17 +316,45 @@ tasks:
working_directory: examples/multi_python_versions
platform: windows

integration_test_custom_py_project_ubuntu_min_workspace:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will deplete the number of runners we have available for running builds. I remember @rickeylev did some work to optimize this so maybe relying on the bazel integration test framework would be better here?

@@ -21,6 +21,9 @@ A brief description of the categories of changes:

[0.XX.0]: https://github.com/bazelbuild/rules_python/releases/tag/0.XX.0

* (py_wheel) Wheel deps can supply requirement specifiers to the generated wheel
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: please rebase and put the bullet point into an appropriate section.

- the golden test depends on the generated wheel, whose METADATA has been
extracted and grepped for `Requires-Dist:`.

> NOTE: this example was originally based off `examples/pip_parse_vendored`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any new example code should be compatible with bzlmod, IMHO so that it is easier to to maintain the examples in the future. Are there reasons to not do this in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied over the files from examples/pip_parse_resolved, but perhaps a different example would be better as a base. Alternatively, I can just remove the WORKSPACE bits.


load("@rules_python//python:packaging.bzl", "PyRequrirementInfo", "PyRequrirementsInfo")

SITE_PACKAGES = "/site-packages/"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may break if we merge #1766, I wonder if there is a better way to do this.

@pcj pcj force-pushed the pcj/py_wheel_requires_dist branch 2 times, most recently from a392fd5 to 5fcb3fe Compare February 29, 2024 02:08
@pcj pcj force-pushed the pcj/py_wheel_requires_dist branch from 5fcb3fe to ee837de Compare February 29, 2024 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants