-
Notifications
You must be signed in to change notification settings - Fork 49
refactor(resolver): move age filter fallback to resolver with cache server lookup #1163
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
| from packaging.requirements import Requirement | ||
| from packaging.version import Version | ||
|
|
||
| from . import resolver, sources, wheels | ||
| from . import finders, resolver, sources, wheels | ||
| from .dependency_graph import DependencyGraph | ||
| from .requirements_file import RequirementType | ||
|
|
||
|
|
@@ -40,15 +40,24 @@ def __init__( | |
| self, | ||
| ctx: context.WorkContext, | ||
| prev_graph: DependencyGraph | None = None, | ||
| multiple_versions: bool = False, | ||
| cache_wheel_server_url: str = "", | ||
| ) -> None: | ||
| """Initialize requirement resolver. | ||
|
|
||
| Args: | ||
| ctx: Work context with constraints and settings | ||
| prev_graph: Optional previous dependency graph for caching | ||
| multiple_versions: If ``True``, age filtering returns an empty | ||
| list instead of falling back to all candidates, letting the | ||
| caller decide how to handle the case. | ||
| cache_wheel_server_url: URL of the remote wheel cache server. | ||
| Used as a fallback when age filtering produces no candidates. | ||
| """ | ||
| self.ctx = ctx | ||
| self.prev_graph = prev_graph | ||
| self.multiple_versions = multiple_versions | ||
| self.cache_wheel_server_url = cache_wheel_server_url | ||
| # Session-level resolution cache to avoid re-resolving same requirements | ||
| # Key: (requirement_string, pre_built) to distinguish source vs prebuilt | ||
| # Value: tuple of (url, version) tuples sorted by version (highest first) | ||
|
|
@@ -72,6 +81,8 @@ def resolve( | |
| 1. Session cache (if previously resolved) | ||
| 2. Previous dependency graph | ||
| 3. PyPI resolution (source or prebuilt based on package build info) | ||
| 4. Remote wheel cache server (multi-version mode only, when age | ||
| filtering produced no candidates) | ||
|
|
||
| Args: | ||
| req: Package requirement | ||
|
|
@@ -113,8 +124,12 @@ def resolve( | |
| # Resolve using strategies | ||
| results = self._resolve(req, req_type, parent_req, pre_built) | ||
|
|
||
| # Cache the result | ||
| self.cache_resolution(req, pre_built, results) | ||
| # Only cache non-empty results. | ||
| if results: | ||
| self.cache_resolution(req, pre_built, results) | ||
|
|
||
| if not results: | ||
| return [] | ||
| return results if return_all_versions else [results[0]] | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| def _resolve( | ||
|
|
@@ -129,6 +144,8 @@ def _resolve( | |
| Tries resolution strategies in order: | ||
| 1. Previous dependency graph | ||
| 2. PyPI resolution (source or prebuilt) | ||
| 3. Remote wheel cache server (multi-version mode only, when age | ||
| filtering produced no source candidates) | ||
|
|
||
| Args: | ||
| req: Package requirement | ||
|
|
@@ -167,18 +184,70 @@ def _resolve( | |
| wheel_server_urls=wheel_server_urls, | ||
| req_type=req_type, | ||
| ) | ||
| else: | ||
| # Resolve source (sdist) | ||
| provider = sources.get_source_provider( | ||
| ctx=self.ctx, | ||
| req=req, | ||
| sdist_server_url=resolver.PYPI_SERVER_URL, | ||
| req_type=req_type, | ||
|
|
||
| # Resolve source (sdist) | ||
| provider = sources.get_source_provider( | ||
| ctx=self.ctx, | ||
| req=req, | ||
| sdist_server_url=resolver.PYPI_SERVER_URL, | ||
| req_type=req_type, | ||
| ) | ||
| max_age_cutoff = resolver._compute_max_age_cutoff(self.ctx) | ||
| results = resolver.find_all_matching_from_provider( | ||
| provider, | ||
| req, | ||
| max_age_cutoff=max_age_cutoff, | ||
| fallback_on_empty_age_filter=not self.multiple_versions, | ||
| ) | ||
|
|
||
| if not results and self.multiple_versions and self.cache_wheel_server_url: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section deserves a comment explaining why we look at the cache server.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might also consider if it makes sense to reverse the logic and break up each reason for returning to make the logic clearer. |
||
| results = self._resolve_from_cache_server(req) | ||
|
|
||
| if not results: | ||
| logger.warning( | ||
| "%s: could not find any versions (all filtered by " | ||
| "max-release-age and no cached wheel on server)", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would help someone debugging this error if the log message included the max-release-age value. |
||
| req.name, | ||
| ) | ||
|
|
||
| return results | ||
|
|
||
| def _resolve_from_cache_server(self, req: Requirement) -> list[tuple[str, Version]]: | ||
| """Fall back to the remote wheel cache server for a cached version. | ||
|
|
||
| When age filtering removes all candidates in multi-version mode, | ||
| queries the remote cache server for the newest available wheel. | ||
| Returns at most one version so that transitive dependencies are | ||
| re-processed without rebuilding every old version. | ||
| """ | ||
| logger.info( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This log message assumes an execution order of the methods that might change in the future. We should log the detail about the max-release-age filtering before calling this function. See my suggested changes in the earlier comment. |
||
| "%s: all versions filtered by max-release-age, " | ||
| "checking cache server %s for existing wheel", | ||
| req.name, | ||
| self.cache_wheel_server_url, | ||
| ) | ||
| try: | ||
| provider = finders.PyPICacheProvider( | ||
| cache_server_url=self.cache_wheel_server_url, | ||
| constraints=self.ctx.constraints, | ||
| ) | ||
| max_age_cutoff = resolver._compute_max_age_cutoff(self.ctx) | ||
| return resolver.find_all_matching_from_provider( | ||
| provider, req, max_age_cutoff=max_age_cutoff | ||
| results = resolver.find_all_matching_from_provider(provider, req) | ||
| if results: | ||
| url, version = results[0] | ||
| logger.info( | ||
| "%s: found version %s on cache server", | ||
| req.name, | ||
| version, | ||
| ) | ||
| return [(url, version)] | ||
| except Exception as err: | ||
| logger.warning( | ||
| "%s: error checking cache server %s: %s", | ||
| req.name, | ||
| self.cache_wheel_server_url, | ||
| err, | ||
| ) | ||
| return [] | ||
|
|
||
| def get_cached_resolution( | ||
| self, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is
_resolve()its own method?