Fix: Multi version bootstrap age filter fallback for edge cases - #1289
Fix: Multi version bootstrap age filter fallback for edge cases #1289rd4398 wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 57 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe resolver now uses the Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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.
🧹 Nitpick comments (2)
src/fromager/resolver.py (1)
304-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd requirement log context.
These logs describe one requirement but do not use
req_ctxvar_context(). Wrap the age-filter operation in the established requirement context before logging. As per coding guidelines, “Usereq_ctxvar_context()for per-requirement logging.”Also applies to: 346-357
🤖 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 304 - 313, Wrap the age-filter logging paths in the established req_ctxvar_context() requirement context before emitting messages. Update the branches around the constrained-package log and the corresponding age-filter operation near the later referenced block, preserving the existing filtering behavior while ensuring all per-requirement logs include the request context.Source: Coding guidelines
tests/test_cooldown.py (1)
807-828: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove log-message assertions from these behavior tests.
The result and wheel assertions already verify the required behavior. Log text is diagnostic output and can change without changing behavior.
tests/test_cooldown.py#L807-L828: removecaplogassertions after verifying the empty result.tests/test_cooldown.py#L831-L853: removecaplogassertions after verifying version2.0.0.tests/test_cooldown.py#L1008-L1036: removecaplogassertions after verifying constrained version1.2.2.tests/test_cooldown.py#L1039-L1060: remove the negative log assertion after verifying the empty result.e2e/test_bootstrap_age_constraint_bypass.sh#L59-L67: remove the loggrep; retain the wheel assertion.e2e/test_bootstrap_age_fallback_newest.sh#L51-L59: remove the loggrep; retain the wheel-count and version assertions.Based on learnings, “avoid asserting on exact log output strings since they are brittle implementation details.”
🤖 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 `@tests/test_cooldown.py` around lines 807 - 828, Remove brittle log-output assertions while preserving all behavior assertions: in tests/test_cooldown.py ranges 807-828, 831-853, 1008-1036, and 1039-1060, delete the caplog assertions after the existing result/version checks; in e2e/test_bootstrap_age_constraint_bypass.sh range 59-67 and e2e/test_bootstrap_age_fallback_newest.sh range 51-59, remove only the log grep commands and retain the wheel, count, and version assertions.Source: Learnings
🤖 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/resolver.py`:
- Around line 304-313: Wrap the age-filter logging paths in the established
req_ctxvar_context() requirement context before emitting messages. Update the
branches around the constrained-package log and the corresponding age-filter
operation near the later referenced block, preserving the existing filtering
behavior while ensuring all per-requirement logs include the request context.
In `@tests/test_cooldown.py`:
- Around line 807-828: Remove brittle log-output assertions while preserving all
behavior assertions: in tests/test_cooldown.py ranges 807-828, 831-853,
1008-1036, and 1039-1060, delete the caplog assertions after the existing
result/version checks; in e2e/test_bootstrap_age_constraint_bypass.sh range
59-67 and e2e/test_bootstrap_age_fallback_newest.sh range 51-59, remove only the
log grep commands and retain the wheel, count, and version assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6f59d0e-48a2-4967-9d79-35f7577e6234
📒 Files selected for processing (7)
e2e/ci_bootstrap_suite.she2e/test_bootstrap_age_constraint_bypass.she2e/test_bootstrap_age_fallback_newest.shsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/resolver.pytests/test_bootstrap_requirement_resolver.pytests/test_cooldown.py
andre-motta
left a comment
andre-motta
left a comment
There was a problem hiding this comment.
LGTM. Lets test this with https://pypi.org/project/dead/ on walkerpass or similar before releasing!
|
The fix for CI failures are part of #1287 |
Packages with explicit constraints (e.g. `boto3==1.35.88` in constraints files) now skip age filtering entirely in `find_all_matching_from_provider()`. A constraint is explicit user intent and should not be silently overridden by the age heuristic. Previously in multi-version mode with `--max-release-age`, a constrained package whose pinned version was older than the age window would have all candidates removed, causing silent resolution failure. In single-version mode the fallback kept all candidates anyway, but emitted a misleading "keeping all to avoid empty resolution" warning. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
3dd9464 to
e6b9dec
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM
Tested on local system (x86_64) with old (main: 0.92.1.dev13) and new (PR: 0.92.1.dev15) side-by-side using --multiple-versions.
tomli>=2.0,<=2.0.2 with --max-release-age=1
| Scenario | OLD (main) | NEW (PR) |
|---|---|---|
| Unconstrained | all 3 candidate(s) older than 1 days → empty, skipped |
falling back to newest version 2.0.2 → resolves |
Constrained tomli==2.0.0 |
all 1 candidate(s) older than 1 days → pinned version skipped |
skipping age filter for constrained package → resolves 2.0.0 |
dead>=1.0 (latest 2.1.0 uploaded 2025-02-08, ~543 days old)
| Scenario | OLD (main) | NEW (PR) |
|---|---|---|
--max-release-age=30 |
all 11 candidate(s) older than 30 days → empty, skipped |
falling back to newest version 2.1.0 → resolves |
--max-release-age=365 |
all 11 candidate(s) older than 365 days → empty, skipped |
falling back to newest version 2.1.0 → resolves |
--max-release-age=600 |
have 3 candidate(s) within 600 days → resolves 3 |
have 3 candidate(s) within 600 days → resolves 3 |
--max-release-age=30 + dead==1.5.2 |
all 1 candidate(s) older than 30 days → pinned version skipped |
skipping age filter for constrained package → resolves 1.5.2 |
When versions fall within the age window, both behave identically. The PR only changes behavior when all candidates are outside the window.
| req, | ||
| max_age_days, | ||
| ) | ||
| elif age_fallback == AgeFallback.NEWEST: |
There was a problem hiding this comment.
This branch is reached when filtered is empty (i.e., all candidates failed the age check). filtered is built from candidates_list, so if candidates_list itself is empty — possible when the requirement specifier matches nothing — candidates_list[0] raises IndexError. There is no guard and no unit test for this path.
Suggested fix: guard before the index access:
elif age_fallback == AgeFallback.NEWEST:
if not candidates_list:
candidates_list = []
else:
newest = candidates_list[0]
logger.info(...)
candidates_list = [newest]
Or just check candidates_list before assigning newest.
There was a problem hiding this comment.
The NEWEST branch is only reached when filtered is empty (all candidates failed the age check), but candidates_list is the original unfiltered list. It's guaranteed non-empty because provider.find_matches() raises ResolverException if zero candidates match the requirement specifier (line 829-832). The exception is re-raised at line 286-288. So by the time we reach the NEWEST branch, candidates_list always has at least one element.
|
|
||
| if max_age_cutoff is not None: | ||
| # Constraints are explicit user intent — they override age filtering. | ||
| is_constrained = provider.constraints.get_constraint(req.name) is not None |
There was a problem hiding this comment.
is_constrained is broader than "pinned"
get_constraint returns a match for any constraint type — range, exclusion, or exact pin — not just pins. So a loose constraint like tomli>=1.0 also sets is_constrained=True and skips age filtering entirely, even though the docstring frames this as bypassing only for exact pins ("explicit user intent").
Failure case: user sets tomli>=1.0 (just to exclude 0.x) plus --max-release-age=365, expecting the age filter to still apply — but it's silently skipped since any constraint match short-circuits it.
Options:
- Only bypass for exact pins (
constraint.specifier == "=="), fall through to age filtering otherwise. - Keep current behavior, but rename "pinned" → "constrained" in docstring/logs and document the tradeoff.
There was a problem hiding this comment.
sounds good, fixed in latest commit
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_max_release_age_skips_filter_for_constrained_package( |
There was a problem hiding this comment.
Constraint-bypass tests don't use production's fallback mode
Both new unit tests call find_all_matching_from_provider with age_fallback=resolver.AgeFallback.NONE, but production multi-version mode (bootstrap --multiple-versions + constraints file) uses AgeFallback.NEWEST. That combination — constrained package + NEWEST fallback — isn't unit tested; only covered implicitly by the (slow, non-isolating) e2e test.
Gap: if constraint bypass later starts interacting with fallback strategy (e.g. "skip bypass when fallback is NEWEST"), nothing here would catch the regression.
Suggested fix: add a unit test in test_cooldown.py with age_fallback=resolver.AgeFallback.NEWEST and a constrained provider, asserting the constrained version is returned rather than just the newest unconstrained one.
There was a problem hiding this comment.
done in latest commit
| # Constraints are explicit user intent — they override age filtering. | ||
| is_constrained = provider.constraints.get_constraint(req.name) is not None | ||
|
|
||
| if max_age_cutoff is not None and is_constrained: |
There was a problem hiding this comment.
is_constrained is computed unconditionally on every call, but it's only ever used inside if max_age_cutoff is not None branches. When max_age_cutoff is None (no age filter configured — the common case), the constraint lookup is wasted work.
This is minor on its own, but find_all_matching_from_provider is called once per requirement per resolution pass, and get_constraint does a dict lookup on a canonicalized name. Not expensive, but the intent is clearer if the check is co-located with its use:
if max_age_cutoff is not None:
is_constrained = provider.constraints.get_constraint(req.name) is not None
if is_constrained:
logger.info(...)
else:
# existing filter logic
Not blocking, but worth the two-line fix for clarity.
There was a problem hiding this comment.
done in latest commit
| ) | ||
|
|
||
|
|
||
| class AgeFallback(enum.StrEnum): |
There was a problem hiding this comment.
NIT:
StrEnum is unnecessary here — plain Enum would be fine
class AgeFallback(enum.StrEnum):
NONE = "none"
ALL = "all"
NEWEST = "newest"StrEnum makes members compare equal to their string values (AgeFallback.NEWEST == "newest"), which is useful when values get serialized to config files, CLI args, or JSON. None of that applies here — AgeFallback is only ever used in internal comparisons, never as a raw string. This leaves an implicit interface open: callers could pass the bare string "newest" and it would silently work, bypassing the type system.
A plain enum.Enum makes the type boundary explicit with no downside.
There was a problem hiding this comment.
Every internal enum in the codebase uses StrEnum (RequirementType, SourceType, BuildSDist, DownloadKind, BootstrapPhase). Switching to Enum would break the established convention. I would prefer keeping it this way
| class AgeFallback(enum.StrEnum): | ||
| """Strategy when max-release-age filtering removes all candidates.""" | ||
|
|
||
| NONE = "none" |
There was a problem hiding this comment.
NIT:
ALL is the default value for age_fallback in find_all_matching_from_provider and represents the original behavior. Convention in Python enums is to put the default/most-common member first. Reordering to ALL, NEWEST, NONE would match the parameter default and read naturally from most- to least-permissive.
There was a problem hiding this comment.
fixed in latest commit
Replace the boolean `fallback_on_empty_age_filter` parameter in `find_all_matching_from_provider()` with an `AgeFallback` enum that supports three strategies when max-release-age filtering removes all candidates: - `ALL`: keep every candidate (single-version default, unchanged) - `NEWEST`: keep only the single newest candidate (new) - `NONE`: return empty list Multi-version mode now uses `AgeFallback.NEWEST` so that when a dependency has no releases within the age window (e.g. bar from 2 years ago depended on by recent foo), the newest version is built instead of failing. This prevents cascading failures where the dependent package also fails to build. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com> fix(resolver): address PR review feedback - Narrow constraint bypass to exact == pins only, so range constraints like >=1.0 still go through age filtering. Reuses the existing `_has_equality_pin()` helper. - Move the constraint check inside the `max_age_cutoff is not None` block to avoid unnecessary work when age filtering is disabled. - Reorder `AgeFallback` enum members: ALL (default), NEWEST, NONE — most- to least-permissive. - Add unit test for NEWEST fallback + pinned constraint combination. - Add unit test verifying range constraints are still age-filtered. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
76a87f6 to
6434bda
Compare
|
I have approved the PR however my concern is that multi-version resolution now has three fallback layers stacked:
That's a lot of "try this, else try that" for one resolution path, and it makes the behavior hard to predict for someone reading the code cold. Single-version mode has none of this . The two modes are diverging in resolution semantics, which could become a maintenance burden as more edge cases surface. I will create a follow up issue which we can look in to resolving this post this PR merge. |
5 participants
Commit 1:
Commit 2:
Closes #1288