fix(bootstrap): don't call get_resolver_provider hook in cache lookups#1051
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes are concentrated and focused on a single behavioral shift (replacing resolver hook invocation with direct provider initialization). The bootstrap file change is compact (7 added/2 removed lines), but requires understanding the resolver API semantics and why the alternate code path avoids hooks. The new test explicitly validates the intended bypass, reducing ambiguity about the expected behavior. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/bootstrapper.py`:
- Around line 1113-1114: The code currently takes the first candidate from
resolver.find_all_matching_from_provider (results[0]) and treats it as a miss if
it fails the build-tag check; instead iterate over the results array for the
pinned_req and pick the first (wheel_url, metadata) whose
build-tag/compatibility check passes (the same logic used later around the
1124-1131 build-tag verification), only falling back to (None, None) if no
candidate matches; update both the immediate use (where results[0] is read) and
the similar block covering lines 1124-1131 to perform this filtered selection so
later valid cache entries aren’t ignored.
🪄 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: 7e540214-986a-4d2e-9422-b35ef46423a9
📒 Files selected for processing (2)
src/fromager/bootstrapper.pytests/test_bootstrapper.py
| results = resolver.find_all_matching_from_provider(provider, pinned_req) | ||
| wheel_url, _ = results[0] |
There was a problem hiding this comment.
Scan all cache candidates before declaring a miss.
Line 1114 takes results[0] before the build-tag check runs. If the cache index has multiple wheels for the same version, a non-matching first entry turns a valid cache hit into (None, None) even when a later candidate has the expected build tag.
💡 Suggested change
- results = resolver.find_all_matching_from_provider(provider, pinned_req)
- wheel_url, _ = results[0]
- wheelfile_name = pathlib.Path(urlparse(wheel_url).path)
pbi = self.ctx.package_build_info(req)
expected_build_tag = pbi.build_tag(resolved_version)
# Log the expected build tag for debugging
logger.info(f"has expected build tag {expected_build_tag}")
# Get changelogs for debug info
changelogs = pbi.get_changelog(resolved_version)
logger.debug(f"has change logs {changelogs}")
-
- _, _, build_tag, _ = wheels.extract_info_from_wheel_file(
- req, wheelfile_name
- )
- if expected_build_tag and expected_build_tag != 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}"
- )
- return None, None
+ results = resolver.find_all_matching_from_provider(provider, pinned_req)
+ wheel_url = None
+ for candidate_url, _ in results:
+ wheel_filename = pathlib.Path(urlparse(candidate_url).path)
+ _, _, build_tag, _ = wheels.extract_info_from_wheel_file(
+ req, wheel_filename
+ )
+ if not expected_build_tag or expected_build_tag == build_tag:
+ wheel_url = candidate_url
+ break
+
+ if wheel_url is None:
+ logger.info(
+ f"found wheel for {resolved_version} in cache but no candidate matched build tag {expected_build_tag}"
+ )
+ return None, NoneAlso applies to: 1124-1131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/bootstrapper.py` around lines 1113 - 1114, The code currently
takes the first candidate from resolver.find_all_matching_from_provider
(results[0]) and treats it as a miss if it fails the build-tag check; instead
iterate over the results array for the pinned_req and pick the first (wheel_url,
metadata) whose build-tag/compatibility check passes (the same logic used later
around the 1124-1131 build-tag verification), only falling back to (None, None)
if no candidate matches; update both the immediate use (where results[0] is
read) and the similar block covering lines 1124-1131 to perform this filtered
selection so later valid cache entries aren’t ignored.
src/fromager/bootstrapper.py
Outdated
| # Use PyPIProvider directly for cache lookups, bypassing resolver | ||
| # hooks. Cache servers are always simple PyPI index servers. | ||
| pinned_req = Requirement(f"{req.name}=={resolved_version}") | ||
| provider = resolver.default_resolver_provider( |
There was a problem hiding this comment.
how about you directly create a PyPIProvider here?
There was a problem hiding this comment.
You are right. Hypothetically if default_resolver_provider ever gains additional logic (logging, filtering, etc.), those changes would not be for cache lookups against a known simple index server.
_download_wheel_from_cache() called resolver.resolve(), which triggered custom resolver hooks. Cache servers are always simple PyPI index servers and don't need hook-based resolution. Call default_resolver_provider() directly instead. Closes: python-wheel-build#1049 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
LalatenduMohanty
force-pushed
the
fix/1049-cache-lookup-bypass-hook
branch
from
9abd5dd to
7a8f95a
Compare
2 participants
_download_wheel_from_cache() called resolver.resolve(), which triggered custom resolver hooks. Cache servers are always simple PyPI index servers and don't need hook-based resolution. Call default_resolver_provider() directly instead.
Closes: #1049