-
Notifications
You must be signed in to change notification settings - Fork 52
feat(wheels): add configurable build tag hook for wheel filenames #1273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e724aff
909caad
5ce0a1e
405b64c
8282781
15584f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -393,6 +393,88 @@ $ tox -e cli -- canonicalize flit-core | |
| flit_core | ||
| ``` | ||
|
|
||
| ## Global settings | ||
|
|
||
| Global settings are configured in the `settings.yaml` file passed via the | ||
| `--settings-file` flag. These settings apply to all packages being built. | ||
|
|
||
| ### Wheel build tag hook | ||
|
|
||
| The `build_tag_hook` is a configuration option that allows you to customize | ||
| wheel filenames by appending configuration-specific suffixes to the build tag. | ||
| This is useful for creating unique, deterministic filenames that reflect the | ||
| build configuration and distinguish wheels built for different variants. | ||
|
|
||
| Configure the hook in your global `settings.yaml`: | ||
|
|
||
| ```yaml | ||
| wheels: | ||
| build_tag_hook: "myproject.hooks:build_tag_hook" | ||
| ``` | ||
|
|
||
| The hook function receives keyword-only arguments and returns a sequence of | ||
| suffix segments (strings) to append to the wheel build tag: | ||
|
|
||
| ```python | ||
| from typing import Sequence | ||
| from packaging.requirements import Requirement | ||
| from packaging.version import Version | ||
| from packaging.tags import Tag | ||
|
|
||
| from fromager import context | ||
|
|
||
|
|
||
| def build_tag_hook( | ||
| *, | ||
| ctx: context.WorkContext, | ||
| req: Requirement, | ||
| version: Version, | ||
| wheel_tags: frozenset[Tag], | ||
| ) -> Sequence[str]: | ||
| """Return suffix segments for the wheel build tag. | ||
|
|
||
| The segments are joined with underscores and appended to the numeric | ||
| build tag. For example, returning ["cpu"] produces the build tag: | ||
| {numeric_base}_cpu, while returning ["gpu", "cuda"] produces | ||
| {numeric_base}_gpu_cuda. | ||
|
|
||
| The hook must return identical suffix segments when called with the same | ||
| configuration, ensuring that wheels built on different machines with the | ||
| same build configuration have identical filenames. This allows wheel caches | ||
| to work correctly across builders. | ||
|
|
||
| Args: | ||
| ctx: The build context, containing variant and settings information | ||
| req: The package requirement being built | ||
| version: The version being built | ||
| wheel_tags: Frozenset of wheel tags (use to distinguish platform-specific | ||
| wheels from pure-python wheels; don't use for platform decisions) | ||
|
|
||
| Returns: | ||
| A sequence of suffix segments (alphanumeric + dots only). | ||
| Must not return a single string or bytes object. | ||
|
|
||
| Raises: | ||
| ValueError: If segments contain invalid characters or types | ||
| """ | ||
| # Example: Return variant-specific suffix | ||
| return [ctx.variant] | ||
| ``` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two different hook mechanisms, no explanation The existing "Process hooks" section (line 478) uses entry points: This new hook uses an import string in YAML: These are fundamentally different plugin mechanisms. The doc doesn't explain why, or even acknowledge the difference. A reader who knows the existing hooks will wonder why this one doesn't use entry points. One sentence of rationale would help e.g., "Unlike process hooks (which use entry points), ted directly in settings.yaml because it applies globally rather than per-package." |
||
|
|
||
| **Important notes:** | ||
|
|
||
| - The hook is only invoked when the package has a non-empty build tag from | ||
| its changelog entry. Pure-python packages without a build tag skip the hook. | ||
| - Each segment must contain only alphanumeric ASCII characters or dots | ||
| (`[a-zA-Z0-9.]`). Invalid characters cause a build error. | ||
| - The hook must be deterministic and independent of wheel contents, build | ||
| environment, or ELF metadata, so that fresh builds and cache lookups | ||
| produce identical tags. | ||
| - Use `wheel_tags` only to distinguish platform-specific wheels (`platlib`) | ||
| from pure-python wheels (`py3-none-any`). Don't use it for platform-specific | ||
| decisions—the hook must return identical results across architectures for | ||
| the same variant. | ||
|
|
||
| ## Process hooks | ||
|
|
||
| Fromager supports plugging in Python hooks to be run after build events. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,22 +86,27 @@ def _look_for_existing_wheel( | |
| search_in: pathlib.Path, | ||
| ) -> tuple[pathlib.Path | None, pathlib.Path | None]: | ||
| pbi = ctx.package_build_info(req) | ||
| expected_build_tag = pbi.build_tag(resolved_version) | ||
| base_build_tag = pbi.build_tag(resolved_version) | ||
| logger.info( | ||
| f"looking for existing wheel for version {resolved_version} with build tag {expected_build_tag} in {search_in}" | ||
| f"looking for existing wheel for version {resolved_version} with build tag {base_build_tag} in {search_in}" | ||
| ) | ||
| wheel_filename = finders.find_wheel( | ||
| downloads_dir=search_in, | ||
| req=req, | ||
| dist_version=str(resolved_version), | ||
| build_tag=expected_build_tag, | ||
| build_tag=base_build_tag, | ||
| ) | ||
| if not wheel_filename: | ||
| return None, None | ||
| _, _, build_tag, _ = wheels.extract_info_from_wheel_file(req, wheel_filename) | ||
| if expected_build_tag and expected_build_tag != build_tag: | ||
| _, _, actual_build_tag, wheel_tags = wheels.extract_info_from_wheel_file( | ||
| req, wheel_filename | ||
| ) | ||
| expected_build_tag = wheels.get_build_tag( | ||
| ctx=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}" | ||
|
Comment on lines
93
to
+109
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift Continue searching after a build-tag mismatch. Both cache paths validate only the first candidate. If that candidate has a stale suffix, the code returns a cache miss even when another candidate has the expected computed tag.
📍 Affects 1 file
🤖 Prompt for AI Agents |
||
| ) | ||
| return None, None | ||
| logger.info(f"found existing wheel {wheel_filename}") | ||
|
|
@@ -129,16 +134,20 @@ def _download_wheel_from_cache( | |
| results = resolver.find_all_matching_from_provider(provider, pinned_req) | ||
| wheel_url, _ = results[0] | ||
| wheelfile_name = pathlib.Path(urlparse(wheel_url).path) | ||
| _, _, actual_build_tag, wheel_tags = wheels.extract_info_from_wheel_file( | ||
| req, wheelfile_name | ||
| ) | ||
| pbi = ctx.package_build_info(req) | ||
| expected_build_tag = pbi.build_tag(resolved_version) | ||
| expected_build_tag = wheels.get_build_tag( | ||
| ctx=ctx, req=req, version=resolved_version, wheel_tags=wheel_tags | ||
| ) | ||
|
Comment on lines
+137
to
+143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Do not convert hook failures into cache misses.
Compute the expected tag outside the cache-lookup handler, or re-raise errors from 🤖 Prompt for AI Agents |
||
| logger.info(f"has expected build tag {expected_build_tag}") | ||
| changelogs = pbi.get_changelog(resolved_version) | ||
| logger.debug(f"has change logs {changelogs}") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason why we are removing this log line?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My mistake. Will restore the changelog debug logging. |
||
|
|
||
| _, _, build_tag, _ = wheels.extract_info_from_wheel_file(req, wheelfile_name) | ||
| if expected_build_tag and expected_build_tag != build_tag: | ||
| if expected_build_tag and expected_build_tag != actual_build_tag: | ||
| logger.info( | ||
| f"found wheel for {resolved_version} in cache but build tag does not match. Got {build_tag} but expected {expected_build_tag}" | ||
| 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, None | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -485,11 +485,25 @@ def _is_wheel_built( | |
| wheel_server_urls=wheel_server_urls, | ||
| ) | ||
| logger.info("found candidate wheel %s", url) | ||
| pbi = wkctx.package_build_info(req) | ||
| build_tag_from_settings = pbi.build_tag(resolved_version) | ||
| build_tag = build_tag_from_settings if build_tag_from_settings else (0, "") | ||
| wheel_basename = downloads.extract_filename_from_url(url) | ||
| _, _, build_tag_from_name, _ = parse_wheel_filename(wheel_basename) | ||
| _, _, build_tag_from_name, wheel_tags = parse_wheel_filename(wheel_basename) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. url, wheel_basename, build_tag_from_name, and wheel_tags are assigned inside the try block and used after the except. The except block returns None, so the code is technically correct, but the flow is fragile. An else clause on the try would make the intent explicit and survive future refactoring more safely: |
||
| except Exception: | ||
| logger.debug( | ||
| "could not locate prebuilt wheel %s-%s on %s", | ||
| dist_name, | ||
| resolved_version, | ||
| wheel_server_urls, | ||
| exc_info=True, | ||
| ) | ||
| logger.info("could not locate prebuilt wheel") | ||
| return None | ||
| else: | ||
| # Compute expected build tag in the else clause so hook | ||
| # validation errors propagate instead of being swallowed. | ||
| 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, "") | ||
| existing_build_tag = build_tag_from_name if build_tag_from_name else (0, "") | ||
| if ( | ||
| existing_build_tag[0] > build_tag[0] | ||
|
|
@@ -513,21 +527,20 @@ def _is_wheel_built( | |
| wheel_filename = None | ||
|
|
||
| if not wheel_filename: | ||
| # if the found wheel was on an external server, then download it | ||
| logger.info("downloading wheel from %s", url) | ||
| wheel_filename = wheels.download_wheel(req, url, wkctx.wheels_downloads) | ||
| try: | ||
| logger.info("downloading wheel from %s", url) | ||
| wheel_filename = wheels.download_wheel(req, url, wkctx.wheels_downloads) | ||
| except Exception: | ||
| logger.debug( | ||
| "failed to download prebuilt wheel %s-%s", | ||
| dist_name, | ||
| resolved_version, | ||
| exc_info=True, | ||
| ) | ||
| logger.info("could not download prebuilt wheel") | ||
| return None | ||
|
|
||
| return wheel_filename | ||
| except Exception: | ||
| logger.debug( | ||
| "could not locate prebuilt wheel %s-%s on %s", | ||
| dist_name, | ||
| resolved_version, | ||
| wheel_server_urls, | ||
| exc_info=True, | ||
| ) | ||
| logger.info("could not locate prebuilt wheel") | ||
| return None | ||
|
|
||
|
|
||
| def _build_parallel( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc string is too large, can we make it concise?