Skip to content

(refactor): RequirementsResolver should have only one method resolve() #949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rd4398 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

(refactor): RequirementsResolver should have only one method resolve() #949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 9 additions & 15 deletions src/fromager/bootstrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Copy link
Member

@dhellmann dhellmann Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this step to get the pbi could move inside the resolver, too.


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?
Expand Down Expand Up @@ -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,
)

Expand Down
96 changes: 27 additions & 69 deletions src/fromager/requirement_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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}")
Copy link
Member

@dhellmann dhellmann Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic of this function would be clearer if you returned early in any cases where you have enough information. So for example here you could just return. Then the else on the next line is not needed and the rest of the code can be moved left 1 indentation level.

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
)
Copy link
Member

@dhellmann dhellmann Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, if we get here and have a result returning means you don't need the else on the next line.

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)
Copy link
Member

@dhellmann dhellmann Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making those other structural changes could mean modifying the cache in multiple places. To void that, you could have resolve() look in the cache, call another method if it doesn't find something, then save the results of the other method to the cache before returning. Then all of the more complex logic for dealing with the resolution is in that _resolve() (for example) method.

return wheel_url, resolved_version
return url, resolved_version

def get_cached_resolution(
self,
Expand Down
7 changes: 4 additions & 3 deletions tests/test_requirement_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Loading