refactor(resolver): return lists of matching versions#975
refactor(resolver): return lists of matching versions#975rd4398 wants to merge 3 commits intopython-wheel-build:mainfrom
Conversation
rd4398
changed the title
rd4398
force-pushed
the
resolver-return-multiple-values
branch
from
31d3717 to
8fe05f7
Compare
|
The PR changes a lot of code and introduces an API-incompatible change. Alternative:
|
rd4398
force-pushed
the
resolver-return-multiple-values
branch
from
b2eaf07 to
e679584
Compare
Okay, that makes sense. I have made changes in the latest commit as per your suggestion |
rd4398
force-pushed
the
resolver-return-multiple-values
branch
from
26890f3 to
7d8965c
Compare
src/fromager/sources.py
Outdated
|
|
||
| Returns the highest matching version. | ||
| """ | ||
| results = resolve_source_all( |
There was a problem hiding this comment.
Reusing resolve_source_all here means that all of the existing plugins that only return 1 value won't work at all any more. We should think about how to make this more backwards compatible by supporting 2 plugin points, one that returns a list and one that returns a single value. If the list plugin is not there then we look for the single value plugin and use it.
There was a problem hiding this comment.
Yeah, I agree. I have made the change in latest commit
rd4398
force-pushed
the
resolver-return-multiple-values
branch
from
717620e to
d5950de
Compare
src/fromager/resolver.py
Outdated
| candidates = provider.find_matches( | ||
| identifier=identifier, | ||
| requirements={identifier: [req]}, | ||
| incompatibilities={}, |
There was a problem hiding this comment.
This is always empty, is this as designed?
There was a problem hiding this comment.
now that resolve_from_provider() do not use resolvelib.Resolver() may be we should rename it something like find_all_matching_from_provider() to signal the changed semantics
There was a problem hiding this comment.
This works today only because get_dependencies() returns [] — there are no transitive dependencies to resolve. But this is a fundamental behavioral change:
- Permanently passes empty incompatibilities, so no version is ever excluded
- Bypasses resolvelib's backtracking logic entirely
- Will silently produce incorrect results if get_dependencies() is ever extended
At minimum, a comment explaining why this is safe should be added:
# Bypass resolvelib's resolver to collect all matching candidates rather than
# just the single best one. This is safe because get_dependencies() returns []
# (no transitive deps to resolve). If get_dependencies() is ever extended,
# this must be revisited to use resolvelib's full resolution
There was a problem hiding this comment.
Hmm, I agree. I have added relevant comments and renamed the function to find_all_matching_from_provider()
@tiran I discussed with Doug about the next steps and we agreed that following would be the steps that I will be working on:
|
rd4398
force-pushed
the
resolver-return-multiple-values
branch
from
d5950de to
779c8ee
Compare
📝 WalkthroughWalkthroughThis refactoring transitions the resolver architecture from single-item resolution to a batch-oriented approach. The Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/fromager/sources.py (1)
149-223:⚠️ Potential issue | 🟠 MajorNormalize new
resolve_source_all()plugin results.The new hook accepts arbitrary lists, but the legacy wrapper immediately does
results[0]. If a plugin returns[]or leaves candidates in source order instead of highest-first,resolve_source()either raisesIndexErroror picks the wrong version. Reject empty results and sort here so the old single-result API stays compatible.Suggested fix
if not isinstance(resolver_results, list): raise ValueError( f"expected list of (url, version) tuples, got {type(resolver_results)}" ) - # Validate each tuple in the list - for _url, version in resolver_results: + normalized_results: list[tuple[str, Version]] = [] + for url, version in resolver_results: if not isinstance(version, Version): raise ValueError(f"expected Version, got {type(version)}: {version}") + normalized_results.append((str(url), version)) - return [(str(url), version) for url, version in resolver_results] + if not normalized_results: + raise ValueError(f"expected at least one source candidate for {req}") + + return sorted(normalized_results, key=lambda item: item[1], reverse=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/sources.py` around lines 149 - 223, resolve_source currently takes results from resolve_source_all and immediately indexes results[0], which can raise IndexError or pick the wrong candidate; update resolve_source to (1) validate that results is non-empty and raise a ValueError with a clear message if empty, and (2) sort the list of (url, Version) tuples by Version descending (e.g., sorted(results, key=lambda t: t[1], reverse=True)) so the highest version is first before selecting results[0]; reference the resolve_source and resolve_source_all functions and ensure you operate on the same tuple shape returned by resolve_source_all.src/fromager/requirement_resolver.py (1)
204-234:⚠️ Potential issue | 🟠 MajorDon’t leak the mutable cache list.
The cache now stores and returns the same list object. Any caller that mutates the result from
resolve_all()in place (pop,sort,append, etc.) will silently poison_resolved_requirementsand change later resolutions. Copy on store/return so the cache remains internal.Suggested fix
cache_key = (str(req), pre_built) - return self._resolved_requirements.get(cache_key) + result = self._resolved_requirements.get(cache_key) + return list(result) if result is not None else None @@ cache_key = (str(req), pre_built) - self._resolved_requirements[cache_key] = result + self._resolved_requirements[cache_key] = list(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/requirement_resolver.py` around lines 204 - 234, The cached list is returned/stored by reference, allowing callers to mutate `_resolved_requirements` via the result from `get_cached_resolution`/`resolve_all`; fix by making defensive copies: in the getter (`get_cached_resolution` shown) return a shallow copy of the list (e.g., result.copy() or list(result)) instead of the stored list, and in `cache_resolution` store a shallow copy of `result` into `_resolved_requirements` (ensure you handle None safely); tuples inside the list can remain as-is since they are immutable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/resolver.py`:
- Around line 203-222: The helper must fully materialize and validate
provider.find_matches() before returning so resolve() gets a non-empty,
highest-first list: call provider.identify(req) then consume
provider.find_matches(...) into a list, catch any exception raised during
iteration and re-raise as resolvelib.resolvers.ResolverException (including
constraint via provider.constraints.get_constraint(req.name) and
provider.get_provider_description()), validate each candidate has .url and
.version, raise a ResolverException if the list is empty or any candidate lacks
those attributes, and finally sort the list by candidate.version descending
before returning [(candidate.url, candidate.version) ...]; update the code
around provider.identify and provider.find_matches accordingly.
In `@tests/test_sources.py`:
- Around line 26-33: The test only asserts that resolve_all() was called; update
test_default_download_source_from_settings to assert the actual return value of
default_resolve_source (i.e., that it returns a single best-match tuple) instead
of only checking resolve invocation—call the function under test
(default_resolve_source) with the patched dependencies and assert it returns the
expected single tuple (best candidate), using a two-candidate
resolve.return_value (e.g., two ("url", Version(...)) entries) to validate the
highest-first contract; also apply the same change to the other test block
referenced (lines 66-79) so both tests verify the wrapper's returned tuple
rather than only delegated calls.
---
Outside diff comments:
In `@src/fromager/requirement_resolver.py`:
- Around line 204-234: The cached list is returned/stored by reference, allowing
callers to mutate `_resolved_requirements` via the result from
`get_cached_resolution`/`resolve_all`; fix by making defensive copies: in the
getter (`get_cached_resolution` shown) return a shallow copy of the list (e.g.,
result.copy() or list(result)) instead of the stored list, and in
`cache_resolution` store a shallow copy of `result` into
`_resolved_requirements` (ensure you handle None safely); tuples inside the list
can remain as-is since they are immutable.
In `@src/fromager/sources.py`:
- Around line 149-223: resolve_source currently takes results from
resolve_source_all and immediately indexes results[0], which can raise
IndexError or pick the wrong candidate; update resolve_source to (1) validate
that results is non-empty and raise a ValueError with a clear message if empty,
and (2) sort the list of (url, Version) tuples by Version descending (e.g.,
sorted(results, key=lambda t: t[1], reverse=True)) so the highest version is
first before selecting results[0]; reference the resolve_source and
resolve_source_all functions and ensure you operate on the same tuple shape
returned by resolve_source_all.
🪄 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: 9d5db5b1-3d50-43ec-9197-add3deb7be5b
📒 Files selected for processing (7)
src/fromager/bootstrapper.pysrc/fromager/requirement_resolver.pysrc/fromager/resolver.pysrc/fromager/sources.pysrc/fromager/wheels.pytests/test_requirement_resolver.pytests/test_sources.py
| # Get all matching candidates directly from provider | ||
| # instead of using resolvelib's resolver which picks just one | ||
| identifier = provider.identify(req) | ||
| try: | ||
| result = rslvr.resolve([req]) | ||
| candidates = provider.find_matches( | ||
| identifier=identifier, | ||
| requirements={identifier: [req]}, | ||
| incompatibilities={}, | ||
| ) | ||
| except resolvelib.resolvers.ResolverException as err: | ||
| constraint = provider.constraints.get_constraint(req.name) | ||
| provider_desc = provider.get_provider_description() | ||
| # Include the original error message to preserve detailed information | ||
| # (e.g., file types, pre-release info from PyPIProvider) | ||
| original_msg = str(err) | ||
| raise resolvelib.resolvers.ResolverException( | ||
| f"Unable to resolve requirement specifier {req} with constraint {constraint} using {provider_desc}: {original_msg}" | ||
| ) from err | ||
| # resolvelib actually just returns one candidate per requirement. | ||
| # result.mapping is map from an identifier to its resolved candidate | ||
| candidate: Candidate | ||
| for candidate in result.mapping.values(): | ||
| return candidate.url, candidate.version | ||
| raise ValueError(f"Unable to resolve {req}") | ||
|
|
||
| # Convert candidates to list of (url, version) tuples | ||
| # Candidates are already sorted by version (highest first) | ||
| return [(candidate.url, candidate.version) for candidate in candidates] |
There was a problem hiding this comment.
Normalize provider output before exposing it.
resolve() now depends on this helper returning a non-empty, highest-first list. A custom find_matches() implementation can still yield an unsorted/empty iterable, or even fail lazily during iteration, which turns the wrapper into a wrong-version pick or an IndexError. Materialize, validate, and sort the candidates here so the contract is enforced in one place.
Suggested fix
try:
- candidates = provider.find_matches(
- identifier=identifier,
- requirements={identifier: [req]},
- incompatibilities={},
- )
+ candidates = list(
+ provider.find_matches(
+ identifier=identifier,
+ requirements={identifier: [req]},
+ incompatibilities={},
+ )
+ )
except resolvelib.resolvers.ResolverException as err:
constraint = provider.constraints.get_constraint(req.name)
provider_desc = provider.get_provider_description()
original_msg = str(err)
raise resolvelib.resolvers.ResolverException(
f"Unable to resolve requirement specifier {req} with constraint {constraint} using {provider_desc}: {original_msg}"
) from err
- # Convert candidates to list of (url, version) tuples
- # Candidates are already sorted by version (highest first)
- return [(candidate.url, candidate.version) for candidate in candidates]
+ if not candidates:
+ raise resolvelib.resolvers.ResolverException(
+ f"found no match for {req} using {provider.get_provider_description()}"
+ )
+
+ candidates = sorted(
+ candidates,
+ key=attrgetter("version", "build_tag"),
+ reverse=True,
+ )
+ return [(candidate.url, candidate.version) for candidate in candidates]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/resolver.py` around lines 203 - 222, The helper must fully
materialize and validate provider.find_matches() before returning so resolve()
gets a non-empty, highest-first list: call provider.identify(req) then consume
provider.find_matches(...) into a list, catch any exception raised during
iteration and re-raise as resolvelib.resolvers.ResolverException (including
constraint via provider.constraints.get_constraint(req.name) and
provider.get_provider_description()), validate each candidate has .url and
.version, raise a ResolverException if the list is empty or any candidate lacks
those attributes, and finally sort the list by candidate.version descending
before returning [(candidate.url, candidate.version) ...]; update the code
around provider.identify and provider.find_matches accordingly.
There was a problem hiding this comment.
Do we do anything in our providers to enforce the ordering?
There was a problem hiding this comment.
@dhellmann Yes — BaseProvider.find_matches() already enforces both guarantees:
- Non-empty: it raises
resolvelib.resolvers.ResolverExceptionif the filtered candidate list is empty. - Sorted highest-first: it returns
sorted(candidates, key=attrgetter("version", "build_tag"), reverse=True).
All built-in providers (PyPIProvider, GitHubTagProvider, GitLabTagProvider, VersionMapProvider) either inherit BaseProvider.find_matches() directly or delegate to it via super(), so the contract holds for them.
The residual risk is a custom plugin provider that overrides find_matches() without honouring the sort order or the non-empty invariant. In that case, find_all_matching_from_provider (or the old resolve_from_provider) would silently return unsorted results or an IndexError at the results[0] call site. The suggested fix in the review comment — materializing, validating, and re-sorting inside the helper — would make the contract explicit and safe regardless of what any third-party provider does.
Replace intermediate resolution functions with direct provider usage: - RequirementResolver now calls resolve_from_provider() directly - sources.resolve_source() uses get_resolver_provider plugin hook - wheels.resolve_prebuilt_wheel() uses get_resolver_provider plugin hook - Removed resolve_all() methods per architect guidance This refactoring simplifies the resolution architecture by eliminating the intermediate *_all() function layer while maintaining all existing functionality. All resolution now goes through the provider pattern with overrides.find_and_invoke() + resolver.resolve_from_provider(). Tests updated to match new implementation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
rd4398
force-pushed
the
resolver-return-multiple-values
branch
from
2331411 to
3cd2c8e
Compare
Rename class and files to better reflect purpose: - RequirementResolver → BootstrapRequirementResolver - requirement_resolver.py → bootstrap_requirement_resolver.py - test_requirement_resolver.py → test_bootstrap_requirement_resolver.py The new name clarifies that this resolver is specifically for the bootstrap process, reducing naming confusion. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
Remove the resolve_source plugin entry point and its documentation: - Removed entry point from pyproject.toml - Removed default_resolve_source from hooks.rst This hook was replaced by the get_resolver_provider hook during the resolver refactoring. Resolution now uses the provider pattern directly instead of calling resolve_source as a plugin hook. Fixes ReadTheDocs CI warning about missing default_resolve_source. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
rd4398
force-pushed
the
resolver-return-multiple-values
branch
from
af9e37e to
40bed8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/fromager/resolver.py (1)
193-213:⚠️ Potential issue | 🟠 MajorNormalize
find_matches()output here.Every caller now assumes this helper returns a non-empty highest-first list and immediately reads
results[0]. A custom provider can still return a lazy or unsorted iterable, which means iteration-time failures escape the wrapper and the wrong version can be selected. Materialize inside thetry, reject empty results, and sort before converting to tuples.Suggested change
try: - # Bypass resolvelib's resolver to collect all matching candidates rather than - # just the single best one. This is safe because get_dependencies() returns [] - # (no transitive deps to resolve). If get_dependencies() is ever extended, - # this must be revisited to use resolvelib's full resolution. - candidates = provider.find_matches( - identifier=identifier, - requirements={identifier: [req]}, - incompatibilities={}, # Empty - safe only because no transitive deps - ) + # Bypass resolvelib's resolver to collect all matching candidates rather than + # just the single best one. This is safe because get_dependencies() returns [] + # (no transitive deps to resolve). If get_dependencies() is ever extended, + # this must be revisited to use resolvelib's full resolution. + candidates = list( + provider.find_matches( + identifier=identifier, + requirements={identifier: [req]}, + incompatibilities={}, # Empty - safe only because no transitive deps + ) + ) except resolvelib.resolvers.ResolverException as err: constraint = provider.constraints.get_constraint(req.name) provider_desc = provider.get_provider_description() original_msg = str(err) raise resolvelib.resolvers.ResolverException( f"Unable to resolve requirement specifier {req} with constraint {constraint} using {provider_desc}: {original_msg}" ) from err - # Convert candidates to list of (url, version) tuples - # Candidates are already sorted by version (highest first) + if not candidates: + raise resolvelib.resolvers.ResolverException( + f"found no match for {req} using {provider.get_provider_description()}" + ) + + candidates = sorted( + candidates, + key=attrgetter("version", "build_tag"), + reverse=True, + ) + return [(candidate.url, candidate.version) for candidate in candidates]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/resolver.py` around lines 193 - 213, The provider.find_matches call returns an iterable that must be materialized and validated inside the try block: call provider.find_matches(...) and immediately convert to a concrete list (e.g., candidates = list(...)), raise a ResolverException if the list is empty (include provider.constraints.get_constraint(req.name), provider.get_provider_description() and the original exception/message), then sort candidates by version descending (highest-first) before converting to the list of (candidate.url, candidate.version) tuples returned by this helper; keep the existing exception wrapping (resolvelib.resolvers.ResolverException from err) and ensure all materialization/sorting happens inside the try so iteration-time errors are caught here.src/fromager/bootstrap_requirement_resolver.py (1)
166-178:⚠️ Potential issue | 🟠 MajorBootstrap resolution should reuse the public resolver wrappers.
These branches reimplement provider lookup directly instead of delegating to the source/wheel resolver APIs. That skips whatever compatibility logic those wrappers carry, so bootstrap can ignore legacy single-result hooks even if the command path still honors them. Route this through the public
*_all()wrappers, or share the same fallback logic here.Also applies to: 192-204
🤖 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 166 - 178, The bootstrap branch currently calls overrides.find_and_invoke(...) and resolver.find_all_matching_from_provider(...) directly, bypassing the public resolver wrapper logic; change this to use the public "*_all" wrapper functions (the same APIs used elsewhere) or the shared fallback helper so compatibility/legacy single-result hooks are honored: replace the overrides.find_and_invoke(...) + resolver.find_all_matching_from_provider(provider, req) sequence with the corresponding public resolver_all(...) call (or the shared fallback call used by other paths) that accepts the same parameters (resolver.default_resolver_provider, ctx=self.ctx, req=req, include_sdists=False, include_wheels=True, sdist_server_url=url, req_type=req_type, ignore_platform=False), and make the identical adjustment in the other branch referenced (lines handling the same logic around 192-204).src/fromager/sources.py (1)
149-167:⚠️ Potential issue | 🟠 MajorLegacy
resolve_sourceoverrides are bypassed here.This path only looks for
get_resolver_provider, so existing package overrides that still implement the single-resultresolve_sourcehook are never called.get_source_type()still treatsresolve_sourceas a valid override, so the command path and metadata can now disagree. Keepresolve_source()behind a compatibility layer (resolve_source_all()plus legacy-hook fallback) instead of requiring all plugins to migrate at once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/sources.py` around lines 149 - 167, This code only calls the "get_resolver_provider" hook via overrides.find_and_invoke and then resolver.find_all_matching_from_provider, which bypasses legacy single-result plugins that implement "resolve_source"; change the logic to fall back to the legacy hook: after attempting overrides.find_and_invoke(..., "get_resolver_provider", ...) and calling resolver.find_all_matching_from_provider(provider, req), if no provider was returned or results is empty, call overrides.find_and_invoke(req.name, "resolve_source", None, ctx=ctx, req=req, req_type=req_type) (or a new compatibility wrapper like "resolve_source_all" that normalizes output) and convert that single-result response into the (url, version) tuple expected by the rest of the code before returning; use the same ctx/req/req_type args and preserve the final return as str(url), version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/bootstrap_requirement_resolver.py`:
- Around line 181-185: The current bare "except Exception: continue" in the loop
that queries wheel_server_urls (around the block that raises ValueError(f"Could
not find a prebuilt wheel for {req} on {' or '.join(wheel_server_urls)}")) hides
real failures; change it to only swallow the specific "no matching wheel"
sentinel/error returned by the provider (e.g., a custom NoWheelFound/NotFound
exception or a well-defined response check for 404/no-match) and re-raise all
other exceptions (auth errors, 5xx, runtime plugin errors) so they surface
immediately; locate the try/except around req and wheel_server_urls and replace
the broad catch with targeted exception handling or explicit response checks to
continue only on expected miss/no-match conditions.
In `@tests/test_resolver.py`:
- Around line 1254-1256: The test currently bypasses resolver.resolve by calling
find_all_matching_from_provider(provider, Requirement("test-package==1.0.0"));
change the test to call resolver.resolve(...) with the same requirement so
resolver's override lookup and error paths are exercised, and instead of
constructing the provider directly patch/mock the override selection (the
function or method used by resolver to pick overrides) to return the intended
provider; keep references to resolver.resolve, find_all_matching_from_provider,
Requirement("test-package==1.0.0") and the provider object so you locate and
replace the direct call with a resolve invocation and a patch of the override
selection behavior.
---
Duplicate comments:
In `@src/fromager/bootstrap_requirement_resolver.py`:
- Around line 166-178: The bootstrap branch currently calls
overrides.find_and_invoke(...) and resolver.find_all_matching_from_provider(...)
directly, bypassing the public resolver wrapper logic; change this to use the
public "*_all" wrapper functions (the same APIs used elsewhere) or the shared
fallback helper so compatibility/legacy single-result hooks are honored: replace
the overrides.find_and_invoke(...) +
resolver.find_all_matching_from_provider(provider, req) sequence with the
corresponding public resolver_all(...) call (or the shared fallback call used by
other paths) that accepts the same parameters
(resolver.default_resolver_provider, ctx=self.ctx, req=req,
include_sdists=False, include_wheels=True, sdist_server_url=url,
req_type=req_type, ignore_platform=False), and make the identical adjustment in
the other branch referenced (lines handling the same logic around 192-204).
In `@src/fromager/resolver.py`:
- Around line 193-213: The provider.find_matches call returns an iterable that
must be materialized and validated inside the try block: call
provider.find_matches(...) and immediately convert to a concrete list (e.g.,
candidates = list(...)), raise a ResolverException if the list is empty (include
provider.constraints.get_constraint(req.name),
provider.get_provider_description() and the original exception/message), then
sort candidates by version descending (highest-first) before converting to the
list of (candidate.url, candidate.version) tuples returned by this helper; keep
the existing exception wrapping (resolvelib.resolvers.ResolverException from
err) and ensure all materialization/sorting happens inside the try so
iteration-time errors are caught here.
In `@src/fromager/sources.py`:
- Around line 149-167: This code only calls the "get_resolver_provider" hook via
overrides.find_and_invoke and then resolver.find_all_matching_from_provider,
which bypasses legacy single-result plugins that implement "resolve_source";
change the logic to fall back to the legacy hook: after attempting
overrides.find_and_invoke(..., "get_resolver_provider", ...) and calling
resolver.find_all_matching_from_provider(provider, req), if no provider was
returned or results is empty, call overrides.find_and_invoke(req.name,
"resolve_source", None, ctx=ctx, req=req, req_type=req_type) (or a new
compatibility wrapper like "resolve_source_all" that normalizes output) and
convert that single-result response into the (url, version) tuple expected by
the rest of the code before returning; use the same ctx/req/req_type args and
preserve the final return as str(url), version.
🪄 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: 82df2c50-841f-41cd-bb10-ffc5b21d5405
📒 Files selected for processing (10)
docs/reference/hooks.rstpyproject.tomlsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/bootstrapper.pysrc/fromager/resolver.pysrc/fromager/sources.pysrc/fromager/wheels.pytests/test_bootstrap_requirement_resolver.pytests/test_resolver.pytests/test_sources.py
💤 Files with no reviewable changes (2)
- docs/reference/hooks.rst
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fromager/wheels.py
| except Exception: | ||
| continue | ||
| # If we get here, no wheel server succeeded | ||
| raise ValueError( | ||
| f"Could not find a prebuilt wheel for {req} on {' or '.join(wheel_server_urls)}" |
There was a problem hiding this comment.
Do not treat provider failures as “try the next index”.
except Exception: continue hides auth failures, 5xx responses, and plugin bugs the same way it hides “no matching wheel”. If the primary wheel server is unhealthy, bootstrap can silently switch to the next server/PyPI and build from a different artifact. Continue only on expected miss/no-match exceptions and surface everything else.
🤖 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 181 - 185, The
current bare "except Exception: continue" in the loop that queries
wheel_server_urls (around the block that raises ValueError(f"Could not find a
prebuilt wheel for {req} on {' or '.join(wheel_server_urls)}")) hides real
failures; change it to only swallow the specific "no matching wheel"
sentinel/error returned by the provider (e.g., a custom NoWheelFound/NotFound
exception or a well-defined response check for 404/no-match) and re-raise all
other exceptions (auth errors, 5xx, runtime plugin errors) so they surface
immediately; locate the try/except around req and wheel_server_urls and replace
the broad catch with targeted exception handling or explicit response checks to
continue only on expected miss/no-match conditions.
| resolver.find_all_matching_from_provider( | ||
| provider, Requirement("test-package==1.0.0") | ||
| ) |
There was a problem hiding this comment.
This no longer tests resolver.resolve().
Despite the test name/docstring, the change now constructs a provider and calls find_all_matching_from_provider() directly. That won't catch regressions in override lookup or error handling inside resolver.resolve(). Call resolver.resolve(...) here and patch the override selection instead.
As per coding guidelines, tests/**: Verify test actually tests the intended behavior. Check for missing edge cases. Flag overly brittle mocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_resolver.py` around lines 1254 - 1256, The test currently bypasses
resolver.resolve by calling find_all_matching_from_provider(provider,
Requirement("test-package==1.0.0")); change the test to call
resolver.resolve(...) with the same requirement so resolver's override lookup
and error paths are exercised, and instead of constructing the provider directly
patch/mock the override selection (the function or method used by resolver to
pick overrides) to return the intended provider; keep references to
resolver.resolve, find_all_matching_from_provider,
Requirement("test-package==1.0.0") and the provider object so you locate and
replace the direct call with a resolve invocation and a patch of the override
selection behavior.
| results = resolver.find_all_matching_from_provider(provider, req) | ||
| if results: | ||
| return results | ||
| except Exception: |
There was a problem hiding this comment.
We should log the failures as we go using debug level messages so we can see what's happening if we have to debug a failed job.
| # Get all matching candidates directly from provider | ||
| # instead of using resolvelib's resolver which picks just one | ||
| identifier = provider.identify(req) | ||
| try: | ||
| result = rslvr.resolve([req]) | ||
| candidates = provider.find_matches( | ||
| identifier=identifier, | ||
| requirements={identifier: [req]}, | ||
| incompatibilities={}, | ||
| ) | ||
| except resolvelib.resolvers.ResolverException as err: | ||
| constraint = provider.constraints.get_constraint(req.name) | ||
| provider_desc = provider.get_provider_description() | ||
| # Include the original error message to preserve detailed information | ||
| # (e.g., file types, pre-release info from PyPIProvider) | ||
| original_msg = str(err) | ||
| raise resolvelib.resolvers.ResolverException( | ||
| f"Unable to resolve requirement specifier {req} with constraint {constraint} using {provider_desc}: {original_msg}" | ||
| ) from err | ||
| # resolvelib actually just returns one candidate per requirement. | ||
| # result.mapping is map from an identifier to its resolved candidate | ||
| candidate: Candidate | ||
| for candidate in result.mapping.values(): | ||
| return candidate.url, candidate.version | ||
| raise ValueError(f"Unable to resolve {req}") | ||
|
|
||
| # Convert candidates to list of (url, version) tuples | ||
| # Candidates are already sorted by version (highest first) | ||
| return [(candidate.url, candidate.version) for candidate in candidates] |
There was a problem hiding this comment.
Do we do anything in our providers to enforce the ordering?
4 participants
This commit introduces new *_all() functions that return all matching versions while
keeping existing functions unchanged for backward compatibility.
New functions return list[tuple[str, Version]] sorted by version (highest first):
Existing functions now call the new *_all() functions and return the
highest version (first element), maintaining identical behavior:
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Signed-off-by: Rohan Devasthale rdevasth@redhat.com