Skip to content

fix(resolver): honor configured prebuilt wheel resolvers - #1393

Open
smoparth wants to merge 1 commit into
python-wheel-build:new-resolver-configfrom
smoparth:codex/issue-1208-prebuilt-resolvers
Open

fix(resolver): honor configured prebuilt wheel resolvers#1393
smoparth wants to merge 1 commit into
python-wheel-build:new-resolver-configfrom
smoparth:codex/issue-1208-prebuilt-resolvers

Conversation

@smoparth

@smoparth smoparth commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Treat pypi-prebuilt and hook-prebuilt as prebuilt packages in bootstrap, the dependency graph, and build order.
  • Resolve fresh wheels with the selected source provider and download them through its download() contract.
  • Use the same configured download path during build and download-sequence replay, while preserving prebuilt hooks, the local wheel mirror, cache reuse, legacy pre_built behavior, and test-mode fallback.
  • Update the resolver and bootstrap documentation.

Validation

  • 573 tests passed across the affected test files.
  • Scoped Ruff and mypy checks passed.
  • git diff --check passed.
  • Full and end-to-end suites were not run locally.

Closes #1208

@smoparth
smoparth requested a review from a team as a code owner October 23, 2026 21:23
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mergify mergify Bot added the ci label Oct 23, 2026
rd4398
rd4398 requested changes Oct 25, 2026
View reviewed changes

@rd4398 rd4398 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/fromager/bootstrap_requirement_resolver.py Outdated
Comment thread src/fromager/bootstrap_requirement_resolver.py Outdated
Comment thread src/fromager/bootstrapper/_cache.py
Comment thread src/fromager/wheels.py Outdated
Comment thread src/fromager/wheels.py
Comment thread src/fromager/wheels.py
Comment thread src/fromager/commands/build.py
Comment thread tests/test_commands.py Outdated
Comment thread src/fromager/bootstrapper/_cache.py Outdated
Comment thread src/fromager/commands/download_sequence.py Outdated
@smoparth
smoparth force-pushed the codex/issue-1208-prebuilt-resolvers branch from 690ff3e to e16b3c4 Compare October 26, 2026 17:49
@smoparth
fix(resolver): honor configured prebuilt wheel resolvers
c9cc5dd 
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 <[email protected]>

Signed-off-by: Shanmukh Pawan <[email protected]>
@smoparth
smoparth force-pushed the codex/issue-1208-prebuilt-resolvers branch from e16b3c4 to c9cc5dd Compare October 26, 2026 18:03
rd4398
rd4398 approved these changes Oct 26, 2026
View reviewed changes

@rd4398 rd4398 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing the feedback! The changes look good to me. I am approving the PR. Please file follow-up issues and resolve threads once done.

@jenlevy

jenlevy commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Looked through the resolver, download, preparation, and prebuilt-wheel paths end to end. The DownloadedSource/DownloadKind boundary makes the artifact handling much clearer, and the legacy compatibility and cache behavior are well covered. No blocking concerns from me.

@jenlevy

jenlevy commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

One clarifying question I have (and this is a nit not a blocker) - DownloadedSource appears to be introduced in this MR, but its docstring says versionadded:: 0.96.0, while the other new APIs use 0.102.0. Should this be updated to 0.102.0?

LalatenduMohanty
LalatenduMohanty requested changes Oct 26, 2026
View reviewed changes
Comment thread src/fromager/bootstrapper/_cache.py
)
unpack_dir = _create_unpack_dir(ctx.work_dir, req, resolved_version)
if ctx.package_build_info(req).source_resolver is not None:
server.update_wheel_mirror(ctx, preferred_prebuilt_wheel=wheel_filename)

@LalatenduMohanty LalatenduMohanty Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With this code we will choose one wheel file, but then use and serve a different copy.

  • Imagine two folders contain pkg-1.0.whl. The filenames match, but one copy has different code or dependencies.
  • Bootstrap selects the copy served by the local package server. The downloader finds the same filename in another folder and reuses that copy without checking its contents.
  • This PR then makes the local server serve that other copy too. Builds can silently receive different code or dependencies.

The fix is to reuse the exact file that was selected, rather than assuming matching filenames mean matching contents.

@LalatenduMohanty LalatenduMohanty Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The earlier part happens in wheels.py, lines 510–519: an existing same-named file is reused without downloading the selected URL. That behavior predates this PR

This was referenced Oct 26, 2026
Open
Open
@smoparth

smoparth commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

One clarifying question I have (and this is a nit not a blocker) - DownloadedSource appears to be introduced in this MR, but its docstring says versionadded:: 0.96.0, while the other new APIs use 0.102.0. Should this be updated to 0.102.0?

@jenlevy , this work is on feature branch so there is no correct version number currently when we merge to main we will update the version through these code changes.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LalatenduMohanty LalatenduMohanty LalatenduMohanty requested changes
@rd4398 rd4398 rd4398 approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

ci

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smoparth @jenlevy @LalatenduMohanty @rd4398