Skip to content

Conversation

@tiran
Copy link
Collaborator

@tiran tiran commented Sep 26, 2025

Redesign the resolver cache to address multiple issues.

  1. The cache now holds all candidates from the provider. Before it only stored candidates that also fulfilled the requirement and constraints.
  2. The cache now takes sdist url, GitHub repo, and GitLab project into account. Before a cache for local PyPI and pypi.org were mixed.
  3. GenericProvider no longer caches candidates. There is no way to construct a good cache key. Bootstrapper is using GenericProvider in way that does not benefit from caching either.
  4. There is just one global cache object for all providers. The new design makes it easier to clear all caches or just the cache for a single identifier.
  5. The custom logic for each provider class is now in find_candidates function. The method just has to return an iterable of candidates. The caching logic is handled by the rest of the code.
  6. Consumers can now opt out of caching with use_resolver_cache=False argument.
  7. All resolver classes now require keyword arguments. The base provider no longer takes PyPI-only arguments like include_sdists.

Fixes: #766

@tiran tiran requested a review from a team as a code owner September 26, 2025 09:10
@tiran tiran requested a review from dhellmann September 26, 2025 09:11
@mergify mergify bot added the ci label Sep 26, 2025
@tiran
Copy link
Collaborator Author

tiran commented Sep 26, 2025

test_provider_cache is expected to fail for now.

@tiran tiran force-pushed the resolver-cache branch 2 times, most recently from 06cd5ce to 2bc01ea Compare September 28, 2025 08:20
@dhellmann
Copy link
Member

It looks like ruff is not happy with the formatting of a file

Would reformat: tests/test_resolver.py

@tiran tiran force-pushed the resolver-cache branch 2 times, most recently from 68edefd to 9e71b5c Compare October 28, 2025 08:36
@tiran tiran added enhancement New feature or request awaiting review labels Oct 28, 2025
@dhellmann
Copy link
Member

I'm happy with this version but I would like another pair of eyes to look at it.

@LalatenduMohanty
Copy link
Member

Before (this code):

Cache: {} (empty)
Resolve: numpy<2  → Query PyPI → Cache: {numpy: [1.24, 1.25, 1.26.4]} ✓
Resolve: numpy    → Use cache  → Returns: [1.24, 1.25, 1.26.4] ✗ (missing 2.x!)

After:

Cache: {} (empty)
Resolve: numpy<2  → Query PyPI → Cache: {numpy: [1.24, ..., 1.26.4, 2.0, 2.1, 2.2]} ✓
                  → Filter for <2 → Returns: [1.24, 1.25, 1.26.4] ✓
Resolve: numpy    → Use cache  → Cache: [1.24, ..., 2.2] (all versions)
                  → Filter (no constraint) → Returns: [2.2] ✓ (latest!)

Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

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

We should add a test demonstrating the numpy scenario from #766
With the code change we need to udpate, docs/hooks.rst for VersionSource Signature

@LalatenduMohanty
Copy link
Member

LalatenduMohanty commented Nov 4, 2025

There might be more changes we need to do in the docs/hooks.rst. Cursor identified these cases.

[ ] Fix _version_source() to take only identifier parameter
[ ] Fix return type to Iterable[tuple[str, Version]]
[ ] Update example to return tuples, not just versions
[ ] Add note about keyword-only arguments
[ ] Test the example code actually works

Copy link
Contributor

@rd4398 rd4398 left a comment

Choose a reason for hiding this comment

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

This looks good. I see @LalatenduMohanty has some comments so I will wait for him to approve

Redesign the resolver cache to address multiple issues.

1. The cache now holds all candidates from the provider. Before it only
   stored candidates that also fulfilled the requirement and
   constraints.
2. The cache now takes sdist url, GitHub repo, and GitLab project into
   account. Before a cache for local PyPI and pypi.org were mixed.
3. GenericProvider no longer caches candidates. There is no way to
   construct a good cache key. Bootstrapper is using GenericProvider in
   way that does not benefit from caching either.
4. There is just one global cache object for all providers. The new
   design makes it easier to clear all caches or just the cache for a
   single identifier.
5. The custom logic for each provider class is now in `find_candidates`
   function. The method just has to return an iterable of candidates.
   The caching logic is handled by the rest of the code.
6. Consumers can now opt out of caching with `use_resolver_cache=False`
   argument.
7. All resolver classes now require keyword arguments. The base provider
   no longer takes PyPI-only arguments like `include_sdists`.

Fixes: python-wheel-build#766
Signed-off-by: Christian Heimes <[email protected]>
@tiran
Copy link
Collaborator Author

tiran commented Nov 5, 2025

@LalatenduMohanty I have updated and fixed the example in the docs

@LalatenduMohanty
Copy link
Member

We should add a test demonstrating the numpy scenario from #766

@tiran I could not find the test covering the numpy scenario. May be you did not add because we do not need it?

@tiran
Copy link
Collaborator Author

tiran commented Nov 5, 2025

@LalatenduMohanty I don't know how to reproduce that problem. Maybe you can come up with a test?

@LalatenduMohanty
Copy link
Member

I don't know how to reproduce that problem. Maybe you can come up with a test?

Sounds good. I have a test locally I can send in a separate PR. So lets go ahead and merge this PR.

@LalatenduMohanty
Copy link
Member

Created a issue to track this #838

@mergify mergify bot merged commit 60e8f11 into python-wheel-build:main Nov 6, 2025
112 checks passed
LalatenduMohanty added a commit to LalatenduMohanty/fromager that referenced this pull request Nov 6, 2025
The test tests the fix we did as part of python-wheel-build#793

Signed-off-by: Lalatendu Mohanty <[email protected]>
LalatenduMohanty added a commit to LalatenduMohanty/fromager that referenced this pull request Nov 6, 2025
The test tests the fix we did as part of python-wheel-build#793

Fixes python-wheel-build#838

Signed-off-by: Lalatendu Mohanty <[email protected]>
LalatenduMohanty added a commit to LalatenduMohanty/fromager that referenced this pull request Nov 6, 2025
This tests the fix we did as part of python-wheel-build#793

Fixes python-wheel-build#838

Signed-off-by: Lalatendu Mohanty <[email protected]>
LalatenduMohanty added a commit to LalatenduMohanty/fromager that referenced this pull request Nov 6, 2025
This tests the fix we did as part of python-wheel-build#793

Fixes python-wheel-build#838

Signed-off-by: Lalatendu Mohanty <[email protected]>
LalatenduMohanty added a commit to LalatenduMohanty/fromager that referenced this pull request Nov 13, 2025
LalatenduMohanty added a commit to LalatenduMohanty/fromager that referenced this pull request Nov 13, 2025
LalatenduMohanty added a commit to LalatenduMohanty/fromager that referenced this pull request Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

resolver caching is overly aggressive

4 participants