Skip to content

fix(resolver): cache known versions by package name#1188

Open
rd4398 wants to merge 1 commit into
python-wheel-build:mainfrom
rd4398:fix-cache-bug
Open

fix(resolver): cache known versions by package name#1188
rd4398 wants to merge 1 commit into
python-wheel-build:mainfrom
rd4398:fix-cache-bug

Conversation

@rd4398

@rd4398 rd4398 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Replace the per-requirement-string resolution cache with a
package-level known-versions cache, as discussed in issue #1187.

The resolver now:

  1. Tracks which requirement rules have been resolved to avoid
    redundant network calls.
  2. Accumulates all discovered versions into a package-level cache
    keyed by (normalized_name, pre_built).
  3. Filters the accumulated versions by the current specifier,
    returning the widest available set.

This ensures that versions discovered in one context (e.g., top-level
with cooldown bypass finding v2.0) are visible to later lookups with
different specifiers (e.g., transitive A>=1.0 will see v2.0 even
though its own network call only found v1.5).

Co-Authored-By: Claude claude@anthropic.com
Closes: #1187
Signed-off-by: Rohan Devasthale rdevasth@redhat.com

@rd4398 rd4398 requested a review from a team as a code owner June 4, 2026 18:43
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

BootstrapRequirementResolver now includes RequirementType in its session-level cache key. The internal _resolved_requirements dict keys changed from (requirement_string, pre_built) to (requirement_string, pre_built, req_type). Public helper methods get_cached_resolution() and cache_resolution() accept the new req_type parameter. The resolve() method and git+ URL path in Bootstrapper.resolve_versions() pass req_type to these cache methods. Tests verify cache separation between transitive (INSTALL) and top-level (TOP_LEVEL) resolutions and confirm tuple immutability.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title mentions 'cache known versions by package name', but the actual change is about including req_type in the resolution cache key to separate transitive vs top-level requirement caching. The title does not accurately reflect the core fix. Update the title to 'fix(resolver): include req_type in resolution cache key' to accurately describe the primary change of separating cache entries by requirement type.
Description check ❓ Inconclusive The PR description discusses package-level known-versions cache and version filtering, but the changeset summary shows only req_type being added to the cache key without implementing the described version accumulation mechanism. Clarify whether the description matches the actual implementation: is this PR only adding req_type to the cache key (as summaries indicate), or does it also implement the package-level known-versions cache mentioned in the description?
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR implementation fully addresses issue #1187 by adding req_type to the cache key in BootstrapRequirementResolver, ensuring transitive and top-level resolutions are cached separately.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the cache key bug: cache key modifications, method signature updates, and related test coverage for cache separation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mergify mergify Bot added the ci label Jun 4, 2026
Comment thread src/fromager/bootstrap_requirement_resolver.py Outdated
dhellmann
dhellmann previously approved these changes Jun 4, 2026
@LalatenduMohanty

Copy link
Copy Markdown
Member

@dhellmann This alone might not fix the issue A>=1.0 blocking A==2.0.0 top-level. We need #1157 as well. But we should merge this first then merge #1157

LalatenduMohanty added a commit to LalatenduMohanty/fromager that referenced this pull request Jun 4, 2026
…ython-wheel-build#1187 exempt_versions

Cover top-level == pin scenarios where transitive deps use a different
requirement specifier. These validate the python-wheel-build#1157 exempt_versions fix;
session-cache coverage remains in python-wheel-build#1188.

Co-Authored-By: Claude <claude@anthropic.com>
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@tiran tiran left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IMO the suggestion from 1187 is not the best solution. Breadth-first resolution of top-level requirements should solve the issue, too.

@rd4398

rd4398 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

IMO the suggestion from 1187 is not the best solution. Breadth-first resolution of top-level requirements should solve the issue, too.

@tiran I agree that 1187 is not the best solution however, I think the BFS approach will make things more complicated. I see the following challenges:

  1. Build dependency ordering constraint
DFS exists for a reason: to build packages in dependency order. Before you can
build package B, you need B's build dependencies (setuptools, wheel, etc.) to
be available as wheels. The DFS loop ensures that when `_phase_prepare_build`
installs build deps into the build environment, those wheels have already been
built.

A pure BFS approach would try to build all top-level packages simultaneously,
but their build dependencies might not be ready yet. This is the fundamental
constraint.

Impact: A naive BFS would require a two-pass approach:
- Pass 1: Resolve all top-level versions (already done)
- Pass 2: Build in topological order based on dependency graph

This is essentially what `bootstrap_parallel` already does (sdist-only serial
pass, then parallel wheel build).
  1. Scope of changes
Changing from DFS to BFS in the iterative bootstrap loop would be a
significant refactor:

- The `WorkItem` phase machine and LIFO stack are designed for DFS
- Changing to a FIFO queue (BFS) would break build ordering guarantees
- The `_track_why` dependency chain tracking assumes DFS order
- The `_seen_requirements` deduplication assumes DFS order
- Progress tracking and logging assume DFS order

I wonder this will complicate the already existing bootstrap logic. A more targeted approach would be to add a resolution-only BFS pass before the DFS build pass, but this is essentially what pre-resolution already does.
  1. BFS doesn't fully solve the problem
Even with BFS for top-level resolution, transitive dependencies of B can
still conflict with transitive dependencies of C. If B depends on `X>=1.0`
and C depends on `X>=2.0`, whichever is resolved first sets the cache. This
is a broader issue than just top-level vs transitive.
  1. Multiple version mode
In `--multiple-versions` mode, the resolver returns ALL matching versions, not
just the highest. BFS resolution would need to handle this correctly — all
versions of all top-level deps resolved before any transitive deps.

This is already handled by pre-resolution (`resolve_and_add_top_level` with
`return_all_versions=self.multiple_versions`)

Some points to consider:

  1. Is building two versions actually wrong? -- In single-version mode, both
    versions will be built, but downstream packages will use whichever version
    the dependency graph and constraints file specify. If A==2.0.0 satisfies
    A>=1.0, the constraints file should pick 2.0.0

  2. The pre-resolution gap -- re-resolution caches ("A==2.0.0", False).
    The transitive dep is ("A>=1.0", False) — a different cache key. These
    don't collide. The bug requires identical requirement strings. How often we will hit this scenario?

  3. To avoid redundant builds, I think the fix could be at the
    _phase_start level: when a transitive resolution produces version X,
    check if a top-level resolution for the same package already produced
    version Y > X that satisfies the transitive specifier, and use Y instead.
    This is version reconciliation, not resolution order change.

  4. If we all still want to do a BFS approach, the lowest refactor and less risky solution will be: resolve all top-level deps in pre-resolution (already done), then in the DFS build
    loop, have _phase_resolve check if the package was already resolved as top-level in pre-resolution and reuse that version when compatible. This avoids changing DFS→BFS entirely

Let me know what you think / whether I am missing anything. cc @LalatenduMohanty @dhellmann

@rd4398

rd4398 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

IMO the suggestion from 1187 is not the best solution. Breadth-first resolution of top-level requirements should solve the issue, too.

@tiran I agree that 1187 is not the best solution however, I think the BFS approach will make things more complicated. I see the following challenges:

  1. Build dependency ordering constraint
DFS exists for a reason: to build packages in dependency order. Before you can
build package B, you need B's build dependencies (setuptools, wheel, etc.) to
be available as wheels. The DFS loop ensures that when `_phase_prepare_build`
installs build deps into the build environment, those wheels have already been
built.

A pure BFS approach would try to build all top-level packages simultaneously,
but their build dependencies might not be ready yet. This is the fundamental
constraint.

Impact: A naive BFS would require a two-pass approach:
- Pass 1: Resolve all top-level versions (already done)
- Pass 2: Build in topological order based on dependency graph

This is essentially what `bootstrap_parallel` already does (sdist-only serial
pass, then parallel wheel build).
  1. Scope of changes
Changing from DFS to BFS in the iterative bootstrap loop would be a
significant refactor:

- The `WorkItem` phase machine and LIFO stack are designed for DFS
- Changing to a FIFO queue (BFS) would break build ordering guarantees
- The `_track_why` dependency chain tracking assumes DFS order
- The `_seen_requirements` deduplication assumes DFS order
- Progress tracking and logging assume DFS order

I wonder this will complicate the already existing bootstrap logic. A more targeted approach would be to add a resolution-only BFS pass before the DFS build pass, but this is essentially what pre-resolution already does.
  1. BFS doesn't fully solve the problem
Even with BFS for top-level resolution, transitive dependencies of B can
still conflict with transitive dependencies of C. If B depends on `X>=1.0`
and C depends on `X>=2.0`, whichever is resolved first sets the cache. This
is a broader issue than just top-level vs transitive.
  1. Multiple version mode
In `--multiple-versions` mode, the resolver returns ALL matching versions, not
just the highest. BFS resolution would need to handle this correctly — all
versions of all top-level deps resolved before any transitive deps.

This is already handled by pre-resolution (`resolve_and_add_top_level` with
`return_all_versions=self.multiple_versions`)

Some points to consider:

  1. Is building two versions actually wrong? -- In single-version mode, both
    versions will be built, but downstream packages will use whichever version
    the dependency graph and constraints file specify. If A==2.0.0 satisfies
    A>=1.0, the constraints file should pick 2.0.0
  2. The pre-resolution gap -- re-resolution caches ("A==2.0.0", False).
    The transitive dep is ("A>=1.0", False) — a different cache key. These
    don't collide. The bug requires identical requirement strings. How often we will hit this scenario?
  3. To avoid redundant builds, I think the fix could be at the
    _phase_start level: when a transitive resolution produces version X,
    check if a top-level resolution for the same package already produced
    version Y > X that satisfies the transitive specifier, and use Y instead.
    This is version reconciliation, not resolution order change.
  4. If we all still want to do a BFS approach, the lowest refactor and less risky solution will be: resolve all top-level deps in pre-resolution (already done), then in the DFS build
    loop, have _phase_resolve check if the package was already resolved as top-level in pre-resolution and reuse that version when compatible. This avoids changing DFS→BFS entirely

Let me know what you think / whether I am missing anything. cc @LalatenduMohanty @dhellmann

UPDATE: I posted this before reading Doug's comment here: #1187 (comment)
I am okay with implementing what he suggested and will make changes to this PR

Replace the per-requirement-string resolution cache with a
package-level known-versions cache, as discussed in issue python-wheel-build#1187.

The resolver now:
1. Tracks which requirement rules have been resolved to avoid
   redundant network calls.
2. Accumulates all discovered versions into a package-level cache
   keyed by (normalized_name, pre_built).
3. Filters the accumulated versions by the current specifier,
   returning the widest available set.

This ensures that versions discovered in one context (e.g., top-level
with cooldown bypass finding v2.0) are visible to later lookups with
different specifiers (e.g., transitive A>=1.0 will see v2.0 even
though its own network call only found v1.5).

Co-Authored-By: Claude <claude@anthropic.com>
Closes: python-wheel-build#1187
Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
@rd4398 rd4398 changed the title fix(resolver): include req_type in resolution cache key fix(resolver): cache known versions by package name Jun 8, 2026
@rd4398

rd4398 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@dhellmann @LalatenduMohanty @tiran I have implemented Doug's suggestion which was discussed here in my latest commit

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.

Release-age cooldown bypass ignored when package is first resolved as transitive dependency

4 participants