refactor/fix: store dists in parse_requirements output #1917
Conversation
This means that we can have a function that then later adds data to the parse_requirements return result.
… of a single candidate
aignas
changed the title
|
I tested #1930 this with the latest commit 552c526. Got error: |
With this we should not get failures, but we would get different wheels, which from the user point of view might be better experience. I have also rewrote the tests to better test things and be more succinct. The complex thing here is that we want to do some filtering of the whls but we should not do too much a we are doing what the select should be doing.
|
With 348ca06 it looks like the credential helper is no longer working: I would only see this prompt if my cred helper script was broken or I remove |
|
Well credential helper not working is weird, this is definitely unintended. I thought I haven't touched the code there. @dougthor42, could you try |
|
For the record, all I'm doing to test these things is changing the git_override(
module_name = "rules_python",
commit = "8791cbbaa2336e24555a6b577ed6e19df24d7d88",
remote = "https://github.com/bazelbuild/rules_python",
)
git_override(
module_name = "rules_python_gazelle_plugin",
commit = "8791cbbaa2336e24555a6b577ed6e19df24d7d88",
remote = "https://github.com/bazelbuild/rules_python",
strip_prefix = "gazelle",
)At first I thought there might be something else between 8791cbb..348ca06 that caused the cred helper to fail, but I didn't have the issue in the previous commit 552c526 so it was definitely a change in 348ca06. I forgot to mention it, but going from 8791cbb to 348ca06 also caused a bunch of debug statements to show up for various packages. Example:
Sure. Same result for 348ca06 (debug messages and credential helper failure) For 0261471:
I'm running test now, but a fresh test can take like 30 minutes (which is why we need to bazel-ify our code!) |
|
Ah, yeah, the DEBUG statements is what I was looking for, but was not sure if they were present. Thanks for debugging, really appreciate it, I must have broken something here. |
|
Added extra params to quiet = False,
verbosity = "TRACE",You can select the verbosity from Maybe you could later upload some of the trace/debug logs if they are not sensitive. |
There was a problem hiding this comment.
LGTM, I think. Nothing in particular stuck out to me as needing significant changes.
re: conversation about cred helper failing: I don't see why this PR would cause that. Maybe the index-url vs extra-index-url? IIRC, Doug has a private index, so maybe that is getting mixed up somewhere, somehow.
| want_abis(list[str]): A list of ABIs that are supported. | ||
| want_platform(str): The target platform. | ||
| want_platforms(str): The platforms | ||
|
|
||
| Returns: | ||
| None or a struct with `url`, `sha256` and `filename` attributes for the |
There was a problem hiding this comment.
nit: I think the doc is now "returns a list of structs ..."
|
|
||
| Args: | ||
| whls(list[struct]): A list of candidates. | ||
| want_version(str): An optional parameter to filter whls by version. Defaults to '3.0'. |
There was a problem hiding this comment.
Is this version referring to python version? If so, please clarify in the doc or arg name.
| if tag.startswith("cp3") or tag.startswith("py3"): | ||
| version = int(tag[len("..3"):] or 0) | ||
| else: | ||
| # tag.startswith("cp2") or tag.startswith("py2") |
There was a problem hiding this comment.
Commented out debug code, or cryptic comment? In any case, please delete or clarify
|
|
||
| platform_tags = list({_LEGACY_ALIASES.get(p, p): True for p in parsed.platform_tag.split(".")}) | ||
| supported_implementations = {} |
There was a problem hiding this comment.
Ah, is this dict acting like a set? Just drop a comment. I really wish starlark had a set type
| supported_implementations = {} | |
| # dict, but used as a set. | |
| supported_implementations = {} |
| candidates = { | ||
| parse_whl_name(w.filename).platform_tag: w | ||
| for w in whls | ||
| if "musllinux_" not in w.filename |
There was a problem hiding this comment.
I'm guessing the musllinux gets special cased because we don't have way to model this yet?
python/private/bzlmod/pip.bzl
Outdated
| want_platform = repository_platform, | ||
| ) or sdist | ||
| ) or (requirement.sdists[0] if requirement.sdists else None) |
There was a problem hiding this comment.
Is the 0th sdist special? Or is this just picking an arbitrary element to have something valid?
|
|
||
| load(":whl_target_platforms.bzl", "select_whls") | ||
|
|
||
| def parse_requirements_add_dists(requirements_by_platform, index_urls, python_version): |
There was a problem hiding this comment.
Would this be better put in parse_requirements.bzl? It just seems closely coupled to it.
python/private/bzlmod/pip.bzl
Outdated
| @@ -261,11 +285,22 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s | |||
| whl_library_args.update({k: v for k, (v, default) in maybe_args_with_default.items() if v == default}) | |||
|
|
|||
| if requirement.whls or requirement.sdists: | |||
| logger.debug("Selecting a compatible dist for {} from dists:\n{}".format( | |||
There was a problem hiding this comment.
Some extra format() calls probably aren't a big deal, but serializing a json object as part of a debug call is a bit much. Perhaps passing a lambda instead, to defer evaluation?
Also, repo_utils.debug_print exists to base it upon an env var
Correct, we use a private repo on Google Artifact Registry and then also public PyPI. I'm still trying to get the Ops people to use a virtual repo that is a union of public pypi and private... but I digress. Here's our current pip.parse: pip.parse(
experimental_extra_index_urls = ["REDACTED/simple"],
experimental_index_url = "https://pypi.org/simple",
hub_name = "pypi",
python_version = "3.11",
requirements_lock = "//:requirements.txt",
)
That should be doable, albeit with some In general, those new params to
Looks good! build and test ran successfully and the installed version of |
decided to move this to the same file and rewire the dependency injection so that the struct stays immutable, removing the need for explanations
|
Great to hear, thanks for being the tester here! I think once this is merged we can start looking into #1837. I have locally stacked it on top of the 1837 and it seems to work for the usecases I have so it will be interesting to hear if I missed something there. |
This moves some of the code out of the
pip.bzlextension and changesthe layout of the code to prepare for multi-platform whl support.
Summary:
a function to populate the lists. Not sure if there is a better way to
do this.
the target platform filtering correctly.
select_whlintoselect_whls, which filtersthe whls (this can be used later in multi-platform selects) and
select_whl , which just is used get the most appropriate whl for the
host platform.
select_whl, which would result inPython 3.12 wheels being selected on Python 3.11 interpreters because
we were not taking into account the interpreter tag when doing the
filtering.
Fixes #1930