Repository navigation
feat(settings): support explicit per-version wheel build numbers - #1391
mikedep333 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely identifies the main change: support for explicit per-version wheel build numbers in settings. |
| Linked Issues check | |
Issue #1390 is active and has coding requirements. The supplied PR evidence reports the build_number model, public-version lookup, nonnegative validation, explicit zero tags, hook suffixes, exact ca… |
| Out of Scope Changes check | |
The changed source files implement issue #1390. The documentation describes the new setting and its cache and installation rules. The tests verify the new tag, cache, and dependency behavior. No unrel… |
| Description check | |
The pull request description clearly describes the added per-package build numbers, cache behavior, dependency installation changes, validation scope, and related design context. It matches the change… |
Full details: Linked Issues check
-
Explanation
Issue
#1390is active and has coding requirements. The supplied PR evidence reports thebuild_numbermodel, public-version lookup, nonnegative validation, explicit zero tags, hook suffixes, exact cache matching acrossbootstrap,build, andbuild-sequence, and legacy behavior for unconfigured and pre-built wheels.PackageBuildInfo.build_tag()uses the explicit number when it is present.BuildEnvironmentuses a local-only wheel view with--reinstallwhen explicit mappings are active and validates installedWHEELmetadata. The added tests cover parsing, tag selection, cache matching, dependency filtering, transitive dependencies, missing wheels, and metadata validation.
Full details: Out of Scope Changes check
-
Explanation
The changed source files implement issue
#1390. The documentation describes the new setting and its cache and installation rules. The tests verify the new tag, cache, and dependency behavior. No unrelated change is demonstrated by the supplied scope summary.
Full details: Description check
-
Explanation
The pull request description clearly describes the added per-package build numbers, cache behavior, dependency installation changes, validation scope, and related design context. It matches the changeset.
- 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.
Comment @coderabbitai help to get the list of available commands.
0b7501b to
9b43f92
Compare
| cwd=str(self.path.parent), | ||
| network_isolation=False, | ||
| ) | ||
| with self._installation_wheel_args() as source_args: |
There was a problem hiding this comment.
[Discussion — Recommendation] — src/fromager/build_environment.py (line 274) and src/fromager/packagesettings/_settings.py (line 214)
Turning on build_number for ONE package flips a secret global switch that changes how EVERY package in the run installs its dependencies — and breaks a workflow that used to work.
How the switch works: if any package anywhere in your settings has a build_number entry — even for some old version nothing builds anymore — then for the whole rest of the run, dependency installs stop using the wheel server and go fully offline, local-wheels-only.
Two bad outcomes:
-
fromager build pkg --wheel-server-url https://cache.example/simple/breaks. That's a supported workflow: "fetch my deps from this external server." But the switch rips out--index-urland replaces it with--no-index --offline. In a fresh work dir, your local wheels folder is empty → boom,MissingDependencyfor stuff the external server was serving just fine yesterday. -
One stale config entry slows down everything, forever, invisibly. Leave
build_number: {"1.0": 0}in your settings for a package nobody builds anymore → every single dependency install in every run pays the filtered-mode tax (see Finding 2 for how big that tax is). And nothing is logged about the switch, so when things get slow or weird, good luck figuring out why.
To be fair: this strictness is partly necessary — the competing higher-build wheels might live on that external server, so "just keep the index" isn't safe either. And the docs do say "all dependency wheels must be available locally." That's why this is a discussion, not a "you're wrong." But the --wheel-server-url breakage is mentioned nowhere, and the docs say the switch activates when a number is "active" without defining "active" — a normal human would assume it means "for the thing I'm building right now." It doesn't. It means "exists anywhere in the config."
Cheapest fix: log the switch when it flips on, and define "active" in the docs:
if self._ctx.settings.has_explicit_build_numbers:
logger.info(
"explicit build numbers configured; installing dependencies "
"from filtered local wheel view only"
)There was a problem hiding this comment.
Agreed. When you posted this, I was also trying to figure out the trade-offs between enforcing exact dependency builds and preserving the configured wheel-server behavior.
The current filtering prevents uv from selecting another build of the same dependency version. However, activating it for any configured counter changes installation behavior across the invocation, including unrelated packages.
I see three options:
- Keep the current enforcement: guarantees the configured dependency builds, but requires all dependency wheels locally and introduces the repeated filtering overhead you identified. Logging and clearer documentation would make this behavior visible, but the workflow restriction remains.
- Preserve existing dependency installation: keep explicit numbers for wheel generation and cache selection, while deferring exact dependency-installation enforcement. This preserves remote-index workflows, but a dependency’s installed build can differ from its configured counter.
- Support exact installation through configured remote sources: select specific wheel artifacts while preserving source configuration. This needs a broader design covering transitive dependencies, authentication, and reused environments.
I’m leaning toward the second option for this feature: define build_number as controlling the wheel Fromager produces or reuses, and discuss exact dependency-installation enforcement separately.
There was a problem hiding this comment.
I’ve added the mode log and defined the activation rule in the settings API and concept docs: any loaded non-prebuilt package with a non-empty mapping activates it, including unused versions. The docs explicitly say installation does not fetch from --wheel-server-url.
I’m retaining this behavior in the proof of concept while #1392 compares all three options from my earlier reply. Logging does not remove the workflow restriction; the proposal also asks whether activation should follow the selected dependency set or a separate opt-in.
| logger.info("created build environment in %s", self.path) | ||
|
|
||
| @contextlib.contextmanager | ||
| def _installation_wheel_args(self) -> typing.Iterator[list[str]]: |
There was a problem hiding this comment.
[Recommendation] — src/fromager/build_environment.py (line 221, _installation_wheel_args)
Every single dependency install re-builds the entire "allowed wheels" folder from scratch — for a big bootstrap that's millions of wasted file operations.
Here's the mechanism: the filtered view is a temporary folder full of symlinks to the "good" wheels. Every time install() runs, the code globs every wheel in the downloads + prebuilt dirs, parses each filename, resolves paths, and creates a symlink for each one — then when the install finishes, the whole folder is deleted. Next install? Do it all again, from zero.
And install() is not rare — it runs 2–3 times per package (build-system deps, then backend/sdist deps). Fromager's whole job is bootstrapping thousands of packages, and the downloads dir accumulates every wheel ever built, including multiple build tags per version. Rough math for a 5,000-package run with ~6,500 accumulated wheels: about 65 million symlink creates plus 65 million unlinks, plus 65 million filename parses — tens of minutes of pure overhead doing literally the same work over and over.
The codebase already has a similar full-scan (server.update_wheel_mirror()), but that one only creates links for new files — it doesn't torch and rebuild everything each time. Freshness per install is needed (new wheels appear between installs), but you can get that with cheap invalidation instead of full demolition:
# instead of a fresh TemporaryDirectory per install():
snapshot = self._ctx.work_dir / "install-wheels"
if _snapshot_stale(self._ctx, snapshot): # compare mtimes/counts of downloads+prebuilt
_rebuild_snapshot(self._ctx, snapshot) # existing loop, clear+rebuild only when stale
yield ["--no-index", "--offline", "--find-links", str(snapshot), "--reinstall"]There was a problem hiding this comment.
This ties into my reply on the installation policy. I’m leaning toward deferring exact dependency-installation enforcement. If we take that option, this filtered wheel view and its repeated scanning would be removed from the PR.
There was a problem hiding this comment.
Implemented in 09fb462: the work context now keeps a filtered view per worker, shared across that worker’s build environments. It caches parsed filenames and creates/removes links only when selection changes. Separate views prevent one worker from changing another worker’s installation candidates.
It still scans filenames and rechecks settings/hooks before each install, so this removes repeated link creation and parsing, not all scanning overhead. Tests cover unchanged-view reuse, counter changes without filename changes, added/removed wheels, and parallel isolation. Large-collection performance still needs benchmarking.
| build_dir: BuildDirectory | None = None | ||
| """sub-directory with setup.py or pyproject.toml""" | ||
|
|
||
| build_number: dict[ |
There was a problem hiding this comment.
[Question] — src/fromager/packagesettings/_models.py (line 630, build_number field)
The old build counter could be different per variant (cpu vs. cuda); the new build_number is one number for everyone — is that permanent?
How it worked before: the numeric build tag came from counting changelog entries, and the global changelog is per-variant. So in a cpu/cuda setup, mypkg could legitimately be build 2 for cpu and build 5 for cuda. The new build_number field sits at the top level of PackageSettings — there's no spot for a variant-specific override — so one configured number applies to every variant in the run.
If that's the intent ("the external counter says this version is build 7, period"), it's fine and arguably exactly right. But it's a one-way door: this is a public settings schema, and if someone later needs per-variant explicit numbers, adding variant scoping after the fact is a breaking schema change for everyone who already wrote flat build_number entries.
So the question for the author: was variant-independence a deliberate call, and does it hold up for the multi-variant setups fromager supports? If yes, one sentence in the field's docstring saying "the number applies to all variants" would record the decision where the next reader will find it.
There was a problem hiding this comment.
The package-level number is deliberate in this proof of concept: it replaces both changelog contributions for every variant. I’ve documented that in the field docstring and concept page.
I’ve also raised variant overrides explicitly in #1392 rather than treating this as settled. An extension could keep the flat mapping as the default and add variant-specific overrides without invalidating existing settings.
| explicit = pbi.get_build_number(candidate_version) is not None | ||
| # An explicit zero is not an untagged wheel. Higher counters are | ||
| # cache misses for explicit settings, just as in bootstrap. | ||
| existing_build_tag = ( | ||
| build_tag_from_name if build_tag_from_name else (0, "") | ||
| build_tag_from_name if explicit or build_tag_from_name else (0, "") | ||
| ) | ||
| if ( | ||
| existing_build_tag[0] > build_tag[0] | ||
| not explicit | ||
| and existing_build_tag | ||
| and existing_build_tag[0] > build_tag[0] | ||
| and existing_build_tag[1] == build_tag[1] | ||
| ): |
There was a problem hiding this comment.
[Nit] — src/fromager/commands/build.py (lines 550–561, _is_wheel_built)
A dead condition and a three-way one-liner make this tricky cache logic harder to read than it needs to be.
Two things trip up a reader here:
- In
if not explicit and existing_build_tag and existing_build_tag[0] > build_tag[0] ..., the middle checkexisting_build_tagcan never be false — whenexplicitis false, that branch of the assignment always produces either a real tag or the fallback(0, ""), both truthy. A reader will assume it can be empty and go hunting for that case. It can't. build_tag_from_name if explicit or build_tag_from_name else (0, "")crams three outcomes into one expression: explicit-and-untagged gives(), legacy-untagged gives(0, ""), tagged gives the tag. The()vs(0, "")distinction is the subtle point of this whole feature (explicit zero ≠ untagged), and it's hiding inside a conditional expression.
The logic is correct — verified — but since this exact-match-vs-untagged distinction is the heart of the feature, spelling it out in branches makes it self-documenting:
| explicit = pbi.get_build_number(candidate_version) is not None | |
| # An explicit zero is not an untagged wheel. Higher counters are | |
| # cache misses for explicit settings, just as in bootstrap. | |
| existing_build_tag = ( | |
| build_tag_from_name if build_tag_from_name else (0, "") | |
| build_tag_from_name if explicit or build_tag_from_name else (0, "") | |
| ) | |
| if ( | |
| existing_build_tag[0] > build_tag[0] | |
| not explicit | |
| and existing_build_tag | |
| and existing_build_tag[0] > build_tag[0] | |
| and existing_build_tag[1] == build_tag[1] | |
| ): | |
| if explicit: | |
| # Exact match required: () means untagged, which can never | |
| # equal an explicit number (including zero). | |
| existing_build_tag = build_tag_from_name | |
| else: | |
| existing_build_tag = build_tag_from_name or (0, "") |
There was a problem hiding this comment.
Implemented in 09fb462. Explicit and legacy tags now have separate branches, and the redundant truthiness check is gone. Explicit zero remains distinct from an untagged wheel; the existing cache regressions pass.
| `2_cuda13_torch2.10`. With an explicit `build_number` of zero, the same | ||
| segments produce `0_cuda13_torch2.10`. | ||
|
|
||
| **Important notes:** |
There was a problem hiding this comment.
[Nit] — docs/customization.md (line 454, build_tag_hook notes) and src/fromager/build_environment.py (lines 232, 305)
The hook is now sometimes called with a fake, bare requirement — but the docs still imply it always gets the real one.
The new install-time filtering and post-install validation call wheels.get_build_tag(req=Requirement(name), ...) — a requirement reconstructed from just the package name, no extras, no marker, no URL. Meanwhile the build and cache paths pass the real requirement from the resolve graph.
Why that matters: a build_tag_hook that looks at anything beyond the name (say, req.extras or req.marker) will compute a different expected tag during install-time filtering than during the actual build — so wheels get filtered out or installs rejected for no visible reason. The docs say the hook must be "deterministic and independent of wheel contents, build environment, or ELF metadata" — which a hook reading req.extras technically satisfies today, yet it would still break under this PR.
The bare requirement is inherent to the design (those code paths iterate all local wheels / all installed dists, so there is no real requirement to thread through). Fine — but then the tightened contract should be written down. One extra bullet in the "Important notes" list:
- The suffix must depend only on the requirement name, version, variant, and
wheel_tags: during dependency-installation filtering, Fromager re-evaluates the hook with a bare requirement containing just the name.
There was a problem hiding this comment.
This also ties into my reply on the installation policy. Deferring exact dependency-installation enforcement would remove these installation-time hook calls with bare requirements. If we retain enforcement, this hook-contract question still needs addressing.
There was a problem hiding this comment.
I’ve documented the narrower contract for explicit-counter installation checks: the suffix must use only the name, version, variant, and wheel_tags, within the existing compatibility-tag restrictions. Those calls lack the original extras, markers, specifier, and URL.
This remains a design question in #1392: accept that restriction for enforcement, or preserve the original requirement context. The generation/cache-only option would avoid adding these installation-time hook calls.
| public = typing.cast(PackageVersion, Version(version.public)) | ||
| return self._ps.build_number.get(public) | ||
|
|
||
| def build_tag(self, version: Version) -> BuildTag: |
There was a problem hiding this comment.
[Nit] — src/fromager/packagesettings/_pbi.py (line 297, build_tag())
The method's behavior changed, its docstring first line was updated, but its version history wasn't — the repo's own convention (and CONTRIBUTING.md) says to record that.
PackageBuildInfo.build_tag() is a public method, and this PR changes its contract meaningfully: an explicit build_number for the version now takes precedence over the changelog count — including zero, which produces a real (0, "") tag where the changelog path would have produced an empty one. The docstring's summary line was updated to say so, but the .. versionchanged:: history still stops at 0.54.0. Since the API reference is generated from these docstrings, anyone reading the reference sees a changed method with no record of when or how.
The new get_build_number() and the build_number field both carry versionadded:: 0.102.0 (correct — last tag is 0.101.0); this method should carry the matching versionchanged:
| def build_tag(self, version: Version) -> BuildTag: | |
| .. versionchanged 0.102.0 | |
| An explicit ``build_number`` for the version takes precedence over | |
| the changelog count, including zero. |
There was a problem hiding this comment.
Added versionchanged:: 0.102.0 to PackageBuildInfo.build_tag(), documenting changelog precedence and explicit zero returning (0, ""). I also corrected the existing directive syntax so both entries render as version history.
| self.path, | ||
| ) | ||
|
|
||
| def _validate_installed_build_numbers(self) -> None: |
There was a problem hiding this comment.
[Nit] — src/fromager/build_environment.py (line 291, _validate_installed_build_numbers) vs. get_distributions()
The new validation re-implements the existing get_distributions() introspection inline — a second, slightly different copy of the same mechanism.
The inline python -c one-liner (json.dump([(d.metadata['Name'], d.version, d.read_text('WHEEL')) for d in distributions()], sys.stdout)) is a near-copy of get_distributions() a few lines away (json.dump({d.name: d.version for d in distributions()}, sys.stdout)) — same trick, different shape, no shared helper. Running a python -c snippet and JSON-decoding the result is an established pattern in this file, so the style itself is fine. The problem is having two divergent copies: a future fix applied to one — JSON error handling (the existing one wraps json.loads with logging on failure; the new one doesn't), or the d.metadata['Name'] vs d.name canonicalization difference — will silently miss the other.
One small helper returning (name, version, wheel_text) tuples serves both call sites:
| def _validate_installed_build_numbers(self) -> None: | |
| def _installed_distributions(self) -> list[tuple[str, str, str | None]]: | |
| """Return (name, version, WHEEL metadata text) for each installed dist.""" | |
| script = ( | |
| "import json,sys; from importlib.metadata import distributions; " | |
| "json.dump([(d.metadata['Name'], d.version, d.read_text('WHEEL')) " | |
| "for d in distributions()], sys.stdout)" | |
| ) | |
| return json.loads(self.run([str(self.python), "-c", script])) |
There was a problem hiding this comment.
See my reply on the installation policy. I’m leaning toward deferring exact dependency-installation enforcement, which would remove _validate_installed_build_numbers() and this duplication from the PR.
There was a problem hiding this comment.
Implemented the shared _installed_distributions() helper in 09fb462. Both callers now use the same name/version/WHEEL introspection, with JSON-decoding diagnostics in one place. get_distributions() preserves its sorted name-to-Version mapping.
| cwd=str(self.path.parent), | ||
| network_isolation=False, | ||
| ) | ||
| with self._installation_wheel_args() as source_args: |
There was a problem hiding this comment.
[Nit] — src/fromager/build_environment.py (line 274, install())
The code asks "are explicit build numbers on?" twice, and nothing forces the two answers to match.
Once inside _installation_wheel_args() — "should I filter the wheels uv sees?" — and once right after — "should I verify what got installed?". Two separate lookups of the same setting, at two separate moments.
Today they always agree, because the setting can't change mid-install. But if that ever stops being true (say the setting becomes version-aware, per the discussion on the mode switch, or tests swap the settings object mid-run — which they already do), the two checks can disagree: filter but don't verify = sneaky direct-URL installs slip through; verify but don't filter = failures that make no sense.
Fix: ask the question once, use the answer for both.
explicit = self._ctx.settings.has_explicit_build_numbers
with self._installation_wheel_args(explicit) as source_args:
...
if explicit:
self._validate_installed_build_numbers()There was a problem hiding this comment.
See my reply on the installation policy. If we defer exact dependency-installation enforcement, both of these mode checks would be removed from the PR.
There was a problem hiding this comment.
Implemented in 09fb462: install() computes the mode once, passes it to _installation_wheel_args(), and uses that same decision for post-install validation.
| return path | ||
|
|
||
|
|
||
| def test_uv_install_enforces_transitive_counters_in_reused_environment( |
There was a problem hiding this comment.
[Discussion — Nit] — tests/test_build_number.py (line 266, test_uv_install_enforces_transitive_counters_in_reused_environment)
One ~90-line test covers five different behaviors — and it's the first test in tests/ to spin up a real venv, real uv pip install, and a live HTTP wheel server.
Everything this one test asserts: (1) uv greedily picks build 2 when builds 0/1/2 are visible, (2) install fails when the allowed wheel is missing, (3) counters 0, 1 and 7 all install correctly across reconfigurations, (4) temp dirs get cleaned up, (5) a direct-URL reference is caught by the WHEEL metadata check. That's five "why did this fail?" stories bundled into one, against the repo's one-behavior-per-test convention — and when it breaks in the middle, you're debugging a half-finished narrative with a live venv and server to reproduce.
To be fair (why it's a discussion, not a demand): the core thing it tests — uv's greedy build-tag behavior and the direct-URL bypass — genuinely can't be mocked, the environment reuse is itself part of the scenario, and splitting into five tests means rebuilding the venv/server five times. Options: split with a session-scoped fixture (loses some reuse realism), keep as-is, or mark it as slow/integration so it's obvious it's a heavyweight scenario test rather than a normal unit test.
If the team is happy keeping it monolithic, a one-line comment above the test saying "this is intentionally one end-to-end scenario; see the asserts as a timeline" would help the next debugger.
There was a problem hiding this comment.
This depends on the scope decision in my reply on the installation policy. If we defer exact dependency-installation enforcement, this test would move out of this PR with that work. Your feedback on structuring the test would still apply when we revisit the feature.
There was a problem hiding this comment.
I kept the real uv regression as one scenario and added a docstring explaining the timeline. Reusing the same environment and installer cache is essential to checking stale installed builds, recovery after a missing expected wheel, and successive counter changes.
View reuse and parallel cleanup/isolation now have separate focused tests. I haven’t introduced a new integration marker; the scenario continues to run with the existing test suite.
|
@mikedep333 thank you for the attempt
|
|
Update: I have removed the references for now |
You mean this PR I opened?
Thank you for doing that. |
#1390 is a issue and not a PR. |
I understand you now. I will open up a feature proposal PR that adds a file under docs/proposals. EDIT: Done #1392 |
Add an optional `build_number` mapping keyed by public version. Explicit numbers replace combined variant and package changelog counts while keeping changelogs and `build_tag_hook` suffixes. Reject local-version keys and use the public version when looking up a requested local version. Require exact tags for configured versions in bootstrap and build cache checks, including numeric zero. Preserve legacy behavior for unconfigured versions and retain pre-built wheel tags. Keep exact dependency installation as a proof of concept pending the scope discussion in python-wheel-build#1392. Any loaded non-prebuilt counter activates local-only installation, including unused versions. Log this policy, reuse filtered views per worker, reinstall dependencies, and check installed WHEEL metadata before compilation. All required dependency wheels must be available locally. Related: python-wheel-build#1392 Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Mike DePaulo <mikedep333@redhat.com>
9b43f92 to
09fb462
Compare
iangelak
left a comment
iangelak
left a comment
There was a problem hiding this comment.
Sorry I am late to the party. I left two comments.
| link.symlink_to(path.resolve()) | ||
| # uv treats build tags as preferences, not constraints. Keep other | ||
| # builds out of its candidate set, including its installed/cache state. | ||
| yield ["--no-index", "--offline", "--find-links", temporary, "--reinstall"] |
There was a problem hiding this comment.
With setuptools 70.0.0 configured at build 1 and builds 1 and 2 available, download-sequence --include-wheels stages only build 2 because it uses resolve_cached_wheel's first candidate. This installation view then excludes that wheel, so step prepare-build cannot satisfy setuptools==70.0.0 even though the correct build exists on the cache server. Please apply exact configured-tag selection during prefetch so it supplies wheels the installer accepts
Add an optional per-package
build_numbermapping from public versions to nonnegative integers:Configured numbers replace combined variant and package changelog counts for all variants.
build_tag_hookstill appends suffixes. Local-version keys are rejected; requested versions with local suffixes use their public version for lookup. Unconfigured versions retain changelog-derived tags and cache behavior, and pre-built wheels retain their tags.Explicit numbers require complete expected build tags in local and remote caches across
bootstrap,build, andbuild-sequence. Zero produces a numeric0tag; missing or different tags, including higher numbers, are cache misses.This remains a working proof of concept for the discussion in #1392, which supersedes issue #1390. The proposal presents three installation options and leaves the choice open for upstream discussion. This implementation retains exact dependency enforcement while that decision is open:
--wheel-server-url; all required dependency wheels must already be available locally.WHEELmetadata is checked before compilation, including for direct references that bypass filtering.Variant overrides, enforcement activation, and installation-time hook inputs remain design questions in #1392. Related: #1360 for broader legacy cache semantics.
Validation: 327 focused tests passed. The real uv regression reproduces build 2 winning from a shared HTTP index, then verifies builds 0, 1, and 7 in a reused environment; missing expected wheels and unexpected direct references fail. Additional regressions cover view reuse, changed settings and wheels, worker isolation, and logging for unused counters.
The existing Stevedore source-build validation for counters 0 and 1 previously passed filename and embedded
WHEELmetadata checks with global and package changelogs present. It was not rerun for these review fixes. Large-collection performance has not been benchmarked.