fix(resolver): exempt top-level pinned versions from transitive cooldown#1157
fix(resolver): exempt top-level pinned versions from transitive cooldown#1157LalatenduMohanty wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR fixes cooldown enforcement for transitive dependencies that match top-level exact pins. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
LalatenduMohanty
changed the title
| ) | ||
|
|
||
|
|
||
| def resolve_package_cooldown( |
There was a problem hiding this comment.
I'm finding the logic in this function really hard to follow. There are a lot of exceptions to rules applying the days or deciding which number of days to use.
Why not make it always return a new valid Cooldown instance, even if the number of days is 0 in some cases or the values match the global settings? That would simplify the logic to just needing to decide which value to use for each argument to that class.
There was a problem hiding this comment.
I have tried to make the code more easy to follow. PTAL
| global_cooldown = ctx.cooldown | ||
| if per_package_days is None: | ||
| return global_cooldown | ||
| if global_cooldown is None: |
There was a problem hiding this comment.
If the global cooldown is None, does that mean cooldown is disabled?
There was a problem hiding this comment.
Yes, global_cooldown is None (i.e. ctx.cooldown is None) means cooldown is disabled globally. It means the user did not pass --min-release-age on the CLI, so no cooldown policy is in effect for the run.
There was a problem hiding this comment.
OK, so it seems like that check could be done earlier, outside of this branch, because that decision overrides any other settings, right?
There was a problem hiding this comment.
ctx.cooldown does not override everything. For example. It gets the value from individual package package override. A per-package min_release_age setting in the package's YAML config would lead to ctx.cooldown have the value even when no global cooldown is configured
Sorry I was wrong, the right answer is ctx.cooldown is none alone doesn't mean cooldown is disabled for all packages. A per-package min_release_age setting in the package's YAML config can enable cooldown for that package as well. That's why the check is ctx.cooldown is None and per_package_days is None , we must check both.
There was a problem hiding this comment.
@dhellmann does that answer your question. So IMO we should check value of ctx.cooldown here.
LalatenduMohanty
force-pushed
the
fix/cooldown-exempt-versions
branch
2 times, most recently
from
cc39991 to
400382b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/fromager/candidate.py (1)
36-36: ⚡ Quick winAdd Sphinx
versionaddeddirective for the new field.The
exempt_versionsfield is a user-facing change to a public API. As per coding guidelines, use theversionaddeddirective to document when this field was introduced.📝 Suggested documentation addition
exempt_versions bypasses the age check for specific versions that were already approved via a top-level exact pin. + + .. versionadded:: <version> + The `exempt_versions` field. """🤖 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/candidate.py` at line 36, The new public field exempt_versions (dataclass field in Candidate) needs a Sphinx versionadded directive in the class docstring; update the Candidate class docstring to add a ".. versionadded:: X.Y.Z" entry describing exempt_versions and when it was introduced (replace X.Y.Z with the release version), e.g., note that exempt_versions is a frozenset[Version] used to skip certain versions, so documentation consumers see when this public API appeared.src/fromager/resolver.py (1)
196-218: ⚡ Quick winAdd Sphinx
versionchangeddirective for the behavior change.The function now populates
exempt_versionsto allow top-level pinned versions to bypass cooldown when encountered as transitive dependencies. This is a significant user-facing behavior change that should be documented.📝 Suggested documentation addition
Otherwise a ``Cooldown`` is returned with: * *min_age* from the per-package override (if set) or the global cooldown. * *bootstrap_time* inherited from the global cooldown (for a consistent cutoff across the entire run) or the current time. * *exempt_versions* populated from top-level exact-pinned entries in the dependency graph, so transitive resolutions of the same package honour the user's explicit pin. + + .. versionchanged:: <version> + Added support for exempting top-level pinned versions from cooldown + when encountered as transitive dependencies via the `exempt_versions` field. """🤖 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/resolver.py` around lines 196 - 218, Add a Sphinx "versionchanged" directive to the docstring of resolve_package_cooldown documenting that it now populates exempt_versions from top-level exact-pinned entries so transitive dependencies can bypass cooldown; update the docstring near the explanatory bullet points to include a short versionchanged note (mentioning the new behavior, version number or "since X.Y" placeholder, and a one-line rationale) and ensure formatting matches existing Sphinx directives used elsewhere in the project.
🤖 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/candidate.py`:
- Line 36: The new public field exempt_versions (dataclass field in Candidate)
needs a Sphinx versionadded directive in the class docstring; update the
Candidate class docstring to add a ".. versionadded:: X.Y.Z" entry describing
exempt_versions and when it was introduced (replace X.Y.Z with the release
version), e.g., note that exempt_versions is a frozenset[Version] used to skip
certain versions, so documentation consumers see when this public API appeared.
In `@src/fromager/resolver.py`:
- Around line 196-218: Add a Sphinx "versionchanged" directive to the docstring
of resolve_package_cooldown documenting that it now populates exempt_versions
from top-level exact-pinned entries so transitive dependencies can bypass
cooldown; update the docstring near the explanatory bullet points to include a
short versionchanged note (mentioning the new behavior, version number or "since
X.Y" placeholder, and a one-line rationale) and ensure formatting matches
existing Sphinx directives used elsewhere in the project.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08b06f2d-b90a-423b-b7d4-a5b322a32805
📒 Files selected for processing (3)
src/fromager/candidate.pysrc/fromager/resolver.pytests/test_cooldown.py
LalatenduMohanty
force-pushed
the
fix/cooldown-exempt-versions
branch
from
400382b to
e494e95
Compare
Add `exempt_versions` field to `Cooldown` so that only the specific version(s) approved via a top-level `==` pin bypass the cooldown age check when resolved as transitive dependencies. Other versions remain subject to cooldown filtering. Fixes: python-wheel-build#1153 python-wheel-build#1155 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
LalatenduMohanty
force-pushed
the
fix/cooldown-exempt-versions
branch
from
e494e95 to
abcfd23
Compare
2 participants
Add
exempt_versionsfield toCooldownso that only the specific version(s) approved via a top-level==pin bypass the cooldown age check when resolved as transitive dependencies. Other versions remain subject to cooldown filtering.Fixes: #1153 #1155