-
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 |
|---|---|---|
|
|
@@ -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}" | ||
| ) | ||
| 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.
🚀 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.
src/fromager/bootstrapper/_cache.py#L93-L109: enumerate matching local wheels and return the first candidate whose computed tag matches its filename tag.src/fromager/bootstrapper/_cache.py#L137-L150: iterate remote resolver results and continue after a tag mismatch.📍 Affects 1 file
src/fromager/bootstrapper/_cache.py#L93-L109(this comment)src/fromager/bootstrapper/_cache.py#L137-L150🤖 Prompt for AI Agents