Repository navigation
Conversation
Resolve and download fresh prebuilt wheels through the selected source resolver. Preserve local wheel cache reuse, legacy prebuilt downloads, prebuilt hooks, and the test-mode fallback. Add focused coverage for both configured profiles, build-order replay, and legacy behavior. Closes python-wheel-build#1208 Co-Authored-By: Codex <noreply@openai.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
Overall this is really solid PR and the core of the change is right. I verified the settings validation too: all four source: + legacy pre_built mixing combinations correctly raise, so the precedence change in PackageBuildInfo.pre_built is safe.
I have added few comments/suggestions. Let me know what do you think.
| req=req, | ||
| cache_server_url=self.ctx.wheel_server_url, | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
The expected failure here is resolvelib.resolvers.ResolverException (raised at resolver.py:880), which is also the commoncase on a first encounter, so exc_info=True at debug will be noisy. Can we consider narrowing it?
| age_fallback=age_fallback, | ||
| ) | ||
|
|
||
| if not results and not pre_built: |
There was a problem hiding this comment.
find_all_matching_from_provider raises rather than returning empty (resolver.py:880), and age_fallback is never NONE on this path, so this warning was already effectively unreachable. Adding and not pre_built doesn't change behavior but does make a reader think it does.
| ) -> PreparedSourceData: | ||
| """Background-safe prebuilt download: no Bootstrapper state accessed.""" | ||
| """Download a prebuilt wheel, reusing local cache or test-mode fallback.""" | ||
| # Thread-safe: paths include {req.name}-{resolved_version} (unique per package), |
There was a problem hiding this comment.
This same policy: configured + prebuilt → download_prebuilt_wheel;
configured + not prebuilt → raise; no resolver → download_wheel now exists three times:
here, build.py:366-384, and download_sequence.py:72-92. Three different error strings, three
different output directories (wheels_prebuilt / wheels_build / wheels_prebuilt), and the
local-URL branch below exists only here.
Suggestion: Fold the policy into wheels.download_prebuilt_wheel():
def download_prebuilt_wheel(
*, ctx, req, version, download_url, output_directory: pathlib.Path | None = None
) -> pathlib.Path:
"""Fetch a prebuilt wheel via the configured resolver, or the legacy downloader."""with the capability check and legacy fallback inside. Each call site becomes one line and the
error message is written once.
| downloaded: packagesettings.DownloadedSource = sources.download_source( | ||
| ctx=ctx, req=req, version=version, download_url=download_url | ||
| ) | ||
| if downloaded.kind is not packagesettings.DownloadKind.prebuilt_wheel: |
There was a problem hiding this comment.
This guard is unreachable. PyPIPrebuiltResolver.download() hardcodes DownloadKind.prebuilt_wheel (_resolver.py:304), and _download_from_hook() already validates against download_kinds (_resolver.py:900-906), which for both prebuilt resolvers is frozenset({prebuilt_wheel}).
line 536 is in the uncovered set after running tests/test_wheels.py. And test_configured_prebuilt_rejects_other_kind matches "unsupported kind 'sdist'", which is the
resolver's message, not this one: the test passes while the guard it claims to cover never runs
Suggestion: Either delete the guard (the resolver contract already enforces it) or make the test exercise it
directly.
| f"{req}: configured prebuilt source returned {downloaded.kind.value!r}; " | ||
| "expected 'prebuilt_wheel'" | ||
| ) | ||
| wheel_path = downloaded.path |
There was a problem hiding this comment.
- This only fires for
hook-prebuiltwith a custom hook:_download()already lands inwheels_prebuilt. The right home isAbstractHookResolver._download_from_hook()(_resolver.py:845-905), which already knowslegacy_kind is prebuilt_wheeland is where the non-hook path's destination is decided. Doing it there collapses this whole function to a one-liner oversources.download_source()and removes the newwheels→sourcesimport - Unconditional
copy2unlike every other download path indownloads.py, which short-circuits onexists(). For a fromager workload this re-copies multi-GB CUDA/torch wheels on every run and doubles disk for each. Anos.link()with copy fallback, or an existence check, would be better. - The hook's original file is left behind with no cleanup.
| requirements_file, | ||
| resolver, | ||
| sbom, | ||
| sources, |
There was a problem hiding this comment.
There is no cycle today (sources doesn't import wheels), but this sets us one import away from getting into a cycle, and sources is the heavier module. Moving the staging logic intoc_download_from_hook() removes the need for this entirely
| # moved to a new directory. | ||
| wheel_filename = wkctx.wheels_downloads / wheel_filename.name | ||
| # The mirror moves wheels from the build directory to downloads; | ||
| # configured prebuilt wheels stay in the prebuilt directory. |
There was a problem hiding this comment.
The guard is correct : update_wheel_mirror only moves out of wheels_build. But the net effect
is that the same conceptual artifact now lands in three places depending on mode and phase:
wheels-repo/prebuilt— bootstrap, both modeswheels-repo/downloads— build-sequence, legacy prebuilt (line 379 targetswheels_build)wheels-repo/prebuilt— build-sequence, configured prebuilt (new)
This also changes what fromager build <pkg> prints on stdout (line 125), which is a scripted
interface. Could we pick one location for prebuilt wheels across both modes and document it?
wheels_prebuilt looks right, which would mean changing the legacy branch too
|
|
||
|
|
||
| @pytest.mark.parametrize("provider", ["pypi-prebuilt", "hook-prebuilt"]) | ||
| def test_bootstrap_prebuilt_uses_configured_download( |
There was a problem hiding this comment.
I think these four tests this one plus test_bootstrap_configured_prebuilt_reuses_local_wheel (:56),
test_bootstrap_prebuilt_rejects_source_only_resolver (:80) and
test_bootstrap_records_configured_prebuilt_in_graph_and_order (:286) exercise
_cache.bg_prepare_prebuilt and the Start/PrepareSource/ProcessInstallDeps phases, not
commands so they belong in tests/test_bootstrapper.py
| logger.info(f"using pre-built wheel for {req_type} requirement") | ||
| wheel_filename = wheels.download_wheel(req, wheel_url, ctx.wheels_prebuilt) | ||
| source_resolver = ctx.package_build_info(req).source_resolver | ||
| local_wheel_url = bool(ctx.wheel_server_url) and wheel_url.startswith( |
There was a problem hiding this comment.
local_wheel_url is computed before the force_legacy_download branch that ignores it. Also uses rstrip("/") + "/" while the equivalent check at build.py:620 is a bare url.startswith(wkctx.wheel_server_url). I wonder whether we need a shared helper here?
| download_url=entry["source_url"], | ||
| ) | ||
| elif source_resolver is not None: | ||
| raise ValueError( |
There was a problem hiding this comment.
reset(token) only runs at the end of download_one. The prebuilt branch's return at :95 already skips it (pre-existing), and the new raise ValueError at :84 skips it too. Since this runs on a pooled thread, the next task on that thread logs under the wrong requirement. Can we add try/finally around the block?
Summary
pypi-prebuiltandhook-prebuiltas prebuilt packages in bootstrap, the dependency graph, and build order.download()contract.pre_builtbehavior, and test-mode fallback.Validation
git diff --checkpassed.Closes #1208