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(bzlmod): support wheel-only additions to the hub repo #1744

Closed
wants to merge 73 commits into from

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Feb 5, 2024

With this PR we can reuse the same hub-repo and setup download_only = True
closures, which use only wheels but download the right platform specific wheels
independent of the host platform. That should allow users to build images with
rules_oci without needing to write extra code to create alias repos which
mimic the behaviour of rules_python ones.

This is stacked on #1743.

The snippet of the resultant pylint hub repo alias that is the main goal is:

alias(
    name = "pkg",
    actual = select(
        {
            "//:is_python_3.9": "@pip_39_pylint//:pkg",
            "//:is_python_3.9_linux_x86_64": "@pip_39_manylinux_2_17_x86_64_39_pylint//:pkg",
            "//:is_python_3.10": "@pip_310_pylint//:pkg",
            "//:is_linux_x86_64": "@pip_39_manylinux_2_17_x86_64_39_pylint//:pkg",
            "//conditions:default": "@pip_39_pylint//:pkg",
        },
    ),
)

Work towards #1606

@aignas
Copy link
Collaborator Author

aignas commented Feb 5, 2024

Right now the examples/bzlmod suceeds locally on a linux laptop:

bazel build @pip//pylint

However pyyaml is not available as a whl yet, hence the failure of the tests.

@aignas
Copy link
Collaborator Author

aignas commented Feb 29, 2024

This depends on #1750 and #1764. Will continue work on this once those get merged.

aignas added a commit to aignas/rules_python that referenced this pull request Mar 10, 2024
This is a variant of bazelbuild#1625 and was inspired by bazelbuild#1788. In bazelbuild#1625, we
attempt to parse the simple API HTML files in the same `pip.parse`
extension and it brings the follownig challenges:

* The `pip.parse` cannot be easily use in `isolated` mode and it may
  be difficult to implement the isolation if bazelbuild/bazel#20186
  moves forward.
* Splitting the `pypi_index` out of the `pip.parse` allows us to accept
  the location of the parsed simple API artifacts encoded as a bazel
  label.
* Separation of the logic allows us to very easily implement usage of
  the downloader for cross-platform wheels.
* The `whl` `METADATA` might not be exposed through older versions of
  Artifactory, so having the complexity hidden in this single extension
  allows us to not increase the complexity and scope of `pip.parse` too
  much.
* The repository structure can be reused for `pypi_install` extension
  from bazelbuild#1728.

TODO:
- [ ] Add unit tests for functions in `pypi_index.bzl` bzlmod extension if
  the design looks good.
- [ ] Changelog.

Out of scope of this PR:
- Further usage of the downloaded artifacts to implement something
  similar to bazelbuild#1625 or bazelbuild#1744. This needs bazelbuild#1750 and bazelbuild#1764.
- Making the lock file the same on all platforms - We would need
  to fully parse the requirements file.
- Support for different dependency versions in the `pip.parse` hub repos
  based on each platform - we would need to be able to interpret
  platform markers in some way, but `pypi_index` should be good already.
- Implementing the parsing of METADATA to detect dependency cycles.
- Support for `requirements` files that are not created via
  `pip-compile`.
- Support for other lock formats, though that would be reasonably
  trivial to add.

Open questions:
- Support for VCS dependencies in requirements files - We should
  probably handle them as `overrides` in the `pypi_index` extension and
  treat them in `pip.parse` just as an `sdist`, but I am not sure it
  would work without any issues.
aignas added a commit to aignas/rules_python that referenced this pull request Mar 16, 2024
The PR bazelbuild#1743 explored the idea of creating extra config settings for
each target platform that our toolchain is targetting, however that has
a drawback of not being usable in `bzlmod` if someone built Python for
a platform that we don't provide a toolchain for and tried to use the
`pip.parse` machinery with that by providing the
`python_interpreter_target`. That is a niche usecase, but `rules_python`
is a core ruleset that should only provide abstractions/helpers that
work in all cases or make it possible to extend things.

This explores a way to decouple the definition of the available
`config_settings` values and how they are constructed by adding an extra
`is_python_config_setting` macro, that could be used to declare the
config settings from within the `pip.parse` hub repo. This makes the
work in bazelbuild#1744 to support whl-only hub repos more self-contained.

Supersedes bazelbuild#1743.
@aignas aignas force-pushed the refactor/config-settings branch from 0565a23 to 67a3e8a Compare March 16, 2024 06:50
@aignas
Copy link
Collaborator Author

aignas commented Mar 18, 2024

This should merge #1811 as the right fix for platform specific wheel platform setting.

github-merge-queue bot pushed a commit that referenced this pull request Mar 18, 2024
The PR #1743 explored the idea of creating extra config settings for
each target platform that our toolchain is targetting, however that has
a drawback of not being usable in `bzlmod` if someone built Python for
a platform that we don't provide a toolchain for and tried to use the
`pip.parse` machinery with that by providing the
`python_interpreter_target`. That is a niche usecase, but `rules_python`
is a core ruleset that should only provide abstractions/helpers that
work in all cases or make it possible to extend things.

This explores a way to decouple the definition of the available
`config_settings` values and how they are constructed by adding an extra
`is_python_config_setting` macro, that could be used to declare the
config settings from within the `pip.parse` hub repo. This makes the
work in #1744 to support whl-only hub repos more self-contained.

Supersedes #1743.

---------

Co-authored-by: Richard Levasseur <rlevasseur@google.com>
@aignas
Copy link
Collaborator Author

aignas commented Mar 26, 2024

I'll close this for now as I focus on the implementation outlined in this design doc.

@aignas aignas closed this Mar 26, 2024
@aignas aignas deleted the feat/download-only-wheel-hubs branch October 17, 2024 02:51
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.

1 participant