From fdd028a7f4c27f9da2dabed3fcce6fb83f18e829 Mon Sep 17 00:00:00 2001 From: Rohan Devasthale Date: Thu, 5 Mar 2026 15:49:19 -0500 Subject: [PATCH] (refactor): RequirementsResolver should have only one method resolve() This commits attempts to refactor the RequirementsResolver to have only one method resolve() instead of separate methods It is consistent with existing code in resolver.py where there is only single method for resolution Signed-off-by: Rohan Devasthale --- src/fromager/bootstrapper.py | 24 +++---- src/fromager/requirement_resolver.py | 96 ++++++++-------------------- tests/test_requirement_resolver.py | 7 +- 3 files changed, 40 insertions(+), 87 deletions(-) diff --git a/src/fromager/bootstrapper.py b/src/fromager/bootstrapper.py index 08845d06..63e4377c 100644 --- a/src/fromager/bootstrapper.py +++ b/src/fromager/bootstrapper.py @@ -183,7 +183,6 @@ def resolve_version( Delegates PyPI/graph resolution to RequirementResolver. """ if req.url: - # Git URLs must be resolved in Bootstrapper (orchestration concern) if req_type != RequirementType.TOP_LEVEL: raise ValueError( f"{req} includes a URL, but is not a top-level dependency" @@ -201,22 +200,16 @@ def resolve_version( self._resolver.cache_resolution(req, (source_url, resolved_version)) return source_url, resolved_version - # Delegate to RequirementResolver (handles caching internally) + # Delegate to RequirementResolver parent_req = self.why[-1][1] if self.why else None pbi = self.ctx.package_build_info(req) - if pbi.pre_built: - return self._resolver.resolve_prebuilt( - req=req, - req_type=req_type, - parent_req=parent_req, - ) - else: - return self._resolver.resolve_source( - req=req, - req_type=req_type, - parent_req=parent_req, - ) + return self._resolver.resolve( + req=req, + req_type=req_type, + pre_built=pbi.pre_built, + parent_req=parent_req, + ) def _processing_build_requirement(self, current_req_type: RequirementType) -> bool: """Are we currently processing a build requirement? @@ -918,9 +911,10 @@ def _handle_test_mode_failure( try: parent_req = self.why[-1][1] if self.why else None - wheel_url, fallback_version = self._resolver.resolve_prebuilt( + wheel_url, fallback_version = self._resolver.resolve( req=req, req_type=req_type, + pre_built=True, parent_req=parent_req, ) diff --git a/src/fromager/requirement_resolver.py b/src/fromager/requirement_resolver.py index aa84cebf..056db78e 100644 --- a/src/fromager/requirement_resolver.py +++ b/src/fromager/requirement_resolver.py @@ -52,26 +52,28 @@ def __init__( # Session-level resolution cache to avoid re-resolving same requirements self._resolved_requirements: dict[str, tuple[str, Version]] = {} - def resolve_source( + def resolve( self, req: Requirement, req_type: RequirementType, + pre_built: bool, parent_req: Requirement | None = None, ) -> tuple[str, Version]: - """Resolve source package (sdist). + """Resolve package requirement. Tries resolution strategies in order: 1. Session cache (if previously resolved) 2. Previous dependency graph - 3. PyPI source resolution + 3. PyPI resolution (source or prebuilt based on pre_built parameter) Args: req: Package requirement (must NOT have URL) req_type: Type of requirement + pre_built: If True, resolve prebuilt wheel; if False, resolve source (sdist) parent_req: Parent requirement from dependency chain Returns: - Tuple of (source_url, resolved_version) + Tuple of (url, resolved_version) Raises: ValueError: If req contains a URL (must use Bootstrapper for git URLs) @@ -91,80 +93,36 @@ def resolve_source( cached_resolution = self._resolve_from_graph( req=req, req_type=req_type, - pre_built=False, - parent_req=parent_req, - ) - if cached_resolution: - source_url, resolved_version = cached_resolution - logger.debug(f"resolved from previous bootstrap to {resolved_version}") - else: - # Fallback to PyPI - source_url, resolved_version = sources.resolve_source( - ctx=self.ctx, - req=req, - sdist_server_url=resolver.PYPI_SERVER_URL, - req_type=req_type, - ) - - # Cache the result - result = (source_url, resolved_version) - self.cache_resolution(req, result) - return source_url, resolved_version - - def resolve_prebuilt( - self, - req: Requirement, - req_type: RequirementType, - parent_req: Requirement | None = None, - ) -> tuple[str, Version]: - """Resolve pre-built package (wheels only). - - Tries resolution strategies in order: - 1. Session cache (if previously resolved) - 2. Previous dependency graph - 3. PyPI wheel resolution - - Args: - req: Package requirement - req_type: Type of requirement - parent_req: Parent requirement from dependency chain - - Returns: - Tuple of (source_url, resolved_version) - - Raises: - ValueError: If unable to resolve - """ - # Check session cache first - cached_result = self.get_cached_resolution(req) - if cached_result is not None: - logger.debug(f"resolved {req} from cache") - return cached_result - - # Try graph - cached_resolution = self._resolve_from_graph( - req=req, - req_type=req_type, - pre_built=True, + pre_built=pre_built, parent_req=parent_req, ) if cached_resolution and not req.url: - wheel_url, resolved_version = cached_resolution + url, resolved_version = cached_resolution logger.debug(f"resolved from previous bootstrap to {resolved_version}") else: - # Fallback to PyPI prebuilt resolution - servers = wheels.get_wheel_server_urls( - self.ctx, req, cache_wheel_server_url=resolver.PYPI_SERVER_URL - ) - wheel_url, resolved_version = wheels.resolve_prebuilt_wheel( - ctx=self.ctx, req=req, wheel_server_urls=servers, req_type=req_type - ) + # Fallback to PyPI + if pre_built: + # Resolve prebuilt wheel + servers = wheels.get_wheel_server_urls( + self.ctx, req, cache_wheel_server_url=resolver.PYPI_SERVER_URL + ) + url, resolved_version = wheels.resolve_prebuilt_wheel( + ctx=self.ctx, req=req, wheel_server_urls=servers, req_type=req_type + ) + else: + # Resolve source (sdist) + url, resolved_version = sources.resolve_source( + ctx=self.ctx, + req=req, + sdist_server_url=resolver.PYPI_SERVER_URL, + req_type=req_type, + ) # Cache the result - result = (wheel_url, resolved_version) + result = (url, resolved_version) self.cache_resolution(req, result) - return wheel_url, resolved_version + return url, resolved_version def get_cached_resolution( self, diff --git a/tests/test_requirement_resolver.py b/tests/test_requirement_resolver.py index 14f6f085..359009fa 100644 --- a/tests/test_requirement_resolver.py +++ b/tests/test_requirement_resolver.py @@ -248,15 +248,16 @@ def test_resolve_from_graph_no_previous_graph(tmp_context: WorkContext) -> None: ) -def test_resolve_source_rejects_git_urls(tmp_context: WorkContext) -> None: - """RequirementResolver.resolve_source() rejects git URLs.""" +def test_resolve_rejects_git_urls(tmp_context: WorkContext) -> None: + """RequirementResolver.resolve() rejects git URLs.""" resolver = RequirementResolver(tmp_context) with pytest.raises( ValueError, match="Git URL requirements must be handled by Bootstrapper" ): - resolver.resolve_source( + resolver.resolve( req=Requirement("package @ git+https://github.com/example/repo.git"), req_type=RequirementType.TOP_LEVEL, + pre_built=False, parent_req=None, )