-
-
Notifications
You must be signed in to change notification settings - Fork 574
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
refactor(pypi): split out more utils and start passing 'abi_os_arch' around #2069
refactor(pypi): split out more utils and start passing 'abi_os_arch' around #2069
Conversation
currently stacked on #2068 |
…around This is extra preparation needed for bazel-contrib#2059. Summary: - Create `pypi_repo_utils` for more ergonomic handling of Python in repo context. - Split the resolution of requirements files to platforms into a separate function to make the testing easier. This also allows more validation that was realized that there is a need for in the WIP feature PR. - Make the code more robust about the assumption of the target platform label. Work towards bazel-contrib#260, bazel-contrib#1105, bazel-contrib#1868.
42b8510
to
f2601eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some comments to hopefully make the review easier.
requirements_by_platform = pip_attr.requirements_by_platform, | ||
requirements_linux = pip_attr.requirements_linux, | ||
requirements_lock = pip_attr.requirements_lock, | ||
requirements_osx = pip_attr.requirements_darwin, | ||
requirements_windows = pip_attr.requirements_windows, | ||
extra_pip_args = pip_attr.extra_pip_args, | ||
requirements_by_platform = requirements_files_by_platform( | ||
requirements_by_platform = pip_attr.requirements_by_platform, | ||
requirements_linux = pip_attr.requirements_linux, | ||
requirements_lock = pip_attr.requirements_lock, | ||
requirements_osx = pip_attr.requirements_darwin, | ||
requirements_windows = pip_attr.requirements_windows, | ||
extra_pip_args = pip_attr.extra_pip_args, | ||
python_version = major_minor, | ||
logger = logger, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main idea is to normalize the pip_attr.requirements*
attributes to a dict[Label, list[str]]
. This makes the downstream code much simpler.
@@ -298,7 +303,7 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s | |||
|
|||
requirement = select_requirement( | |||
requirements, | |||
platform = repository_platform, | |||
platform = None if pip_attr.download_only else repository_platform, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If pip_attr.download_only == True
then we want to just select the first requirement
out of requirements
array. Previously the code was burried and we were carrying to much data around. The simplification here means that select_requirement
does not need to care about whe download_only
value anywhere.
for p in plats: | ||
configured_platforms[p] = file | ||
|
||
for file, plats in requirements_by_platform.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code above just got moved to a different file. The tests got also moved.
@@ -325,7 +179,6 @@ def parse_requirements( | |||
requirement_line = requirement_line, | |||
target_platforms = [], | |||
extra_pip_args = extra_pip_args, | |||
download = len(platforms) > 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute got removed as it is no longer needed due to the changes in the select_requirement
function.
if target_platforms: | ||
for p in target_platforms: | ||
if not p.startswith("cp"): | ||
fail("target_platform should start with 'cp' denoting the python version, got: " + p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an extra validation of the internal model invariants.
} | ||
return list(platforms.keys()) | ||
|
||
def _platform(platform_string, python_version = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code has been lifted from parse_requirements.bzl
without any changes except for addition of _platform
function, which normalizes the output of the main function to always return cp311_os_arch
type of values instead of the previous os_arch
. That gets consumed properly in the render_pkg_aliases.bzl
. If this is something that is still hard to review, I can split it out.
@@ -52,33 +52,17 @@ bar==0.0.1 --hash=sha256:deadb00f | |||
|
|||
_tests = [] | |||
|
|||
def _test_fail_no_requirements(env): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests got simplified in this function because I could split the functions.
]: | ||
env.expect.that_dict(got).contains_exactly({ | ||
"requirements_lock": [ | ||
"cp311_linux_aarch64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, that only when python_version
is passed we are adding the cp311_
to the beginning of the platforms.
Rebased on |
It seems that a few things broke in recent commits: - We are not using the `MODULE.bazel.lock` file and it seems that it is easy to miss when the components in the PyPI extension stop integrating well together. This happened during the switch to `{abi}_{os}_{plat}` target platform passing within the code. - The logger code stopped working in the extension after the recent additions to add the `rule_name`. - `repo_utils.getenv` was always getting `PATH` env var on bazel `6.x`. This PR fixes both cases and updates docs to serve as a better reminder. By fixing the `select_whls` code and we can just rely on target platform triples (ABI, OS, CPU). This gets one step closer to maybe supporting optional `python_version` which would address #1708. Whilst at it we are also adding different status messages for building the wheel from `sdist` vs just extracting or downloading the wheel. Tests: - Added more unit tests and brought them in line with the rest of the code. - Checked manually for differences between the `MODULE.bazel.lock` files in our `rules_python` extension before #2069 and after this PR and there are no differences except in the `experimental_target_platforms` attribute in `whl_library`. Before this PR you would see that we do not select any wheels for e.g. `MarkupSafe` and we are always building from `sdist`. Work towards #260.
This is extra preparation needed for #2059.
Summary:
pypi_repo_utils
for more ergonomic handling of Python in repo context.to make the testing easier. This also allows more validation that was realized
that there is a need for in the WIP feature PR.
Work towards #260, #1105, #1868.