Skip to content

Commit 699995e

Browse files
rd4398claude
andcommitted
fix(resolver): use two-step resolution for age filter fallback in multi-version mode
When --max-release-age filters out all candidates, the resolver's safety fallback kept every candidate, causing multi-version bootstrap to attempt building all versions. Now the bootstrapper checks the wheel cache first: if a cached wheel exists the package is skipped; otherwise it re-resolves without the age filter as a last resort. Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
1 parent 029327a commit 699995e

5 files changed

Lines changed: 231 additions & 14 deletions

File tree

src/fromager/bootstrap_requirement_resolver.py

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,20 @@ def __init__(
4040
self,
4141
ctx: context.WorkContext,
4242
prev_graph: DependencyGraph | None = None,
43+
multiple_versions: bool = False,
4344
) -> None:
4445
"""Initialize requirement resolver.
4546
4647
Args:
4748
ctx: Work context with constraints and settings
4849
prev_graph: Optional previous dependency graph for caching
50+
multiple_versions: If ``True``, age filtering returns an empty
51+
list instead of falling back to all candidates, letting the
52+
caller decide how to handle the case.
4953
"""
5054
self.ctx = ctx
5155
self.prev_graph = prev_graph
56+
self.multiple_versions = multiple_versions
5257
# Session-level resolution cache to avoid re-resolving same requirements
5358
# Key: (requirement_string, pre_built) to distinguish source vs prebuilt
5459
# Value: tuple of (url, version) tuples sorted by version (highest first)
@@ -65,6 +70,7 @@ def resolve(
6570
parent_req: Requirement | None = None,
6671
pre_built: bool | None = None,
6772
return_all_versions: bool = False,
73+
skip_age_filter: bool = False,
6874
) -> list[tuple[str, Version]]:
6975
"""Resolve package requirement to matching version(s).
7076
@@ -81,6 +87,9 @@ def resolve(
8187
If None (default), uses package build info to determine.
8288
return_all_versions: If True, return all matching versions. If False,
8389
return only the highest matching version.
90+
skip_age_filter: If ``True``, resolve without applying
91+
``max_release_age`` filtering. Used for fallback resolution
92+
when the initial age-filtered attempt returned no candidates.
8493
8594
Returns:
8695
List of (url, version) tuples sorted by version (highest first).
@@ -104,25 +113,38 @@ def resolve(
104113
f"Git URL requirements must be handled by Bootstrapper: {req}"
105114
)
106115

107-
# Check session cache (keyed by requirement + pre_built)
108-
cached_result = self.get_cached_resolution(req, pre_built)
109-
if cached_result is not None:
110-
logger.debug(f"resolved {req} from cache")
111-
return list(cached_result) if return_all_versions else [cached_result[0]]
116+
# Skip session cache when age filter is overridden to avoid
117+
# mixing filtered and unfiltered results.
118+
if not skip_age_filter:
119+
cached_result = self.get_cached_resolution(req, pre_built)
120+
if cached_result is not None:
121+
logger.debug(f"resolved {req} from cache")
122+
return (
123+
list(cached_result) if return_all_versions else [cached_result[0]]
124+
)
112125

113126
# Resolve using strategies
114-
results = self._resolve(req, req_type, parent_req, pre_built)
127+
results = self._resolve(
128+
req, req_type, parent_req, pre_built, skip_age_filter=skip_age_filter
129+
)
115130

116-
# Cache the result
117-
self.cache_resolution(req, pre_built, results)
118-
return results if return_all_versions else [results[0]]
131+
# Cache non-empty results from normal (age-filtered) resolution only.
132+
if results and not skip_age_filter:
133+
self.cache_resolution(req, pre_built, results)
134+
135+
if return_all_versions:
136+
return results
137+
if not results:
138+
raise RuntimeError(f"No versions found for {req}")
139+
return [results[0]]
119140

120141
def _resolve(
121142
self,
122143
req: Requirement,
123144
req_type: RequirementType,
124145
parent_req: Requirement | None,
125146
pre_built: bool,
147+
skip_age_filter: bool = False,
126148
) -> list[tuple[str, Version]]:
127149
"""Internal resolution logic without caching.
128150
@@ -135,6 +157,7 @@ def _resolve(
135157
req_type: Type of requirement
136158
parent_req: Parent requirement from dependency chain
137159
pre_built: Whether to resolve prebuilt (True) or source (False)
160+
skip_age_filter: If ``True``, skip ``max_release_age`` filtering.
138161
139162
Returns:
140163
List of (url, version) tuples sorted by version (highest first)
@@ -175,9 +198,14 @@ def _resolve(
175198
sdist_server_url=resolver.PYPI_SERVER_URL,
176199
req_type=req_type,
177200
)
178-
max_age_cutoff = resolver._compute_max_age_cutoff(self.ctx)
201+
max_age_cutoff = (
202+
None if skip_age_filter else resolver._compute_max_age_cutoff(self.ctx)
203+
)
179204
return resolver.find_all_matching_from_provider(
180-
provider, req, max_age_cutoff=max_age_cutoff
205+
provider,
206+
req,
207+
max_age_cutoff=max_age_cutoff,
208+
fallback_on_empty_age_filter=not self.multiple_versions,
181209
)
182210

183211
def get_cached_resolution(

src/fromager/bootstrapper.py

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ def __init__(
173173
self._resolver = bootstrap_requirement_resolver.BootstrapRequirementResolver(
174174
ctx=ctx,
175175
prev_graph=prev_graph,
176+
multiple_versions=multiple_versions,
176177
)
177178
# Push items onto the stack as we start to resolve their
178179
# dependencies so at the end we have a list of items that need to
@@ -258,6 +259,7 @@ def resolve_versions(
258259
req: Requirement,
259260
req_type: RequirementType,
260261
return_all_versions: bool = False,
262+
skip_age_filter: bool = False,
261263
) -> list[tuple[str, Version]]:
262264
"""Resolve version(s) of a requirement.
263265
@@ -272,6 +274,8 @@ def resolve_versions(
272274
req_type: Type of requirement
273275
return_all_versions: If True, return all matching versions.
274276
If False, return only highest version.
277+
skip_age_filter: If ``True``, resolve without applying
278+
``max_release_age`` filtering.
275279
276280
Returns:
277281
List of (url, version) tuples. Contains one item when
@@ -307,6 +311,7 @@ def resolve_versions(
307311
req_type=req_type,
308312
parent_req=parent_req,
309313
return_all_versions=return_all_versions,
314+
skip_age_filter=skip_age_filter,
310315
)
311316

312317
def _processing_build_requirement(self, current_req_type: RequirementType) -> bool:
@@ -669,6 +674,26 @@ def _find_cached_wheel(
669674

670675
return None, None
671676

677+
def _has_any_cached_wheel(self, req: Requirement) -> bool:
678+
"""Check if ANY version of a package has a cached wheel locally.
679+
680+
Scans local wheel directories for any ``.whl`` file matching the
681+
package name. Used to determine whether a package can be safely
682+
skipped when all versions are filtered by ``max-release-age``.
683+
"""
684+
prefix = finders._dist_name_to_filename(req.name).lower() + "-"
685+
for search_dir in (
686+
self.ctx.wheels_build,
687+
self.ctx.wheels_downloads,
688+
self.ctx.wheels_prebuilt,
689+
):
690+
if not search_dir.exists():
691+
continue
692+
for whl in search_dir.glob("*.whl"):
693+
if whl.name.lower().startswith(prefix):
694+
return True
695+
return False
696+
672697
def _get_install_dependencies(
673698
self,
674699
req: Requirement,
@@ -1341,17 +1366,49 @@ def _phase_resolve(self, item: WorkItem) -> list[WorkItem]:
13411366
wheels are already cached to avoid redundant builds and
13421367
transitive dependency processing.
13431368
1369+
When ``max-release-age`` filtering yields no candidates in
1370+
multiple-version mode, checks the local wheel cache:
1371+
1372+
- Cached wheel exists: skip the package (return empty list).
1373+
- No cached wheel: re-resolve without the age filter so the
1374+
package can still be built for the first time.
1375+
13441376
Returns:
13451377
One START-phase item per resolved version that needs building.
1346-
Empty list if all versions are already cached.
1378+
Empty list if all versions are already cached or skipped.
13471379
"""
13481380
resolved_versions = self.resolve_versions(
13491381
item.req,
13501382
item.req_type,
13511383
return_all_versions=self.multiple_versions,
13521384
)
1385+
13531386
if not resolved_versions:
1354-
raise RuntimeError(f"Could not resolve any versions for {item.req}")
1387+
if not self.multiple_versions:
1388+
raise RuntimeError(f"Could not resolve any versions for {item.req}")
1389+
1390+
if self._has_any_cached_wheel(item.req):
1391+
logger.info(
1392+
"%s: all versions filtered by max-release-age, "
1393+
"cached wheel exists; skipping",
1394+
item.req.name,
1395+
)
1396+
return []
1397+
1398+
# No cached wheel — fall back to unfiltered resolution
1399+
logger.warning(
1400+
"%s: all versions filtered by max-release-age and no "
1401+
"cached wheel exists; falling back to unfiltered resolution",
1402+
item.req.name,
1403+
)
1404+
resolved_versions = self.resolve_versions(
1405+
item.req,
1406+
item.req_type,
1407+
return_all_versions=True,
1408+
skip_age_filter=True,
1409+
)
1410+
if not resolved_versions:
1411+
raise RuntimeError(f"Could not resolve any versions for {item.req}")
13551412

13561413
if self.multiple_versions:
13571414
logger.info(f"resolved {len(resolved_versions)} version(s) for {item.req}")

src/fromager/resolver.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,7 @@ def find_all_matching_from_provider(
243243
provider: BaseProvider,
244244
req: Requirement,
245245
max_age_cutoff: datetime.datetime | None = None,
246+
fallback_on_empty_age_filter: bool = True,
246247
) -> list[tuple[str, Version]]:
247248
"""Find all matching candidates from provider without full dependency resolution.
248249
@@ -255,6 +256,10 @@ def find_all_matching_from_provider(
255256
max_age_cutoff: If set, reject candidates published before this time.
256257
If all candidates are older than the cutoff, all are kept and
257258
a warning is emitted to avoid empty resolution.
259+
fallback_on_empty_age_filter: If ``True`` (default), keep all
260+
candidates when age filtering would produce an empty result.
261+
If ``False``, return an empty list instead, allowing the
262+
caller to implement its own fallback strategy.
258263
259264
Returns list of (url, version) tuples sorted by version (highest first).
260265
@@ -315,7 +320,7 @@ def find_all_matching_from_provider(
315320
)
316321
if filtered:
317322
candidates_list = filtered
318-
else:
323+
elif fallback_on_empty_age_filter:
319324
logger.warning(
320325
"%s: all %d candidate(s) of %s are older than %d days, "
321326
"keeping all to avoid empty resolution",
@@ -324,6 +329,15 @@ def find_all_matching_from_provider(
324329
req,
325330
max_age_days,
326331
)
332+
else:
333+
logger.info(
334+
"%s: all %d candidate(s) of %s are older than %d days",
335+
req.name,
336+
len(candidates_list),
337+
req,
338+
max_age_days,
339+
)
340+
candidates_list = []
327341

328342
# Convert candidates to list of (url, version) tuples
329343
# Candidates are sorted by version (highest first) by BaseProvider.find_matches()

tests/test_bootstrapper_iterative.py

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,100 @@ def test_no_filtering_in_single_version_mode(
353353
assert len(result) == 1
354354
mock_cache.assert_not_called()
355355

356+
def test_age_filtered_all_with_cache_skips_package(
357+
self, tmp_context: WorkContext
358+
) -> None:
359+
"""When all versions are age-filtered and a cached wheel exists, skip entirely."""
360+
bt = bootstrapper.Bootstrapper(tmp_context, multiple_versions=True)
361+
item = _make_resolve_item()
362+
363+
with (
364+
patch.object(bt, "resolve_versions", return_value=[]),
365+
patch.object(bt, "_has_any_cached_wheel", return_value=True),
366+
):
367+
result = bt._phase_resolve(item)
368+
369+
assert result == []
370+
371+
def test_age_filtered_all_without_cache_falls_back(
372+
self, tmp_context: WorkContext
373+
) -> None:
374+
"""When all versions are age-filtered and no cached wheel, re-resolve without filter."""
375+
bt = bootstrapper.Bootstrapper(tmp_context, multiple_versions=True)
376+
item = _make_resolve_item()
377+
378+
call_count = 0
379+
380+
def mock_resolve(
381+
req: Requirement,
382+
req_type: RequirementType,
383+
return_all_versions: bool = False,
384+
skip_age_filter: bool = False,
385+
) -> list:
386+
nonlocal call_count
387+
call_count += 1
388+
if call_count == 1:
389+
return []
390+
return [("url-1.0", Version("1.0"))]
391+
392+
with (
393+
patch.object(bt, "resolve_versions", side_effect=mock_resolve),
394+
patch.object(bt, "_has_any_cached_wheel", return_value=False),
395+
):
396+
result = bt._phase_resolve(item)
397+
398+
assert len(result) == 1
399+
assert result[0].resolved_version == Version("1.0")
400+
assert call_count == 2
401+
402+
def test_empty_resolution_raises_in_single_version_mode(
403+
self, tmp_context: WorkContext
404+
) -> None:
405+
"""In single-version mode, empty resolution raises RuntimeError."""
406+
bt = bootstrapper.Bootstrapper(tmp_context, multiple_versions=False)
407+
item = _make_resolve_item()
408+
409+
with (
410+
patch.object(bt, "resolve_versions", return_value=[]),
411+
pytest.raises(RuntimeError, match="Could not resolve"),
412+
):
413+
bt._phase_resolve(item)
414+
415+
416+
class TestHasAnyCachedWheel:
417+
def test_finds_wheel_in_build_dir(self, tmp_context: WorkContext) -> None:
418+
"""Detects wheel in build directory."""
419+
bt = bootstrapper.Bootstrapper(tmp_context)
420+
tmp_context.wheels_build.mkdir(parents=True, exist_ok=True)
421+
(tmp_context.wheels_build / "testpkg-1.0-py3-none-any.whl").touch()
422+
assert bt._has_any_cached_wheel(Requirement("testpkg")) is True
423+
424+
def test_finds_wheel_in_downloads_dir(self, tmp_context: WorkContext) -> None:
425+
"""Detects wheel in downloads directory."""
426+
bt = bootstrapper.Bootstrapper(tmp_context)
427+
tmp_context.wheels_downloads.mkdir(parents=True, exist_ok=True)
428+
(tmp_context.wheels_downloads / "testpkg-2.0-py3-none-any.whl").touch()
429+
assert bt._has_any_cached_wheel(Requirement("testpkg")) is True
430+
431+
def test_finds_wheel_in_prebuilt_dir(self, tmp_context: WorkContext) -> None:
432+
"""Detects wheel in prebuilt directory."""
433+
bt = bootstrapper.Bootstrapper(tmp_context)
434+
tmp_context.wheels_prebuilt.mkdir(parents=True, exist_ok=True)
435+
(tmp_context.wheels_prebuilt / "testpkg-3.0-py3-none-any.whl").touch()
436+
assert bt._has_any_cached_wheel(Requirement("testpkg")) is True
437+
438+
def test_no_wheel_returns_false(self, tmp_context: WorkContext) -> None:
439+
"""Returns False when no cached wheel exists."""
440+
bt = bootstrapper.Bootstrapper(tmp_context)
441+
assert bt._has_any_cached_wheel(Requirement("nonexistent")) is False
442+
443+
def test_different_package_not_matched(self, tmp_context: WorkContext) -> None:
444+
"""Wheels for other packages are not matched."""
445+
bt = bootstrapper.Bootstrapper(tmp_context)
446+
tmp_context.wheels_downloads.mkdir(parents=True, exist_ok=True)
447+
(tmp_context.wheels_downloads / "otherpkg-1.0-py3-none-any.whl").touch()
448+
assert bt._has_any_cached_wheel(Requirement("testpkg")) is False
449+
356450

357451
class TestPhaseStart:
358452
def test_new_item_advances_to_prepare_source(

tests/test_cooldown.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -796,6 +796,30 @@ def test_max_release_age_all_too_old_keeps_all(
796796
assert "keeping all to avoid empty resolution" in caplog.text
797797

798798

799+
def test_max_release_age_all_too_old_returns_empty_when_fallback_disabled(
800+
caplog: pytest.LogCaptureFixture,
801+
) -> None:
802+
"""When fallback is disabled and all versions are too old, return empty list."""
803+
max_age_cutoff = _BOOTSTRAP_TIME + datetime.timedelta(days=1)
804+
with requests_mock.Mocker() as r:
805+
r.get(
806+
"https://pypi.org/simple/test-pkg/",
807+
json=_cooldown_json_response,
808+
headers={"Content-Type": _PYPI_SIMPLE_JSON_CONTENT_TYPE},
809+
)
810+
provider = resolver.PyPIProvider(include_sdists=True)
811+
with caplog.at_level(logging.INFO, logger="fromager.resolver"):
812+
results = resolver.find_all_matching_from_provider(
813+
provider,
814+
Requirement("test-pkg"),
815+
max_age_cutoff=max_age_cutoff,
816+
fallback_on_empty_age_filter=False,
817+
)
818+
assert results == []
819+
assert "all 3 candidate(s)" in caplog.text
820+
assert "keeping all to avoid empty resolution" not in caplog.text
821+
822+
799823
def test_max_release_age_candidates_without_upload_time_pass_through() -> None:
800824
"""Candidates without upload_time are not filtered out by max-release-age."""
801825
no_timestamp_response = {

0 commit comments

Comments
 (0)