-
Notifications
You must be signed in to change notification settings - Fork 50
feat(resolver): exempt top-level == pins from release cooldown #1124
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
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 |
|---|---|---|
|
|
@@ -103,7 +103,7 @@ def resolve( | |
| req_type=req_type, | ||
| ignore_platform=ignore_platform, | ||
| ) | ||
| provider.cooldown = resolve_package_cooldown(ctx, req) | ||
| provider.cooldown = resolve_package_cooldown(ctx, req, req_type=req_type) | ||
| max_age_cutoff = _compute_max_age_cutoff(ctx) | ||
| results = find_all_matching_from_provider( | ||
| provider, req, max_age_cutoff=max_age_cutoff | ||
|
|
@@ -137,19 +137,33 @@ def default_resolver_provider( | |
| ) | ||
|
|
||
|
|
||
| def _has_equality_pin(req: Requirement) -> bool: | ||
| """Return True if the requirement contains an ``==`` version specifier.""" | ||
| return any(spec.operator == "==" for spec in req.specifier) | ||
|
|
||
|
|
||
| def resolve_package_cooldown( | ||
| ctx: context.WorkContext, | ||
| req: Requirement, | ||
| req_type: RequirementType | None = None, | ||
| ) -> Cooldown | None: | ||
| """Compute the effective cooldown for a single package. | ||
|
|
||
| Args: | ||
| ctx: The current work context (provides the global cooldown). | ||
| req: The package requirement being resolved. | ||
| req_type: The requirement type (top-level, install, etc.). | ||
|
|
||
| Returns: | ||
| The cooldown to pass to the provider, or ``None`` if disabled. | ||
| """ | ||
| if req_type == RequirementType.TOP_LEVEL and _has_equality_pin(req): | ||
| logger.info( | ||
| "%s: cooldown bypassed — top-level requirement uses == pin", | ||
| req.name, | ||
| ) | ||
|
Comment on lines
+161
to
+164
Collaborator
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. There is no need to prefix the log message with the package name. It's automatically added with logger context. Also please keep log lines ASCII-only. There is an em-dash. |
||
| return None | ||
|
|
||
| per_package_days = ctx.package_build_info(req).resolver_min_release_age | ||
| global_cooldown = ctx.cooldown | ||
| if per_package_days is None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| from packaging.version import Version | ||
|
|
||
| from fromager import candidate, context, packagesettings, resolver, sources, wheels | ||
| from fromager.requirements_file import RequirementType | ||
|
|
||
| _BOOTSTRAP_TIME = datetime.datetime(2026, 3, 26, 0, 0, 0, tzinfo=datetime.UTC) | ||
| _COOLDOWN_7_DAYS = datetime.timedelta(days=7) | ||
|
|
@@ -860,3 +861,47 @@ def test_compute_max_age_cutoff_disabled( | |
| """_compute_max_age_cutoff returns None when max_release_age is not set.""" | ||
| cutoff = resolver._compute_max_age_cutoff(tmp_context) | ||
| assert cutoff is None | ||
|
|
||
|
|
||
| def test_resolve_package_cooldown_exempt_toplevel_equality_pin( | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Top-level == pin bypasses cooldown.""" | ||
| ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN) | ||
| result = resolver.resolve_package_cooldown( | ||
| ctx, Requirement("test-pkg==1.3.2"), req_type=RequirementType.TOP_LEVEL | ||
| ) | ||
| assert result is None | ||
|
|
||
|
|
||
| def test_resolve_package_cooldown_enforced_transitive_equality_pin( | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Transitive == pin does NOT bypass cooldown.""" | ||
| ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN) | ||
| result = resolver.resolve_package_cooldown( | ||
| ctx, Requirement("test-pkg==1.3.2"), req_type=RequirementType.INSTALL | ||
| ) | ||
| assert result is _COOLDOWN | ||
|
|
||
|
|
||
| def test_resolve_package_cooldown_enforced_toplevel_no_pin( | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Top-level requirement without == still gets cooldown.""" | ||
| ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN) | ||
| result = resolver.resolve_package_cooldown( | ||
| ctx, Requirement("test-pkg>=1.0"), req_type=RequirementType.TOP_LEVEL | ||
| ) | ||
| assert result is _COOLDOWN | ||
|
|
||
|
|
||
| def test_resolve_package_cooldown_none_req_type_not_exempt( | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Unknown req_type (None) with == does NOT bypass cooldown.""" | ||
| ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN) | ||
| result = resolver.resolve_package_cooldown( | ||
| ctx, Requirement("test-pkg==1.3.2"), req_type=None | ||
| ) | ||
| assert result is _COOLDOWN | ||
|
Comment on lines
+866
to
+907
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. Add a boundary test for wildcard equality ( These tests cover the core matrix well, but they don’t guard the exact-pin boundary. Add one case asserting top-level Suggested test+def test_resolve_package_cooldown_toplevel_wildcard_equality_not_exempt(
+ tmp_path: pathlib.Path,
+) -> None:
+ """Top-level wildcard equality is not an exact pin, so cooldown remains active."""
+ ctx = _make_ctx(tmp_path, cooldown=_COOLDOWN)
+ result = resolver.resolve_package_cooldown(
+ ctx, Requirement("test-pkg==1.*"), req_type=RequirementType.TOP_LEVEL
+ )
+ assert result is _COOLDOWN🤖 Prompt for AI Agents |
||
Uh oh!
There was an error while loading. Please reload this page.
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.
🧩 Analysis chain
🏁 Script executed:
Repository: python-wheel-build/fromager
Length of output: 97
Exclude wildcard pins from
==operator exemptionThe function
_has_equality_pin()currently exempts any requirement with==operator, including wildcards like==1.*. This widens the cooldown bypass beyond exact pins and weakens the protection model.Fix
def _has_equality_pin(req: Requirement) -> bool: """Return True if the requirement contains an ``==`` version specifier.""" - return any(spec.operator == "==" for spec in req.specifier) + return any( + spec.operator == "==" and "*" not in spec.version + for spec in req.specifier + )🤖 Prompt for AI Agents
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.
This is a valid concern.
The function also does not handle edge cases correctly:
I recommend that you only return true if the requirement has exactly one specifier and the specifier is
==without a glob.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.
@tiran, agreed. Here's the updated implementation that applies both constraints — exactly one specifier,==operator, no wildcard:This correctly rejects:
foo==1.*— wildcard pinfoo==1,>2— compound specifier (not a pure exact pin)foo>=1.0— non-equality specifier