feat(bootstrap): add --max-release-age for multiple-versions mode#1089
feat(bootstrap): add --max-release-age for multiple-versions mode#1089rd4398 wants to merge 1 commit intopython-wheel-build:mainfrom
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 56 minutes and 0 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis pull request implements date range filtering for release candidate selection in the bootstrapper. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
This adds --max-release-age to the bootstrap (and bootstrap-parallel) commands as part of the multiple-versions bootstrap feature. It complements the existing --min-release-age (cooldown from PR #1018) to create a sliding time window for version selection. What it does:
|
rd4398
force-pushed
the
date-range-filtering
branch
from
8f748f3 to
58b9c20
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/test_bootstrap.py (1)
744-745: Prefer behavior assertions over log-string matching.
assert tmp_context.max_release_age == timedelta(days=45)already proves the CLI wired through correctly. The log-substring check is brittle if the message wording changes — consider dropping it.assert result.exit_code == 0 assert tmp_context.max_release_age == timedelta(days=45) - assert "rejecting versions older than 45 days" in caplog.textBased on learnings: avoid asserting exact log output strings since they are brittle implementation details.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bootstrap.py` around lines 744 - 745, Remove the brittle log-string assertion and rely on the behavior assertion already present: keep the existing assert tmp_context.max_release_age == timedelta(days=45) and delete the assert "rejecting versions older than 45 days" in caplog.text; if you still want to validate logging behavior, replace the exact substring check with a non-brittle assertion that a log record at the expected level was emitted (using caplog.records or any log-level check) rather than matching message text, referencing tmp_context.max_release_age and caplog for locating the test logic.src/fromager/bootstrap_requirement_resolver.py (1)
176-179: Cross-module use of a private helper.
resolver._compute_max_age_cutoffis used here and insources.py. Since it has real callers outsideresolver.py, consider dropping the leading underscore (or wrapping it infind_all_matching_from_providerso callers don't need to compute the cutoff themselves).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/bootstrap_requirement_resolver.py` around lines 176 - 179, The call site uses the internal helper resolver._compute_max_age_cutoff from outside resolver.py; either make the helper public by renaming it to compute_max_age_cutoff, or encapsulate the cutoff computation inside resolver.find_all_matching_from_provider so callers (like the code calling resolver.find_all_matching_from_provider and other callers such as sources.py) no longer need to call a private method. Update references to resolver._compute_max_age_cutoff accordingly (rename imports/usages to resolver.compute_max_age_cutoff if you choose the public rename) or change find_all_matching_from_provider signature/implementation to accept ctx and compute the max_age_cutoff internally and remove external calls to the private helper.src/fromager/commands/bootstrap.py (1)
113-119: Considerdefault=Noneinstead of the0sentinel.Using
0to mean "disabled" while also being a legalIntRange(min=0)value is what creates the ambiguity above. Making the optionint | Nonewithdefault=Nonelets you distinguish "not provided" from "provided as 0" cleanly, and aligns with howcooldownis modelled onWorkContext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/commands/bootstrap.py` around lines 113 - 119, Change the "--max-release-age" option to use None as the disabled sentinel by setting default=None and allowing an optional int type (so the option becomes int | None rather than always 0..). Update the option definition for "max_release_age" to accept None (preserve the min=0 constraint when a value is provided) and adjust any downstream checks that currently treat 0 as "disabled" (search for usages of max_release_age) to treat None as "disabled" instead; also revise the help text to state that omitting the option disables the check and ensure the existing requirement logic for "--multiple-versions" still enforces that a value must be provided when required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/test_bootstrap_max_release_age.sh`:
- Around line 44-55: The test is non-deterministic because the current
requirement range 'certifi>=2025.11,<=2026.5' allows newer releases that can
slip past the "within window" assertions; update the hardcoded versions iterated
by the for loop (the `version` list checked by find
"$OUTDIR/wheels-repo/downloads/" for 'certifi-*.whl') and tighten the upper
bound in the requirement string to a fixed cutoff (e.g., change '<=2026.5' to
'<=2026.4.22') so the test consistently expects specific certifi wheels; also
ensure any related wheel checks (like the setuptools check referenced around the
later find pattern) use the same tightened upper bound to keep expectations
deterministic.
In `@src/fromager/commands/bootstrap.py`:
- Around line 166-170: The current check treats 0 as "disabled" but raises a
misleading message when users pass --max-release-age 0 with --multiple-versions;
update the validation around multiple_versions/max_release_age so it requires a
positive value and gives a clear message (e.g. raise
click.UsageError("--max-release-age must be > 0 when using
--multiple-versions")) or alternatively change the CLI option for
max_release_age to use click.IntRange(min=1) so the CLI enforces a positive
integer; locate the validation that references multiple_versions and
max_release_age in bootstrap.py and apply one of these fixes.
In `@src/fromager/resolver.py`:
- Around line 276-281: The max-age filter currently keeps candidates with
upload_time None which diverges from is_blocked_by_cooldown's behavior for
providers that declare supports_upload_time=True; update the max-age filtering
logic (the block using max_age_cutoff and candidates_list) to only treat missing
upload_time as acceptable when the candidate.provider.supports_upload_time is
False (i.e., if provider.supports_upload_time is True, treat upload_time None as
failing the max-age check), or alternatively add a clear docstring comment on
the max-age filter explaining the intentional asymmetry; reference
max_age_cutoff, candidates_list, upload_time, and is_blocked_by_cooldown when
making the change so the behavior is aligned or explicitly documented.
- Around line 292-298: The filter that checks release age currently falls back
to keeping all candidates and only logs a warning, which defeats the
--max-release-age filter; in the resolver.py function that filters candidates by
max release age (the block referencing req.name and candidates_list) change the
behavior to fail loudly: instead of returning all candidates when every
candidate is older than the cutoff, raise a clear exception (e.g., a ValueError
or a custom ResolutionError) so callers can decide how to proceed; additionally,
update the function docstring to state the explicit "fail on cutoff" policy and
mention the raised exception type so callers know to catch it.
---
Nitpick comments:
In `@src/fromager/bootstrap_requirement_resolver.py`:
- Around line 176-179: The call site uses the internal helper
resolver._compute_max_age_cutoff from outside resolver.py; either make the
helper public by renaming it to compute_max_age_cutoff, or encapsulate the
cutoff computation inside resolver.find_all_matching_from_provider so callers
(like the code calling resolver.find_all_matching_from_provider and other
callers such as sources.py) no longer need to call a private method. Update
references to resolver._compute_max_age_cutoff accordingly (rename
imports/usages to resolver.compute_max_age_cutoff if you choose the public
rename) or change find_all_matching_from_provider signature/implementation to
accept ctx and compute the max_age_cutoff internally and remove external calls
to the private helper.
In `@src/fromager/commands/bootstrap.py`:
- Around line 113-119: Change the "--max-release-age" option to use None as the
disabled sentinel by setting default=None and allowing an optional int type (so
the option becomes int | None rather than always 0..). Update the option
definition for "max_release_age" to accept None (preserve the min=0 constraint
when a value is provided) and adjust any downstream checks that currently treat
0 as "disabled" (search for usages of max_release_age) to treat None as
"disabled" instead; also revise the help text to state that omitting the option
disables the check and ensure the existing requirement logic for
"--multiple-versions" still enforces that a value must be provided when
required.
In `@tests/test_bootstrap.py`:
- Around line 744-745: Remove the brittle log-string assertion and rely on the
behavior assertion already present: keep the existing assert
tmp_context.max_release_age == timedelta(days=45) and delete the assert
"rejecting versions older than 45 days" in caplog.text; if you still want to
validate logging behavior, replace the exact substring check with a non-brittle
assertion that a log record at the expected level was emitted (using
caplog.records or any log-level check) rather than matching message text,
referencing tmp_context.max_release_age and caplog for locating the test logic.
🪄 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: 048507fd-9138-44b5-96b3-46d791f117f7
📒 Files selected for processing (9)
e2e/test_bootstrap_max_release_age.she2e/test_bootstrap_multiple_versions.shsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/commands/bootstrap.pysrc/fromager/context.pysrc/fromager/resolver.pysrc/fromager/sources.pytests/test_bootstrap.pytests/test_cooldown.py
| for version in 2026.2.25 2026.4.22; do | ||
| if find "$OUTDIR/wheels-repo/downloads/" -name "certifi-$version-*.whl" | grep -q .; then | ||
| echo "✓ Found wheel for certifi $version (within max-release-age window)" | ||
| else | ||
| echo "✗ Missing wheel for certifi $version" | ||
| echo "ERROR: certifi $version should be within the max-release-age window" | ||
| echo "" | ||
| echo "Found wheels:" | ||
| find "$OUTDIR/wheels-repo/downloads/" -name 'certifi-*.whl' | ||
| exit 1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
Non-deterministic if a newer certifi ships.
The version range 'certifi>=2025.11,<=2026.5' plus the "within window" assertion will still pass if PyPI publishes certifi 2026.4.30 later, but won't catch it. More importantly, if a future build-dep (setuptools) version gets a bump outside the age window, the setuptools-*.whl check on line 90 could start failing. Consider pinning the upper bound tighter (e.g., <=2026.4.22) so the test's expectations stay deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/test_bootstrap_max_release_age.sh` around lines 44 - 55, The test is
non-deterministic because the current requirement range
'certifi>=2025.11,<=2026.5' allows newer releases that can slip past the "within
window" assertions; update the hardcoded versions iterated by the for loop (the
`version` list checked by find "$OUTDIR/wheels-repo/downloads/" for
'certifi-*.whl') and tighten the upper bound in the requirement string to a
fixed cutoff (e.g., change '<=2026.5' to '<=2026.4.22') so the test consistently
expects specific certifi wheels; also ensure any related wheel checks (like the
setuptools check referenced around the later find pattern) use the same
tightened upper bound to keep expectations deterministic.
| if max_age_cutoff is not None: | ||
| filtered = [ | ||
| c | ||
| for c in candidates_list | ||
| if c.upload_time is None or c.upload_time >= max_age_cutoff | ||
| ] |
There was a problem hiding this comment.
upload_time is None handling diverges from is_blocked_by_cooldown.
For providers with supports_upload_time=True (e.g. PyPI), is_blocked_by_cooldown fails closed on missing upload_time (line 684-691), but here we always keep such candidates. If a PyPI candidate genuinely lacks metadata, the cooldown side rejects it while the max-age side admits it — same candidate, opposite verdicts in the same run.
Probably fine in practice (PyPI rarely omits upload_time), but worth aligning — either gate on provider.supports_upload_time here too, or note the intentional asymmetry in the docstring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/resolver.py` around lines 276 - 281, The max-age filter
currently keeps candidates with upload_time None which diverges from
is_blocked_by_cooldown's behavior for providers that declare
supports_upload_time=True; update the max-age filtering logic (the block using
max_age_cutoff and candidates_list) to only treat missing upload_time as
acceptable when the candidate.provider.supports_upload_time is False (i.e., if
provider.supports_upload_time is True, treat upload_time None as failing the
max-age check), or alternatively add a clear docstring comment on the max-age
filter explaining the intentional asymmetry; reference max_age_cutoff,
candidates_list, upload_time, and is_blocked_by_cooldown when making the change
so the behavior is aligned or explicitly documented.
| else: | ||
| logger.warning( | ||
| "%s: all %d versions are older than max release age cutoff, " | ||
| "keeping all versions to avoid empty resolution", | ||
| req.name, | ||
| len(candidates_list), | ||
| ) |
There was a problem hiding this comment.
Silent fallback defeats the filter's purpose.
When every candidate is older than the cutoff, the filter is bypassed and all old versions are returned anyway. A user who set --max-release-age=N explicitly asked not to build those. Falling back silently (with only a warning log) can lead to building the very stale releases the flag was meant to exclude — the exact problem issue #1037 calls out for packages like setuptools.
Consider failing loudly instead, so the caller decides:
🔧 Suggested change
- if filtered:
- dropped = len(candidates_list) - len(filtered)
- if dropped > 0:
- logger.info(
- "%s: filtered %d/%d versions by max release age",
- req.name,
- dropped,
- len(candidates_list),
- )
- candidates_list = filtered
- else:
- logger.warning(
- "%s: all %d versions are older than max release age cutoff, "
- "keeping all versions to avoid empty resolution",
- req.name,
- len(candidates_list),
- )
+ dropped = len(candidates_list) - len(filtered)
+ if dropped:
+ logger.info(
+ "%s: filtered %d/%d versions by max release age",
+ req.name,
+ dropped,
+ len(candidates_list),
+ )
+ if not filtered:
+ raise resolvelib.resolvers.ResolverException(
+ f"all {len(candidates_list)} candidate(s) for {req} were "
+ f"published before the max-release-age cutoff"
+ )
+ candidates_list = filteredAt minimum, this is worth a design-intent note in the function docstring so reviewers/users understand the deliberate "fail open" behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/resolver.py` around lines 292 - 298, The filter that checks
release age currently falls back to keeping all candidates and only logs a
warning, which defeats the --max-release-age filter; in the resolver.py function
that filters candidates by max release age (the block referencing req.name and
candidates_list) change the behavior to fail loudly: instead of returning all
candidates when every candidate is older than the cutoff, raise a clear
exception (e.g., a ValueError or a custom ResolutionError) so callers can decide
how to proceed; additionally, update the function docstring to state the
explicit "fail on cutoff" policy and mention the raised exception type so
callers know to catch it.
| if multiple_versions: | ||
| if max_release_age <= 0: | ||
| raise click.UsageError( | ||
| "--max-release-age is required when using --multiple-versions" |
There was a problem hiding this comment.
Why is the age value required?
There was a problem hiding this comment.
Without --max-release-age, --multiple-versions would attempt to build every version that matches the requirement specifier. For a package like setuptools>=60, that could mean dozens or hundreds of versions going back years. The age limit acts as a guardrail to keep the build scope manageable.
Let me know if you think I should make it optional
There was a problem hiding this comment.
It's definitely advisable. I'm not sure I want to make it required. We could require a user to provide some value, but allow 0 as an explicit choice, I guess. Let's see what others think.
| for c in candidates_list | ||
| if c.upload_time is None or c.upload_time >= max_age_cutoff | ||
| ] | ||
| if filtered: |
There was a problem hiding this comment.
We're going to want some structure to the logging as we add more filtering features. So maybe we log the full number of versions found, then for each filter step how many remain and the criteria used to do the filtering. Something like this, for example:
INFO have 100 versions of foo>=1.0.0
INFO have 10 version of foo>=1.0.0 older than 30 days
INFO have 1 version of foo>=1.0.0 at least 14 days old
There was a problem hiding this comment.
makes sense. I have attempted to add some structure to logging in my latest push
rd4398
force-pushed
the
date-range-filtering
branch
from
35c8ffc to
642bd45
Compare
Add --max-release-age flag to bootstrap command to filter out package versions older than N days. This flag is required when --multiple-versions is enabled, preventing builds of hundreds of old releases. Together with --min-release-age (cooldown), it creates a sliding time window for version selection in the multiple-versions bootstrap feature. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
rd4398
force-pushed
the
date-range-filtering
branch
from
63ed4fa to
e0fa9e5
Compare
3 participants
Add --max-release-age flag to bootstrap command to filter out package versions older than N days. This flag is required when --multiple-versions is enabled, preventing builds of hundreds of old releases. Together with --min-release-age (cooldown), it creates a sliding time window for version selection in the multiple-versions bootstrap feature.
Closes #1037