feat(wheels): add configurable build tag hook for wheel filenames#1217
feat(wheels): add configurable build tag hook for wheel filenames#1217shifa-khan wants to merge 1 commit into
Conversation
Allow downstream projects to append environment-specific suffixes (e.g. OS, accelerator) to wheel build tags via a user-defined hook. The hook is configured in settings.yaml under `wheels.build_tag_hook` and receives the build context, requirement, version, and wheel tags. Cache lookups and build-tag validation are updated to account for suffixed wheels. No behavior change when the hook is not configured. Co-Authored-By: Claude <claude@anthropic.com> Closes: python-wheel-build#1181 Signed-off-by: shifa-khan <shikhan@redhat.com>
📝 WalkthroughWalkthroughAdds a Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fromager/finders.py (1)
170-195: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReturning only one candidate breaks the new multi-prefix search.
With
build_tag=(2, ""), this now searches both2-and2_, butcandidate_basesis asetand the function still returns the first matching wheel. If bothpkg-1.0-2-...whlandpkg-1.0-2_<suffix>-...whlexist, callers only validate that one result and can miss the valid suffixed wheel entirely. Please keep searching until a candidate survives build-tag validation, or return ordered candidates instead of a single arbitrary match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fromager/finders.py` around lines 170 - 195, The wheel lookup in find_wheel_candidate is stopping at the first match from candidate_bases, which is now unsafe because the build-tag search can yield both plain and suffixed prefixes. Update the search in src/fromager/finders.py so it does not return a single arbitrary wheel from the first prefix match; instead, keep iterating through all matching wheels/candidates until one passes build-tag validation, or collect and return ordered candidates for the caller to evaluate. Use the existing find_wheel_candidate logic and the candidate_bases/build_tag_prefixes handling as the place to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fromager/bootstrapper.py`:
- Around line 992-997: The broad cache-lookup exception handling around
wheels.get_build_tag() is masking build_tag_hook validation failures as
recoverable cache misses. Update the try/except logic in the bootstrapper code
path that calls wheels.get_build_tag() so hook validation errors are not caught
by the generic except Exception and are allowed to fail the build, while still
preserving cache-miss handling for genuine lookup issues; apply the same fix to
both call sites in the bootstrapper flow.
- Around line 948-956: Normalize build tags before comparing in the bootstrapper
cache check so empty tags are treated consistently with the build verification
path. Update the logic around expected_build_tag in the bootstrapper flow to
normalize falsy values to the same canonical form used in
src/fromager/commands/build.py (for example via the existing build-tag helper or
equivalent normalization) before comparing against actual_build_tag. Apply the
same fix anywhere the bootstrapper repeats this check, including the other
build-tag comparison block, so cached wheels are rejected whenever their
normalized tag differs.
In `@src/fromager/commands/build.py`:
- Around line 498-504: The build-tag resolution in build.py is swallowing errors
from wheels.get_build_tag(), causing invalid suffix segments or build_tag_hook
failures to fall back to a cached-wheel miss instead of failing the build.
Narrow the surrounding try/except in the build flow so only the intended cache
lookup is caught, and let errors from wheels.get_build_tag() propagate (or
explicitly re-raise them) in the expected_tag/build_tag handling path and the
analogous block later in the file.
In `@src/fromager/packagesettings/_models.py`:
- Around line 88-102: WheelSettings.build_tag_hook currently accepts any
importable callable, so incompatible hooks can slip through until
fromager/wheels.py invokes them with ctx, req, version, and wheel_tags. Update
the WheelSettings validation in _models.py to inspect the resolved callable’s
signature during settings load and reject values that cannot accept the required
keyword arguments, so bad configs fail fast. Use the existing _colon_to_dot /
build_tag_hook validator path to locate the fix, and adjust the parsing tests to
use a helper that matches the actual hook contract instead of a generic
callable.
In `@src/fromager/wheels.py`:
- Around line 77-95: The build-tag hook result in the wheel tag helper is being
used as an arbitrary iterable before validation, which lets a single string be
treated as multiple segments and can exhaust generators. In the wheel-tag logic
around the hook call and _validate_build_tag_segments, first materialize the
hook result into a concrete sequence and verify every item is a string segment
before validating/joining it, so _validate_build_tag_segments and the subsequent
"_".join use the same stable data. Add regression coverage for both a
single-string return and a generator return to ensure invalid hook shapes fail
fast.
In `@tests/test_finders.py`:
- Around line 151-195: The wheel finder tests are missing the coexistence
scenario where both plain and suffixed build-tag wheels exist together and the
first candidate is rejected on revalidation. Add a test around
finders.find_wheel that places both the `...-2-...whl` and
`...-2_<suffix>-...whl` files in the same downloads directory, then verifies the
function still resolves the correct suffixed wheel after the plain one is
skipped. Use the existing test patterns in test_find_wheel_with_build_tag,
test_find_wheel_with_build_tag_suffix, and test_find_wheel_with_exact_suffix as
the reference points.
---
Outside diff comments:
In `@src/fromager/finders.py`:
- Around line 170-195: The wheel lookup in find_wheel_candidate is stopping at
the first match from candidate_bases, which is now unsafe because the build-tag
search can yield both plain and suffixed prefixes. Update the search in
src/fromager/finders.py so it does not return a single arbitrary wheel from the
first prefix match; instead, keep iterating through all matching
wheels/candidates until one passes build-tag validation, or collect and return
ordered candidates for the caller to evaluate. Use the existing
find_wheel_candidate logic and the candidate_bases/build_tag_prefixes handling
as the place to make this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9dc0d7d1-93be-4169-9c36-b2879f009258
📒 Files selected for processing (10)
src/fromager/bootstrapper.pysrc/fromager/commands/build.pysrc/fromager/finders.pysrc/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_settings.pysrc/fromager/wheels.pytests/test_finders.pytests/test_packagesettings.pytests/test_wheels.py
| expected_build_tag = wheels.get_build_tag( | ||
| ctx=self.ctx, | ||
| req=req, | ||
| version=resolved_version, | ||
| wheel_tags=wheel_tags, | ||
| ) | ||
| if expected_build_tag and expected_build_tag != actual_build_tag: | ||
| logger.info( | ||
| f"found wheel for {resolved_version} in {wheel_filename} but build tag does not match. Got {build_tag} but expected {expected_build_tag}" | ||
| f"found wheel for {resolved_version} in {wheel_filename} but build tag does not match. Got {actual_build_tag} but expected {expected_build_tag}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize empty build tags before comparing.
These checks only reject mismatches when expected_build_tag is truthy, so () will silently accept a cached wheel whose filename carries a real build tag. That is inconsistent with src/fromager/commands/build.py, which normalizes falsy tags to (0, "") and compares exactly. As written, the bootstrapper can reuse a wheel that the build verification path would reject.
Suggested fix
- if expected_build_tag and expected_build_tag != actual_build_tag:
+ normalized_expected_build_tag = expected_build_tag or (0, "")
+ normalized_actual_build_tag = actual_build_tag or (0, "")
+ if normalized_expected_build_tag != normalized_actual_build_tag:
logger.info(
f"found wheel for {resolved_version} in {wheel_filename} but build tag does not match. Got {actual_build_tag} but expected {expected_build_tag}"
)
return None, None- if expected_build_tag and expected_build_tag != actual_build_tag:
+ normalized_expected_build_tag = expected_build_tag or (0, "")
+ normalized_actual_build_tag = actual_build_tag or (0, "")
+ if normalized_expected_build_tag != normalized_actual_build_tag:
logger.info(
f"found wheel for {resolved_version} in cache but build tag does not match. Got {actual_build_tag} but expected {expected_build_tag}"
)
return None, NoneAlso applies to: 992-1000
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fromager/bootstrapper.py` around lines 948 - 956, Normalize build tags
before comparing in the bootstrapper cache check so empty tags are treated
consistently with the build verification path. Update the logic around
expected_build_tag in the bootstrapper flow to normalize falsy values to the
same canonical form used in src/fromager/commands/build.py (for example via the
existing build-tag helper or equivalent normalization) before comparing against
actual_build_tag. Apply the same fix anywhere the bootstrapper repeats this
check, including the other build-tag comparison block, so cached wheels are
rejected whenever their normalized tag differs.
| expected_build_tag = wheels.get_build_tag( | ||
| ctx=self.ctx, | ||
| req=req, | ||
| version=resolved_version, | ||
| wheel_tags=wheel_tags, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t downgrade hook validation failures to a cache miss.
wheels.get_build_tag() now validates hook output, but this call is still covered by the broad except Exception below. A buggy build_tag_hook will be logged as an unexpected cache lookup error and execution will continue, even though these errors are supposed to fail the build.
Also applies to: 1027-1031
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fromager/bootstrapper.py` around lines 992 - 997, The broad cache-lookup
exception handling around wheels.get_build_tag() is masking build_tag_hook
validation failures as recoverable cache misses. Update the try/except logic in
the bootstrapper code path that calls wheels.get_build_tag() so hook validation
errors are not caught by the generic except Exception and are allowed to fail
the build, while still preserving cache-miss handling for genuine lookup issues;
apply the same fix to both call sites in the bootstrapper flow.
There was a problem hiding this comment.
_download_wheel_from_cache uses except ResolverException (line 1016), not except Exception. A ValueError from the hook would propagate just fine. And _look_for_existing_wheel has no try/except at all.
There was a problem hiding this comment.
@shifa-khan, the except ResolverException isn't the only handler in _download_wheel_from_cache. There's also a broad except Exception as err further down in the same method:
except ResolverException:
logger.info(...)
return None, None
except requests.exceptions.RequestException as err:
logger.warning(...)
return None, None
except Exception as err: # ← this one
logger.warning(
f"unexpected error checking wheel cache for {resolved_version} "
f"at {self.cache_wheel_server_url}: {err}"
)
return None, NoneA ValueError from wheels.get_build_tag() (hook validation failure) falls through to that last clause, gets logged as an "unexpected error checking wheel cache", and returns (None, None) — silently treating a hook bug as a cache miss. The build continues unchecked.
You're right that _look_for_existing_wheel is clean (no try/except there).
🧠 Learnings used
Learnt from: tiran
Repo: python-wheel-build/fromager PR: 1202
File: src/fromager/sources.py:207-223
Timestamp: 2026-06-24T11:25:31.914Z
Learning: In this fromager codebase, any VCS URL parsing for pip-style `git+https://...` or `git+ssh://...` must use the single canonical URL-parsing helper introduced in PR `#1215` (the shared utility), not new inline/ad-hoc parsers. If reviewing code that currently parses these URLs in an ad-hoc way (e.g., in `sources.py` or elsewhere), refactor to call the shared helper to avoid divergent behavior.
| expected_tag = wheels.get_build_tag( | ||
| ctx=wkctx, | ||
| req=req, | ||
| version=resolved_version, | ||
| wheel_tags=wheel_tags, | ||
| ) | ||
| build_tag = expected_tag if expected_tag else (0, "") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Let build_tag_hook failures propagate here.
wheels.get_build_tag() is now where invalid suffix segments and hook errors surface, but this block still sits under except Exception and falls back to None. That turns a bad hook configuration into “no cached wheel” instead of failing the build, which contradicts the new build-tag contract. Narrow the try scope or re-raise errors coming from get_build_tag().
Also applies to: 532-541
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fromager/commands/build.py` around lines 498 - 504, The build-tag
resolution in build.py is swallowing errors from wheels.get_build_tag(), causing
invalid suffix segments or build_tag_hook failures to fall back to a
cached-wheel miss instead of failing the build. Narrow the surrounding
try/except in the build flow so only the intended cache lookup is caught, and
let errors from wheels.get_build_tag() propagate (or explicitly re-raise them)
in the expected_tag/build_tag handling path and the analogous block later in the
file.
| segments = hook(ctx=ctx, req=req, version=version, wheel_tags=wheel_tags) | ||
| _validate_build_tag_segments(segments) | ||
|
|
||
| if not segments: | ||
| return base_tag | ||
|
|
||
| suffix = base_tag[1] + "_" + "_".join(segments) | ||
| return (base_tag[0], suffix) | ||
|
|
||
|
|
||
| def _validate_build_tag_segments(segments: typing.Sequence[str]) -> None: | ||
| """Validate that each segment matches ``[a-zA-Z0-9.]``.""" | ||
| for seg in segments: | ||
| if not _BUILD_TAG_SEGMENT_RE.match(seg): | ||
| raise ValueError( | ||
| f"build tag hook returned invalid segment {seg!r}: " | ||
| "each segment must match [a-zA-Z0-9.]" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Materialize and type-check the hook result before validating it.
Right now a hook that returns "el9.6" is accepted as six one-character segments, and a generator gets exhausted by _validate_build_tag_segments() before "_".join(...) runs. Both cases yield incorrect wheel tags instead of failing fast, which breaks the filename/cache contract this helper is supposed to enforce.
Suggested fix
- segments = hook(ctx=ctx, req=req, version=version, wheel_tags=wheel_tags)
- _validate_build_tag_segments(segments)
+ raw_segments = hook(ctx=ctx, req=req, version=version, wheel_tags=wheel_tags)
+ if isinstance(raw_segments, (str, bytes)):
+ raise ValueError(
+ "build_tag_hook must return a sequence of segment strings"
+ )
+ try:
+ segments = list(raw_segments)
+ except TypeError as err:
+ raise ValueError(
+ "build_tag_hook must return a sequence of segment strings"
+ ) from err
+ if any(not isinstance(seg, str) for seg in segments):
+ raise ValueError("build_tag_hook must return only strings")
+ _validate_build_tag_segments(segments)A regression test for a single-string return and a generator return would lock this down.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| segments = hook(ctx=ctx, req=req, version=version, wheel_tags=wheel_tags) | |
| _validate_build_tag_segments(segments) | |
| if not segments: | |
| return base_tag | |
| suffix = base_tag[1] + "_" + "_".join(segments) | |
| return (base_tag[0], suffix) | |
| def _validate_build_tag_segments(segments: typing.Sequence[str]) -> None: | |
| """Validate that each segment matches ``[a-zA-Z0-9.]``.""" | |
| for seg in segments: | |
| if not _BUILD_TAG_SEGMENT_RE.match(seg): | |
| raise ValueError( | |
| f"build tag hook returned invalid segment {seg!r}: " | |
| "each segment must match [a-zA-Z0-9.]" | |
| ) | |
| raw_segments = hook(ctx=ctx, req=req, version=version, wheel_tags=wheel_tags) | |
| if isinstance(raw_segments, (str, bytes)): | |
| raise ValueError( | |
| "build_tag_hook must return a sequence of segment strings" | |
| ) | |
| try: | |
| segments = list(raw_segments) | |
| except TypeError as err: | |
| raise ValueError( | |
| "build_tag_hook must return a sequence of segment strings" | |
| ) from err | |
| if any(not isinstance(seg, str) for seg in segments): | |
| raise ValueError("build_tag_hook must return only strings") | |
| _validate_build_tag_segments(segments) | |
| if not segments: | |
| return base_tag | |
| suffix = base_tag[1] + "_" + "_".join(segments) | |
| return (base_tag[0], suffix) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/fromager/wheels.py` around lines 77 - 95, The build-tag hook result in
the wheel tag helper is being used as an arbitrary iterable before validation,
which lets a single string be treated as multiple segments and can exhaust
generators. In the wheel-tag logic around the hook call and
_validate_build_tag_segments, first materialize the hook result into a concrete
sequence and verify every item is a string segment before validating/joining it,
so _validate_build_tag_segments and the subsequent "_".join use the same stable
data. Add regression coverage for both a single-string return and a generator
return to ensure invalid hook shapes fail fast.
| def test_find_wheel_with_build_tag(tmp_path: pathlib.Path) -> None: | ||
| downloads = tmp_path / "downloads" | ||
| downloads.mkdir() | ||
| wheel = downloads / "mypkg-1.0.0-2-py3-none-any.whl" | ||
| wheel.write_text("not-empty") | ||
|
|
||
| req = Requirement("mypkg") | ||
| actual = finders.find_wheel(downloads, req, "1.0.0", (2, "")) | ||
| assert actual == wheel | ||
|
|
||
|
|
||
| def test_find_wheel_with_build_tag_suffix(tmp_path: pathlib.Path) -> None: | ||
| """find_wheel with an empty suffix should also match suffixed filenames.""" | ||
| downloads = tmp_path / "downloads" | ||
| downloads.mkdir() | ||
| wheel = downloads / "mypkg-1.0.0-2_el9.6-cp312-cp312-linux_x86_64.whl" | ||
| wheel.write_text("not-empty") | ||
|
|
||
| req = Requirement("mypkg") | ||
| # Search with base tag (no suffix) — should still find the suffixed wheel | ||
| actual = finders.find_wheel(downloads, req, "1.0.0", (2, "")) | ||
| assert actual == wheel | ||
|
|
||
|
|
||
| def test_find_wheel_with_exact_suffix(tmp_path: pathlib.Path) -> None: | ||
| """find_wheel with an exact suffix matches the right wheel.""" | ||
| downloads = tmp_path / "downloads" | ||
| downloads.mkdir() | ||
| wheel = downloads / "mypkg-1.0.0-2_el9.6_cuda13.0-cp312-cp312-linux_x86_64.whl" | ||
| wheel.write_text("not-empty") | ||
|
|
||
| req = Requirement("mypkg") | ||
| actual = finders.find_wheel(downloads, req, "1.0.0", (2, "_el9.6_cuda13.0")) | ||
| assert actual == wheel | ||
|
|
||
|
|
||
| def test_find_wheel_suffix_no_false_positive(tmp_path: pathlib.Path) -> None: | ||
| """Build tag 2 should not match build tag 20.""" | ||
| downloads = tmp_path / "downloads" | ||
| downloads.mkdir() | ||
| (downloads / "mypkg-1.0.0-20-py3-none-any.whl").write_text("not-empty") | ||
|
|
||
| req = Requirement("mypkg") | ||
| actual = finders.find_wheel(downloads, req, "1.0.0", (2, "")) | ||
| assert actual is None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the coexistence case for plain and suffixed wheels.
These tests only cover plain and suffixed filenames in isolation. The regression shows up when both are present in the same directory and the caller rejects the first candidate after revalidation. Please add a case with both ...-2-...whl and ...-2_<suffix>-...whl present so this behavior is locked down. As per path instructions, "Verify test actually tests the intended behavior. Check for missing edge cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_finders.py` around lines 151 - 195, The wheel finder tests are
missing the coexistence scenario where both plain and suffixed build-tag wheels
exist together and the first candidate is rejected on revalidation. Add a test
around finders.find_wheel that places both the `...-2-...whl` and
`...-2_<suffix>-...whl` files in the same downloads directory, then verifies the
function still resolves the correct suffixed wheel after the plain one is
skipped. Use the existing test patterns in test_find_wheel_with_build_tag,
test_find_wheel_with_build_tag_suffix, and test_find_wheel_with_exact_suffix as
the reference points.
Source: Path instructions
|
|
||
| model_config = MODEL_CONFIG | ||
|
|
||
| build_tag_hook: pydantic.ImportString[typing.Callable[..., typing.Any]] | None = ( |
There was a problem hiding this comment.
Let's define a more detailed callable:
BuildTagHookCallable = typing.Callable[
["context.WorkContext", Requirement, Version, frozenset[Tag]],
typing.Sequence[str],
]the define the hook as:
build_tag_hook: pydantic.ImportString[BuildTagHookCallable] | None = None
| @pydantic.field_validator("build_tag_hook", mode="before") | ||
| @classmethod | ||
| def _colon_to_dot(cls, v: typing.Any) -> typing.Any: | ||
| if isinstance(v, str) and ":" in v: | ||
| return v.replace(":", ".", 1) | ||
| return v |
There was a problem hiding this comment.
You don't need to modify the string. ImportString supports the : syntax.
>>> import pydantic
>>> class WheelSettings(pydantic.BaseModel):
... build_tag_hook: pydantic.ImportString
...
>>> WheelSettings(build_tag_hook="fromager.context:WorkContext")
WheelSettings(build_tag_hook=<class 'fromager.context.WorkContext'>)| wheels: | ||
| build_tag_hook: "mypackage.hooks:build_tag_hook" | ||
|
|
||
| .. versionadded:: 0.82.0 |
There was a problem hiding this comment.
Fromager 0.88.0 is out, the new feature will land in 0.89.0 or later.
| .. versionadded:: 0.82.0 | |
| .. versionadded:: 0.89.0 |
| ctx: context.WorkContext, | ||
| req: Requirement, | ||
| version: Version, | ||
| wheel_tags: frozenset[Tag] | None = None, |
There was a problem hiding this comment.
Under which circumstances is wheel_tags None? All wheels must have at least one wheel tag.
There was a problem hiding this comment.
AI tends to write a lot of test cases. Every test case comes with a setup cost and makes the test unnecessary slow. I always tell Claude to combine tests to reduce the amount of test cases.
There was a problem hiding this comment.
The finder API is an old, badly designed part of Fromager. It tries to be clever, but gets it wrong in some cases. We may need to reconsider and redesign this part of Fromager, first. :/
2 participants
Allow downstream projects to append environment-specific suffixes (e.g. OS, accelerator) to wheel build tags via a user-defined hook. The hook is configured in settings.yaml under
wheels.build_tag_hookand receives the build context, requirement, version, and wheel tags. Cache lookups and build-tag validation are updated to account for suffixed wheels. No behavior change when the hook is not configured.Closes: #1181