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

fix(bzlmod): keep the lockfile platform independent when resolving python #2135

Merged

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Aug 20, 2024

Before this PR the lockfile would become platform dependent when the
requirements file would have env markers. This was not caught because
we do not have MODULE.bazel.lock checked into the rules_python
repository because the CI is running against many versions and the lock
file is different, therefore we would not be able to run with
bazel build --lockfile_mode=error.

With this change we use the label to BUILD.bazel which is living next
to the python symlink and since the BUILD.bazel is the same on all
platforms, the lockfile will remain the same.

Summary

  • refactor(uv): create a reusable macro for using uv for locking reqs.
  • test(bzlmod): enable testing the MODULE.bazel.lock breakage across platforms.
  • test(bzlmod): use a universal requirements file for 3.9.
    This breaks the CI, because the python interpreter file hash is added to the lock file.
  • fix(bzlmod): keep the lockfile platform independent when resolving python

Fixes #1105 and #1868 for real this time.
Implements an additional helper for #1975.

@aignas
Copy link
Collaborator Author

aignas commented Aug 20, 2024

@fmeum do you have a better idea of how we could effectively do
mctx.path(python_interpreter_label) without writing the file resulting file
hash to the lockfile? In this case we do not care about the python interpreter
and the code would work equally well even if it was python3 (taken from the
PATH).

@rickeylev, do you have ideas how we could test this? Should we have one of the
examples using the lockfile and --lockfile_mode=error? I am not fully sure
how that plays with testing against the latest_green.

@fmeum
Copy link
Contributor

fmeum commented Aug 20, 2024

I don't see a better way at the moment. We should probably add a watch parameter to mctx.path though.

Cc @Wyverald

@Wyverald
Copy link
Member

We should probably add a watch parameter to mctx.path though.

Yeah, that seems fair.

@aignas aignas force-pushed the fix/no-lockfile-entry-for-python-interpreter branch 7 times, most recently from 19698d6 to 777d7ed Compare August 21, 2024 12:19
@aignas aignas marked this pull request as draft August 21, 2024 12:22
@aignas aignas force-pushed the fix/no-lockfile-entry-for-python-interpreter branch from e2b0f00 to 65f6efa Compare August 21, 2024 13:16
@aignas aignas marked this pull request as ready for review August 21, 2024 13:20
@rickeylev
Copy link
Contributor

re: testing: I think --lockfile_mode=error in the example is OK. The other option is an integration test that...I guess just sets --lockfile_mode? If there's something in particular to look for in the output, the python runner can be used to read the lock file and look for that.

I think we'll just have to see how it plays out with the downstream bazel testing. If it causes too many bogus failures, then we can factor out a smaller test under a separate CI job that sets skip_in_bazel_downstream_pipeline.

Copy link
Contributor

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Functionally, looks fine, so approved. Just comments about some doc phrasing and code reuse.

CHANGELOG.md Outdated Show resolved Hide resolved
python/private/pypi/pypi_repo_utils.bzl Show resolved Hide resolved
load("//python:py_binary.bzl", "py_binary")
load("//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED") # buildifier: disable=bzl-visibility

_REQUIREMENTS_TARGET_COMPATIBLE_WITH = select({
Copy link
Contributor

Choose a reason for hiding this comment

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

Does uv only provide linux/mac binaries...?

Copy link
Collaborator Author

@aignas aignas Aug 22, 2024

Choose a reason for hiding this comment

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

No, windows binaries are also provided, just the genrule implementation is in shell, however, I have decided to remove the select here and just enable this in bzlmod.

python/uv/private/pin.bzl Outdated Show resolved Hide resolved
docs/BUILD.bazel Outdated Show resolved Hide resolved
docs/BUILD.bazel Outdated Show resolved Hide resolved
Also:
- Fix the lock file rule to take into account the currently existing
  lockfiles.
- Use a transition to allow for a smart selection of the python version
  to generate the lock file for.
Summary:
- separate target to update the pip requirements
- Use a universal requirements file
…thon

Before this PR the lockfile would become platform dependent when the
`requirements` file would have env markers. This was not caught because
we do not have MODULE.bazel.lock checked into the `rules_python`
repository because the CI is running against many versions and the lock
file is different, therefor we would not be able to run with
`bazel build --lockfile_mode=error`.

With this change we use the label to `BUILD.bazel` which is living next
to the `python` symlink and since the `BUILD.bazel` is the same on all
platforms, the lockfile will remain the same.

Work towards bazelbuild#1105, bazelbuild#1868.
- Improve the CHANGELOG wording.
- Rename `pin` to `lock` and set the load visibility to internal.
- Retain the same naming convention of `requirements.update`.
- Use `py_binary` with the transitions.
- Add a simple validation to the `mrctx.watch` wrapping.
@aignas aignas force-pushed the fix/no-lockfile-entry-for-python-interpreter branch from 05e8f10 to 31c82ba Compare August 22, 2024 11:39
@aignas aignas added this pull request to the merge queue Aug 22, 2024
Merged via the queue into bazelbuild:main with commit 5eff339 Aug 22, 2024
4 checks passed
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.

pip_parse does not respect requirement markers
5 participants