fix(wheels): disable cooldown for local and cache wheel servers#1141
fix(wheels): disable cooldown for local and cache wheel servers#1141smoparth wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR threads a new cache_wheel_server_url through build and download paths, adds resolve_cached_wheel and cache-first resolution to prefer local/cache servers when locating prebuilt wheels, and updates acquisition to use a local cached file when the resolved URL is the local wheel server. resolve_all_prebuilt_wheels no longer special-cases local servers for upload-time support. Tests were added/updated to assert that local and cache wheel servers skip cooldown warnings while external servers retain cooldown behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fromager/wheels.py`:
- Around line 556-565: The equality checks against ctx.wheel_server_url and
cache_wheel_server_url can fail due to URL normalization differences (trailing
slashes, etc.); update the comparisons in the block that assigns
provider.cooldown to None so they compare normalized URLs instead of raw strings
— normalize `url`, `ctx.wheel_server_url`, and `cache_wheel_server_url` (e.g.,
parse and rebuild or at minimum strip trailing slashes and ensure consistent
scheme/netloc/path) before doing the equality checks, and then set
provider.cooldown = None when the normalized values match.
🪄 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: 13501074-a382-4bd6-bbb9-92f828557ff1
📒 Files selected for processing (3)
src/fromager/commands/build.pysrc/fromager/wheels.pytests/test_cooldown.py
|
This PR feels like a hack on top of a hack. The code is becoming increasingly confusing and hard to understand. I think the root cause of the issue is the fact that code paths are mixing cache wheel server and PyPI server for pre-built wheels. Cache lookup is using the same code path as pre-built download. At least that's what I think is happening... |
LalatenduMohanty
left a comment
LalatenduMohanty
left a comment
There was a problem hiding this comment.
We should change this PR to use PyPICacheProvider for both the local and cache wheel server URLs inside resolve_all_prebuilt_wheels, rather than conditionally nulling out cooldown after the fact. refer #1143
smoparth
force-pushed
the
fix/disable-cooldown-cache-server
branch
from
f1ca260 to
eae3f0d
Compare
smoparth
force-pushed
the
fix/disable-cooldown-cache-server
branch
from
eae3f0d to
85b2780
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
smoparth
force-pushed
the
fix/disable-cooldown-cache-server
branch
from
85b2780 to
19c9cd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fromager/commands/build.py (1)
524-532: 💤 Low valueLog wording: "prebuilt" no longer matches the function's scope.
_is_wheel_builtnow covers cache/local candidates too, not just prebuilt wheels. The "could not locate prebuilt wheel" log messages at lines 526 and 531 will be misleading when the failure is for a cache hit. Consider "existing" to match line 488.♻️ Suggested change
- logger.debug( - "could not locate prebuilt wheel %s-%s", - dist_name, - resolved_version, - exc_info=True, - ) - logger.info("could not locate prebuilt wheel") + logger.debug( + "could not locate existing wheel %s-%s", + dist_name, + resolved_version, + exc_info=True, + ) + logger.info("could not locate existing wheel")🤖 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 524 - 532, The logs in _is_wheel_built incorrectly say "prebuilt wheel" while the function also checks cache/local candidates; update both logger calls that reference dist_name and resolved_version (the logger.debug with exc_info and the logger.info that returns None) to use wording like "existing wheel" or "existing candidate" (consistent with earlier "existing" wording at line 488) so messages accurately reflect cache/local/prebuilt scope.
🤖 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/commands/build.py`:
- Around line 481-489: The call to wheels.resolve_existing_wheel is currently
executed before the try/except in _is_wheel_built, so any exceptions (e.g.,
transient network errors) escape and abort the build; move the
resolve_existing_wheel(...) invocation into the existing try block that
surrounds the logic at lines 491+ so that exceptions are caught and the function
returns None (falling back to source build) as before—adjust references to
ctx=wkctx, req=req, pbi=pbi, cache_wheel_server_url=cache_wheel_server_url and
preserve the existing logger.info("could not locate existing wheel")/return None
flow inside the except path.
---
Nitpick comments:
In `@src/fromager/commands/build.py`:
- Around line 524-532: The logs in _is_wheel_built incorrectly say "prebuilt
wheel" while the function also checks cache/local candidates; update both logger
calls that reference dist_name and resolved_version (the logger.debug with
exc_info and the logger.info that returns None) to use wording like "existing
wheel" or "existing candidate" (consistent with earlier "existing" wording at
line 488) so messages accurately reflect cache/local/prebuilt scope.
🪄 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: d96726c4-5302-4336-905e-1d3f306b488c
📒 Files selected for processing (4)
src/fromager/commands/build.pysrc/fromager/commands/download_sequence.pysrc/fromager/wheels.pytests/test_cooldown.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_cooldown.py
- src/fromager/commands/download_sequence.py
- src/fromager/wheels.py
smoparth
force-pushed
the
fix/disable-cooldown-cache-server
branch
from
19c9cd9 to
128b1b2
Compare
smoparth
force-pushed
the
fix/disable-cooldown-cache-server
branch
from
128b1b2 to
8862a7e
Compare
smoparth
force-pushed
the
fix/disable-cooldown-cache-server
branch
from
8862a7e to
a7aba28
Compare
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
This looks good! I will wait for @LalatenduMohanty to approve since he requested changes before
|
The function
|
Use PyPICacheProvider (introduced in #1143) for local and cache wheel server lookups in the build and download-sequence paths. This separates trusted internal sources (no cooldown, no upload-time checks) from upstream pre-built sources. - Add resolve_cached_wheel() using PyPICacheProvider - Simplify _is_wheel_built() to only check cache servers (local + cache_wheel_server) via resolve_cached_wheel - Route download_sequence local server through resolve_cached_wheel - Remove supports_upload_time workaround from resolve_all_prebuilt_wheels - Add tests for cache/local cooldown bypass and upstream cooldown retention Follows up on #1143 and addresses maintainer feedback from #1141. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
smoparth
force-pushed
the
fix/disable-cooldown-cache-server
branch
from
a7aba28 to
6b21905
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fromager/wheels.py (1)
532-548: 💤 Low valueAdd a Sphinx
versionaddeddirective to the new public helper.
resolve_cached_wheelis a new public function; per the repo's documentation conventions, user-facing additions should be annotated so downstream docs and consumers can tell when the API appeared.📝 Suggested docstring tweak
def resolve_cached_wheel( *, ctx: context.WorkContext, req: Requirement, cache_server_url: str, ) -> tuple[str, Version]: """Resolve a wheel from a trusted cache server (local or remote). Uses ``PyPICacheProvider`` -- no cooldown, no hooks, no upload-time checks. + + .. versionadded:: NEXT """As per coding guidelines: "Use Sphinx
versionadded,versionremoved,versionchangeddirectives for user-facing changes".🤖 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 532 - 548, The new public helper resolve_cached_wheel needs a Sphinx "versionadded" directive in its docstring; edit the triple-quoted docstring for resolve_cached_wheel to add a line like "versionadded:: <release>" (use the current release number for this change) immediately under the short description so the function appears in generated docs as added in that version. Ensure the directive follows Sphinx formatting and stays inside the existing docstring for the resolve_cached_wheel function.
🤖 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.
Nitpick comments:
In `@src/fromager/wheels.py`:
- Around line 532-548: The new public helper resolve_cached_wheel needs a Sphinx
"versionadded" directive in its docstring; edit the triple-quoted docstring for
resolve_cached_wheel to add a line like "versionadded:: <release>" (use the
current release number for this change) immediately under the short description
so the function appears in generated docs as added in that version. Ensure the
directive follows Sphinx formatting and stays inside the existing docstring for
the resolve_cached_wheel function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7777c231-6814-4a3e-9e71-54bc6e528cc5
📒 Files selected for processing (4)
src/fromager/commands/build.pysrc/fromager/commands/download_sequence.pysrc/fromager/wheels.pytests/test_cooldown.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/commands/download_sequence.py
- tests/test_cooldown.py
@tiran, fixed. _is_wheel_built now only checks cache servers (local + cache_wheel_server) via resolve_cached_wheel using PyPICacheProvider. |
Use PyPICacheProvider (introduced in #1143) for local and cache wheel server lookups in the build and download-sequence paths. This separates trusted internal sources (no cooldown, no upload-time checks) from upstream pre-built sources. - Add resolve_cached_wheel() using PyPICacheProvider - Simplify _is_wheel_built() to only check cache servers (local + cache_wheel_server) via resolve_cached_wheel - Route download_sequence local server through resolve_cached_wheel - Remove supports_upload_time workaround from resolve_all_prebuilt_wheels - Add tests for cache/local cooldown bypass and upstream cooldown retention Follows up on #1143 and closes #1140. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
smoparth
force-pushed
the
fix/disable-cooldown-cache-server
branch
from
6b21905 to
186577a
Compare
4 participants
Use PyPICacheProvider (introduced in #1143) for local and cache wheel server lookups in the build and download-sequence paths. This separates trusted internal sources (no cooldown, no upload-time checks) from upstream pre-built sources.
Follows up on #1143 and closes #1140.
Pull Request Description
What
Why