feat(bootstrap): add --max-release-age for multiple-versions mode#1089
Merged
LalatenduMohanty merged 1 commit intopython-wheel-build:mainfrom Apr 28, 2026
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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)
📝 WalkthroughWalkthroughAdds date-based release-age filtering to bootstrap resolution. Introduces a Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
Author
|
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
EmilienM
previously approved these changes
Apr 23, 2026
dhellmann
reviewed
Apr 23, 2026
rd4398
force-pushed
the
date-range-filtering
branch
2 times, most recently
from
63ed4fa to
e0fa9e5
Compare
LalatenduMohanty
requested changes
Apr 24, 2026
LalatenduMohanty
requested changes
Apr 24, 2026
rd4398
force-pushed
the
date-range-filtering
branch
from
42c83fb to
43ac347
Compare
dhellmann
reviewed
Apr 27, 2026
LalatenduMohanty
requested changes
Apr 27, 2026
rd4398
force-pushed
the
date-range-filtering
branch
from
5b048bd to
ebd42b9
Compare
LalatenduMohanty
previously approved these changes
Apr 27, 2026
Member
|
@mergify rebase |
Contributor
|
Deprecation notice: This pull request comes from a fork and was rebased using |
Contributor
✅ Branch has been successfully rebased |
LalatenduMohanty
force-pushed
the
date-range-filtering
branch
from
ebd42b9 to
7821ded
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_cooldown.py (1)
714-716: Avoid log-text assertions in these unit testsThese checks couple tests to log wording instead of behavior. The version-set assertions already validate the filtering logic.
Proposed simplification
- assert "found 3 candidate(s)" in caplog.text - assert "have 2 candidate(s)" in caplog.text - assert "published within" in caplog.text @@ - assert "found 3 candidate(s)" in caplog.text - assert "have 1 candidate(s)" in caplog.text - assert "published within" in caplog.textBased on learnings: In this project’s Python test suite, avoid asserting on exact log output strings since they are brittle implementation details.
Also applies to: 738-740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cooldown.py` around lines 714 - 716, Remove the brittle log-text assertions that inspect caplog.text (the three asserts containing "found 3 candidate(s)", "have 2 candidate(s)", and "published within" and the similar trio at the later block) and rely on the existing version-set/assertions that validate filtering behavior; specifically, delete or comment out those caplog-based assertions in tests/test_cooldown.py so the test only asserts on functional outputs (e.g., the version/filter result variables) instead of exact log wording.tests/test_bootstrap.py (1)
745-745: Drop the exact log-string assertion from this CLI test
tmp_context.max_release_age == timedelta(days=45)already verifies the behavior. The log text check adds brittleness without increasing confidence.Proposed change
- assert "rejecting versions older than 45 days" in caplog.textBased on learnings: In this project’s Python test suite, avoid asserting on 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` at line 745, Remove the brittle exact log-string assertion in the CLI test: delete the line asserting "rejecting versions older than 45 days" in tests/test_bootstrap.py and rely on the existing assertion that tmp_context.max_release_age == timedelta(days=45) to verify behavior; ensure no other tests depend on caplog.text here and keep the tmp_context.max_release_age check as the authoritative verification.e2e/test_bootstrap_max_release_age.sh (1)
76-84: Prefer functional E2E checks over log-content checksThe wheel presence/absence assertions already prove filtering behavior. Grepping log phrasing is brittle and can fail on harmless wording changes.
Proposed change
-# Verify that max-release-age filtering was applied (check log) -echo "" -echo "Checking log for max-release-age filtering..." -if grep -q "published within.*days" "$OUTDIR/bootstrap.log"; then - echo "✓ Log confirms max-release-age filtering was applied" -else - echo "✗ No max-release-age filtering found in log" - exit 1 -fiBased on learnings: In fromager tests, prefer verifying functional behavior over asserting log output strings.
🤖 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 76 - 84, The test currently greps bootstrap.log for the phrase "published within.*days" (the block using grep -q on "$OUTDIR/bootstrap.log") which is brittle; remove that log-content check and instead assert the filtering via the existing functional checks (the wheel presence/absence assertions already in this script), i.e. delete the if/then/else grep block and replace it with a short comment stating that filtering is validated by the wheel artifact assertions, referencing the OUTDIR/bootstrap.log and the wheel existence checks in this script so maintainers know where the functional assertions live.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_cooldown.py`:
- Around line 762-779: The test test_max_release_age_all_too_old_raises asserts
a ResolverException when all candidates exceed max_age_cutoff, but the PR
changes resolvers.find_all_matching_from_provider behavior to retain all
candidates and emit a warning instead of raising; update the test to call
resolver.find_all_matching_from_provider with provider =
resolver.PyPIProvider(include_sdists=True) and Requirement("test-pkg"), assert
it returns the expected candidate list (e.g., length 3 or contains the versions
from _cooldown_json_response) and assert a warning was emitted (use pytest.warns
or caplog) about "max-release-age cutoff" rather than expecting
resolvelib.resolvers.ResolverException. Ensure you reference the test function
name test_max_release_age_all_too_old_raises and the function
find_all_matching_from_provider when making the change.
---
Nitpick comments:
In `@e2e/test_bootstrap_max_release_age.sh`:
- Around line 76-84: The test currently greps bootstrap.log for the phrase
"published within.*days" (the block using grep -q on "$OUTDIR/bootstrap.log")
which is brittle; remove that log-content check and instead assert the filtering
via the existing functional checks (the wheel presence/absence assertions
already in this script), i.e. delete the if/then/else grep block and replace it
with a short comment stating that filtering is validated by the wheel artifact
assertions, referencing the OUTDIR/bootstrap.log and the wheel existence checks
in this script so maintainers know where the functional assertions live.
In `@tests/test_bootstrap.py`:
- Line 745: Remove the brittle exact log-string assertion in the CLI test:
delete the line asserting "rejecting versions older than 45 days" in
tests/test_bootstrap.py and rely on the existing assertion that
tmp_context.max_release_age == timedelta(days=45) to verify behavior; ensure no
other tests depend on caplog.text here and keep the tmp_context.max_release_age
check as the authoritative verification.
In `@tests/test_cooldown.py`:
- Around line 714-716: Remove the brittle log-text assertions that inspect
caplog.text (the three asserts containing "found 3 candidate(s)", "have 2
candidate(s)", and "published within" and the similar trio at the later block)
and rely on the existing version-set/assertions that validate filtering
behavior; specifically, delete or comment out those caplog-based assertions in
tests/test_cooldown.py so the test only asserts on functional outputs (e.g., the
version/filter result variables) instead of exact log wording.
🪄 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: 8daac40b-9f5f-4555-abd5-de93128e3112
📒 Files selected for processing (10)
e2e/ci_bootstrap_suite.she2e/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
✅ Files skipped from review due to trivial changes (2)
- e2e/ci_bootstrap_suite.sh
- src/fromager/sources.py
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/test_bootstrap_multiple_versions.sh
- src/fromager/commands/bootstrap.py
- src/fromager/resolver.py
- src/fromager/bootstrap_requirement_resolver.py
rd4398
force-pushed
the
date-range-filtering
branch
from
7821ded to
952b43d
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
952b43d to
f280d30
Compare
LalatenduMohanty
approved these changes
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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