fix(resolver): cache known versions by package name

[rd4398]

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

[coderabbitai]

Warning

Review limit reached

@rd4398, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 52 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2c03fe7a-662f-4840-a46d-3583028f6b94

📥 Commits

Reviewing files that changed from the base of the PR and between 8cd195f and e5c7817.

📒 Files selected for processing (3)

• src/fromager/bootstrap_requirement_resolver.py
• src/fromager/bootstrapper.py
• tests/test_bootstrap_requirement_resolver.py

📝 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[LalatenduMohanty]

@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

[tiran]

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

[rd4398]

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).

2. 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.

3. 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.

4. 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]

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).

2. 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.

3. 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.

4. 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

[rd4398]

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

[dhellmann]

Since we're updating 2 data structures now, we need a lock to make sure it's thread safe.

[rd4398]

Good catch, I did not think about it. I have pushed this implementation in latest commit:

• resolve() holds self._lock around the check + extend + filter block
• _resolve_and_extend() and resolve() call the unlocked _extend_known_versions() / _get_matching_versions() (no nested
  locking)
• The public extend_known_versions() / get_matching_versions() acquire the lock themselves, for external callers in bootstrapper.py

Does this make sense?

[dhellmann]

This looks good to me but I'm reluctant to approve and merge it late on a Friday.

[dhellmann]

This looks safe. If we were worried about the resolver running in multiple threads then we would probably want to not hold the lock for so long, because that would make this part of the code a bottleneck. We would want to try to organize the order of operations so that the resolution happened outside of the locked region and only operations to save state happen within the locked region. I don't think it's worth reorganizing the code to make that easy right now, though, because we are not using multiple threads to resolve rules.