feat(bootstrap): add --multiple-versions flag to bootstrap all matching versions#1035
Merged
mergify[bot] merged 1 commit intopython-wheel-build:mainfrom Apr 10, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces an opt-in multiple-versions bootstrap mode. CLI commands Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_bootstrap_requirement_resolver_multiple.py (1)
16-33: Fixture shadows conftesttmp_context.This file defines a local
tmp_contextfixture that returns aMagicMockinstead of the realWorkContextfrom conftest. This is intentional for unit testing the resolver in isolation, but consider renaming tomock_contextfor clarity and to avoid confusion with the integration-level fixture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bootstrap_requirement_resolver_multiple.py` around lines 16 - 33, The fixture tmp_context in this test shadows the integration-level conftest fixture and returns a MagicMock WorkContext; rename this fixture (e.g., mock_context) to avoid confusion and clarify intent, update the fixture definition name from tmp_context to mock_context and update all usages in this test file (including any test function parameters) to use mock_context, keeping the same MagicMock setup for WorkContext, ctx, ctx.package_build_info, and pbi return values.src/fromager/bootstrapper.py (1)
131-133: Consider clearing_failed_versionsbetween top-level requirements.The list accumulates failures across all top-level requirements. While the reporting at lines 313-324 filters by package name, consider if explicit clearing or a dict keyed by package would be cleaner for long bootstrap runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/bootstrapper.py` around lines 131 - 133, The _failed_versions list in the bootstrapper (self._failed_versions: list[tuple[str, str, Exception]]) currently accumulates failures across all top-level requirements; change the logic so failures are cleared per top-level package run (or replace the list with a dict keyed by package name to isolate failures) — specifically, reset/clear self._failed_versions at the start of processing each top-level requirement in the method that iterates top-level packages (or switch the storage to something like self._failed_versions_by_package and record failures under the current package key), and update any reporting code that reads _failed_versions (e.g., the code that filters by package name at report time) to use the new cleared list or per-package dict.src/fromager/dependency_graph.py (1)
339-370: Parent edges on child nodes are not cleaned up.When removing a node, the method clears incoming edges (from parents) but doesn't update the
parentslist on child nodes that referenced the removed node. If the removed node had children, those children will retain stale parent edge references.In the current PR context (removing failed bootstrap versions), this is likely safe because failures occur before child dependencies are processed. However, for general API correctness:
Optional: Also clean up parent edges on children
# Remove all edges pointing to this node from parent nodes - # Use list comprehension to filter, then clear and re-add for node in self.nodes.values(): filtered_children = [ edge for edge in node.children if edge.destination_node.key != key ] node.children.clear() node.children.extend(filtered_children) + + # Also clean up parent edges that reference the removed node + filtered_parents = [ + edge for edge in node.parents if edge.destination_node.key != key + ] + node.parents.clear() + node.parents.extend(filtered_parents)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/dependency_graph.py` around lines 339 - 370, The remove_dependency function deletes the node and removes parent->child edges from remaining nodes but fails to update the removed node's children's parents lists, leaving stale parent edges; fix by (before deleting self.nodes[key]) iterating over the removed node's children (the list on the node being removed), and for each child node filter/cleanup its parents list to remove any Edge whose source (or source_node) key equals the removed key (i.e., remove parent edges referencing the node being deleted), then proceed to delete the node and continue filtering other nodes' children as currently implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/test_bootstrap_multiple_versions.sh`:
- Around line 11-12: The trap currently expands $constraints_file when set
instead of at cleanup time; update the trap invocation that references the
constraints_file created by mktemp so it uses single quotes to defer expansion
(i.e., change the trap command that runs rm -f $constraints_file to use 'rm -f
$constraints_file') so the variable is evaluated when the trap runs rather than
when it is defined.
In `@src/fromager/bootstrapper.py`:
- Around line 292-308: TOP_LEVEL dependencies can be added twice when
multiple_versions is enabled because resolve_and_add_top_level() pre-adds the
highest version and bootstrap() re-adds all resolved versions; modify the
bootstrap() logic around the call to self.ctx.dependency_graph.add_dependency
(inside bootstrap() where req_type == RequirementType.TOP_LEVEL) to first check
for an existing parent->child edge (e.g., via an existing graph lookup method
like has_dependency / get_node / get_children or by checking DependencyNode
children for req.name + resolved_version) and only call add_dependency when that
exact dependency edge does not already exist; alternatively implement
deduplication inside DependencyNode.add_child() or add_dependency() so duplicate
parent edges are ignored.
In `@tests/test_bootstrapper.py`:
- Around line 298-302: Move the local import of canonicalize_name out of the
body of test_multiple_versions_continues_on_error and add a top-level import
statement "from packaging.utils import canonicalize_name" with the other imports
at the top of the file; then remove the inline "from packaging.utils import
canonicalize_name" line inside the test function so the test uses the top-level
canonicalize_name import.
---
Nitpick comments:
In `@src/fromager/bootstrapper.py`:
- Around line 131-133: The _failed_versions list in the bootstrapper
(self._failed_versions: list[tuple[str, str, Exception]]) currently accumulates
failures across all top-level requirements; change the logic so failures are
cleared per top-level package run (or replace the list with a dict keyed by
package name to isolate failures) — specifically, reset/clear
self._failed_versions at the start of processing each top-level requirement in
the method that iterates top-level packages (or switch the storage to something
like self._failed_versions_by_package and record failures under the current
package key), and update any reporting code that reads _failed_versions (e.g.,
the code that filters by package name at report time) to use the new cleared
list or per-package dict.
In `@src/fromager/dependency_graph.py`:
- Around line 339-370: The remove_dependency function deletes the node and
removes parent->child edges from remaining nodes but fails to update the removed
node's children's parents lists, leaving stale parent edges; fix by (before
deleting self.nodes[key]) iterating over the removed node's children (the list
on the node being removed), and for each child node filter/cleanup its parents
list to remove any Edge whose source (or source_node) key equals the removed key
(i.e., remove parent edges referencing the node being deleted), then proceed to
delete the node and continue filtering other nodes' children as currently
implemented.
In `@tests/test_bootstrap_requirement_resolver_multiple.py`:
- Around line 16-33: The fixture tmp_context in this test shadows the
integration-level conftest fixture and returns a MagicMock WorkContext; rename
this fixture (e.g., mock_context) to avoid confusion and clarify intent, update
the fixture definition name from tmp_context to mock_context and update all
usages in this test file (including any test function parameters) to use
mock_context, keeping the same MagicMock setup for WorkContext, ctx,
ctx.package_build_info, and pbi return values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b1fd183-63f7-431d-b992-1672b07d3d94
📒 Files selected for processing (7)
e2e/test_bootstrap_multiple_versions.shsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/bootstrapper.pysrc/fromager/commands/bootstrap.pysrc/fromager/dependency_graph.pytests/test_bootstrap_requirement_resolver_multiple.pytests/test_bootstrapper.py
rd4398
force-pushed
the
bootstrap-multiple-version-flag
branch
from
b94cf5f to
60c563e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
e2e/test_bootstrap_multiple_versions.sh (1)
11-12:⚠️ Potential issue | 🟡 MinorUse single quotes in trap to defer variable expansion.
Per SC2064:
$constraints_fileexpands when the trap is set, not when triggered. If the variable changes or cleanup runs in a different context, this could fail silently.constraints_file=$(mktemp) -trap "rm -f $constraints_file" EXIT +trap 'rm -f "$constraints_file"' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_multiple_versions.sh` around lines 11 - 12, The trap currently expands $constraints_file at assignment time (SC2064); change the trap invocation to use single quotes so the variable is expanded when the trap runs (e.g., replace trap "rm -f $constraints_file" EXIT with a single-quoted form to defer expansion), ensuring cleanup uses the final value of the constraints_file variable.src/fromager/bootstrapper.py (1)
292-308:⚠️ Potential issue | 🟡 MinorDuplicate graph edges for TOP_LEVEL when
multiple_versionsis enabled.When
multiple_versions=True,resolve_and_add_top_level()adds the highest version to the graph during pre-resolution. Thenbootstrap()re-resolves all versions and adds each again at lines 298-307. The highest version gets added twice sinceadd_dependencydoesn't deduplicate.Suggested fix: skip if already present
for source_url, resolved_version in resolved_versions: # For top-level requirements, add to graph before bootstrapping # so that build dependencies can reference the parent node if req_type == RequirementType.TOP_LEVEL: + # Skip if already added (e.g., from pre-resolution phase) + node_key = f"{canonicalize_name(req.name)}=={resolved_version}" + if node_key in self.ctx.dependency_graph.nodes: + logger.debug(f"skipping graph add for {node_key} - already present") + else: - pbi = self.ctx.package_build_info(req) - self.ctx.dependency_graph.add_dependency( + pbi = self.ctx.package_build_info(req) + self.ctx.dependency_graph.add_dependency( parent_name=None, parent_version=None, req_type=RequirementType.TOP_LEVEL, req=req, req_version=resolved_version, download_url=source_url, pre_built=pbi.pre_built, constraint=self.ctx.constraints.get_constraint(req.name), ) - self.ctx.write_to_graph_to_file() + self.ctx.write_to_graph_to_file()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/bootstrapper.py` around lines 292 - 308, The TOP_LEVEL dependency is being added again in bootstrap() for each resolved_version, causing duplicates when multiple_versions=True; before calling self.ctx.dependency_graph.add_dependency(...) in bootstrap() (the block that handles RequirementType.TOP_LEVEL), check whether that exact dependency (parent_name=None, parent_version=None, req=req, req_version=resolved_version) already exists in the graph (e.g., via a has_dependency / contains / find API on self.ctx.dependency_graph) and only call add_dependency and self.ctx.write_to_graph_to_file() when it is not present, so the highest version previously added by resolve_and_add_top_level() is skipped.
🧹 Nitpick comments (1)
src/fromager/bootstrapper.py (1)
131-133: Consider adding a docstring for_failed_versionsstructure.The tuple structure
(str, str, Exception)stores(canonicalized_name, version_str, exception). A brief inline comment clarifies intent but a type alias would improve maintainability.# Track failed versions in multiple_versions mode - # Maps (package_name, version) -> exception info - self._failed_versions: list[tuple[str, str, Exception]] = [] + # List of (canonicalized_name, version_string, exception) for failed bootstraps + self._failed_versions: list[tuple[NormalizedName, str, Exception]] = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/bootstrapper.py` around lines 131 - 133, Add a clear type alias and docstring for the _failed_versions attribute to document the tuple shape and intent: define a top-level type alias like FailedVersion = tuple[str, str, Exception] (or a small NamedTuple/TypedDict for stronger typing) and replace the inline annotation with list[FailedVersion], then add a one-line docstring/comment above self._failed_versions explaining that each entry is (canonicalized_name, version_str, exception) used to track failures in multiple_versions mode; update references to _failed_versions if necessary to use the new alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/test_bootstrap_multiple_versions.sh`:
- Around line 11-12: The trap currently expands $constraints_file at assignment
time (SC2064); change the trap invocation to use single quotes so the variable
is expanded when the trap runs (e.g., replace trap "rm -f $constraints_file"
EXIT with a single-quoted form to defer expansion), ensuring cleanup uses the
final value of the constraints_file variable.
In `@src/fromager/bootstrapper.py`:
- Around line 292-308: The TOP_LEVEL dependency is being added again in
bootstrap() for each resolved_version, causing duplicates when
multiple_versions=True; before calling
self.ctx.dependency_graph.add_dependency(...) in bootstrap() (the block that
handles RequirementType.TOP_LEVEL), check whether that exact dependency
(parent_name=None, parent_version=None, req=req, req_version=resolved_version)
already exists in the graph (e.g., via a has_dependency / contains / find API on
self.ctx.dependency_graph) and only call add_dependency and
self.ctx.write_to_graph_to_file() when it is not present, so the highest version
previously added by resolve_and_add_top_level() is skipped.
---
Nitpick comments:
In `@src/fromager/bootstrapper.py`:
- Around line 131-133: Add a clear type alias and docstring for the
_failed_versions attribute to document the tuple shape and intent: define a
top-level type alias like FailedVersion = tuple[str, str, Exception] (or a small
NamedTuple/TypedDict for stronger typing) and replace the inline annotation with
list[FailedVersion], then add a one-line docstring/comment above
self._failed_versions explaining that each entry is (canonicalized_name,
version_str, exception) used to track failures in multiple_versions mode; update
references to _failed_versions if necessary to use the new alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00bcfaf8-ece6-4f00-9f01-ad051e400027
📒 Files selected for processing (8)
e2e/ci_bootstrap_suite.she2e/test_bootstrap_multiple_versions.shsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/bootstrapper.pysrc/fromager/commands/bootstrap.pysrc/fromager/dependency_graph.pytests/test_bootstrap_requirement_resolver_multiple.pytests/test_bootstrapper.py
✅ Files skipped from review due to trivial changes (3)
- e2e/ci_bootstrap_suite.sh
- tests/test_bootstrapper.py
- tests/test_bootstrap_requirement_resolver_multiple.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/dependency_graph.py
- src/fromager/bootstrap_requirement_resolver.py
dhellmann
reviewed
Apr 7, 2026
rd4398
force-pushed
the
bootstrap-multiple-version-flag
branch
from
60c563e to
889df21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/fromager/commands/lint_requirements.py (1)
82-96:⚠️ Potential issue | 🟠 MajorStop after recording an invalid requirement.
The
exceptpath falls through into the resolver block. A bad line can either raiseUnboundLocalErroron the first failure or re-use the previous iteration’srequirementand resolve the wrong package.Suggested fix
except InvalidRequirement as err: msg = f"{path}: {line}: {err}" logger.error(msg) failures.append(msg) + continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/commands/lint_requirements.py` around lines 82 - 96, The except InvalidRequirement block logs and appends the failure but then continues into the resolver code and may reuse an invalid or previous `requirement`; update the control flow so that after handling the InvalidRequirement you skip resolution (e.g., add an immediate continue/return from the loop or wrap the resolver in an else) so the resolver block that uses `requirement`, `requirement_ctxvar.set`, and calls `bt.resolve_versions(..., req_type=RequirementType.TOP_LEVEL)` only runs when no InvalidRequirement was raised and `resolve_requirements` and `not is_constraints` are true.src/fromager/bootstrapper.py (1)
340-370:⚠️ Potential issue | 🟠 MajorUndo graph/seen state when a version fails.
By this point the version has already been marked as seen and added to the graph. The multi-version failure path removes the node but leaves the seen entry behind, so a later encounter can re-add the node at Line 325 and then return early at Line 334 without retrying. When
test_modeis also enabled, the early return at Lines 352-356 skipsremove_dependency()entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/bootstrapper.py` around lines 340 - 370, The code marks a version seen via _mark_as_seen and adds it to the dependency graph before bootstrapping, but on exceptions the seen state is never undone (only the graph node is removed in the multiple_versions branch), causing future early returns; update the exception handling in the try/except inside _bootstrap_impl's caller so that on any failure you undo the earlier mark and graph addition: either call a new helper (e.g. _unmark_as_seen(req, resolved_version, build_sdist_only)) or inline logic to remove the seen entry and remove_dependency from ctx.dependency_graph, and invoke that cleanup in both the test_mode branch (where _record_test_mode_failure is called) and the multiple_versions branch (before returning), and ensure non-test/non-multi cases also perform cleanup before re-raising or returning; use the existing symbols _mark_as_seen, _record_test_mode_failure, multiple_versions, test_mode, ctx.dependency_graph.remove_dependency to locate and implement the cleanup.
♻️ Duplicate comments (1)
e2e/test_bootstrap_multiple_versions.sh (1)
11-12:⚠️ Potential issue | 🟡 MinorQuote the trap command with single quotes.
$constraints_fileis expanded when the trap is installed, not when it runs.As per coding guidelines, `e2e/**/*.sh`: Check for proper cleanup and teardown (trap handlers). Ensure variables are quoted to prevent word splitting.Suggested fix
constraints_file=$(mktemp) -trap "rm -f $constraints_file" EXIT +trap 'rm -f "$constraints_file"' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_multiple_versions.sh` around lines 11 - 12, The trap handler currently uses double quotes so $constraints_file is expanded at trap installation instead of execution; change the trap invocation to use single quotes so the variable is expanded when the trap runs and also ensure the rm invocation inside the trap quotes the variable (constraints_file) to avoid word-splitting when cleaning up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/bootstrapper.py`:
- Around line 276-291: resolve_versions can return an empty list which needs to
be treated as a resolution failure; after calling resolve_versions (the
resolved_versions variable) add a check for if not resolved_versions and then
mirror the existing exception handling: if not self.test_mode re-raise a new
error (or raise a ResolutionError) otherwise call
self._record_test_mode_failure(req, None, err_or_new_exception, "resolution")
and return so the subsequent for loop over resolved_versions does not silently
skip the dependency; use the same error-path conventions as the current except
block and reference resolve_versions, resolved_versions, self.test_mode,
self._record_test_mode_failure, and self._bootstrap_single_version to locate
where to insert this check.
---
Outside diff comments:
In `@src/fromager/bootstrapper.py`:
- Around line 340-370: The code marks a version seen via _mark_as_seen and adds
it to the dependency graph before bootstrapping, but on exceptions the seen
state is never undone (only the graph node is removed in the multiple_versions
branch), causing future early returns; update the exception handling in the
try/except inside _bootstrap_impl's caller so that on any failure you undo the
earlier mark and graph addition: either call a new helper (e.g.
_unmark_as_seen(req, resolved_version, build_sdist_only)) or inline logic to
remove the seen entry and remove_dependency from ctx.dependency_graph, and
invoke that cleanup in both the test_mode branch (where
_record_test_mode_failure is called) and the multiple_versions branch (before
returning), and ensure non-test/non-multi cases also perform cleanup before
re-raising or returning; use the existing symbols _mark_as_seen,
_record_test_mode_failure, multiple_versions, test_mode,
ctx.dependency_graph.remove_dependency to locate and implement the cleanup.
In `@src/fromager/commands/lint_requirements.py`:
- Around line 82-96: The except InvalidRequirement block logs and appends the
failure but then continues into the resolver code and may reuse an invalid or
previous `requirement`; update the control flow so that after handling the
InvalidRequirement you skip resolution (e.g., add an immediate continue/return
from the loop or wrap the resolver in an else) so the resolver block that uses
`requirement`, `requirement_ctxvar.set`, and calls `bt.resolve_versions(...,
req_type=RequirementType.TOP_LEVEL)` only runs when no InvalidRequirement was
raised and `resolve_requirements` and `not is_constraints` are true.
---
Duplicate comments:
In `@e2e/test_bootstrap_multiple_versions.sh`:
- Around line 11-12: The trap handler currently uses double quotes so
$constraints_file is expanded at trap installation instead of execution; change
the trap invocation to use single quotes so the variable is expanded when the
trap runs and also ensure the rm invocation inside the trap quotes the variable
(constraints_file) to avoid word-splitting when cleaning up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ccc3cfab-161e-4537-9baf-cfce8e73456a
📒 Files selected for processing (11)
e2e/ci_bootstrap_suite.she2e/test_bootstrap_multiple_versions.shsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/bootstrapper.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/lint_requirements.pysrc/fromager/dependency_graph.pytests/test_bootstrap_requirement_resolver.pytests/test_bootstrap_requirement_resolver_multiple.pytests/test_bootstrap_test_mode.pytests/test_bootstrapper.py
✅ Files skipped from review due to trivial changes (3)
- e2e/ci_bootstrap_suite.sh
- tests/test_bootstrapper.py
- tests/test_bootstrap_requirement_resolver_multiple.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/fromager/dependency_graph.py
- src/fromager/bootstrap_requirement_resolver.py
Member
Contributor
Author
#1036 is the sub task in the feature for multiple version bootstrap. We are tracking these sub tasks downstream as well |
LalatenduMohanty
requested changes
Apr 8, 2026
dhellmann
reviewed
Apr 8, 2026
Member
dhellmann
left a comment
dhellmann
left a comment
There was a problem hiding this comment.
This looks very close. There are some comments from CodeRabbit that look like they're worth investigating, so I'll leave it open for those updates. And I had one question about the handling of top-level dependencies that might lead to a change.
LalatenduMohanty
requested changes
Apr 8, 2026
rd4398
force-pushed
the
bootstrap-multiple-version-flag
branch
from
2f0096e to
321a95f
Compare
Contributor
Author
Thank you! I have addressed the feedback from CodeRabbit as well |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_bootstrap_requirement_resolver_multiple.py (1)
167-172: Consider verifying highest-first ordering explicitly.The comment says "should be sorted highest first" but the assertion only checks membership. For completeness, verify the ordering:
# Should return both versions from graph assert len(results) == 2 # Verify versions (should be sorted highest first) - versions = [v for _, v in results] - assert Version("2.0") in versions - assert Version("1.0") in versions + assert results[0][1] == Version("2.0") # Highest first + assert results[1][1] == Version("1.0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bootstrap_requirement_resolver_multiple.py` around lines 167 - 172, The test currently only checks membership for results/versions but not that they're sorted highest-first; update the assertions around results/versions in tests/test_bootstrap_requirement_resolver_multiple.py to assert the ordering explicitly (e.g., that the extracted versions list equals [Version("2.0"), Version("1.0")] or that versions[0] == Version("2.0") and versions[1] == Version("1.0")). Use the existing variables results and versions and the Version(...) constructor to perform the ordered check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/bootstrap_requirement_resolver.py`:
- Around line 112-116: After calling self._resolve(...) and when using cached
results from self.cache_resolution(...), guard against an empty results list
before indexing results[0]: if not results then return [] when
return_all_versions is True, otherwise raise a clear exception (e.g.,
RequirementResolutionError or ValueError) that includes the request/specifier
context; apply this same empty-check to the cached-results branch and the
fresh-resolution branch so no IndexError can occur.
In `@src/fromager/dependency_graph.py`:
- Around line 363-370: In the node-removal loop (the code that filters
node.children by edge.destination_node.key), also remove the corresponding
parent references from the destination node so we don't leave dangling entries
in node.parents; specifically, when you detect an edge whose
destination_node.key == key, remove that edge from the current node.children and
also remove the matching parent edge/reference from
edge.destination_node.parents (or filter destination_node.parents to exclude
parents whose parent key == key). Update the same removal code in the method
that deletes nodes (referencing node.children, edge.destination_node, and
node.parents) so traversals like _collect_dependents() no longer see stale
parent references.
---
Nitpick comments:
In `@tests/test_bootstrap_requirement_resolver_multiple.py`:
- Around line 167-172: The test currently only checks membership for
results/versions but not that they're sorted highest-first; update the
assertions around results/versions in
tests/test_bootstrap_requirement_resolver_multiple.py to assert the ordering
explicitly (e.g., that the extracted versions list equals [Version("2.0"),
Version("1.0")] or that versions[0] == Version("2.0") and versions[1] ==
Version("1.0")). Use the existing variables results and versions and the
Version(...) constructor to perform the ordered check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a114441-3a33-4eec-ac89-f9cf06ee30d4
📒 Files selected for processing (11)
e2e/ci_bootstrap_suite.she2e/test_bootstrap_multiple_versions.shsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/bootstrapper.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/lint_requirements.pysrc/fromager/dependency_graph.pytests/test_bootstrap_requirement_resolver.pytests/test_bootstrap_requirement_resolver_multiple.pytests/test_bootstrap_test_mode.pytests/test_bootstrapper.py
✅ Files skipped from review due to trivial changes (1)
- e2e/ci_bootstrap_suite.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_bootstrap_test_mode.py
- tests/test_bootstrap_requirement_resolver.py
- tests/test_bootstrapper.py
- src/fromager/commands/bootstrap.py
LalatenduMohanty
previously approved these changes
Apr 8, 2026
Member
LalatenduMohanty
left a comment
LalatenduMohanty
left a comment
There was a problem hiding this comment.
The review comments I have are not blocker for this PR.
dhellmann
approved these changes
Apr 9, 2026
dhellmann
requested changes
Apr 9, 2026
rd4398
force-pushed
the
bootstrap-multiple-version-flag
branch
from
6a0dd0b to
72aad14
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fromager/bootstrapper.py (1)
351-387:⚠️ Potential issue | 🟠 MajorClear the seen state when a version fails in multi-version mode.
Line 342 adds the node before the seen check, and Line 357 marks it seen before the build runs. If the build then fails, you remove the node from the graph but keep the seen marker. A later dependency on the same version will re-add the node, hit
_has_been_seen(), and return early, leaving a failed version back in the graph.Proposed fix
if self.multiple_versions: pkg_name = canonicalize_name(req.name) self._failed_versions.append((pkg_name, str(resolved_version), err)) + self._seen_requirements.discard( + self._resolved_key(req, resolved_version, "sdist") + ) + self._seen_requirements.discard( + self._resolved_key(req, resolved_version, "wheel") + ) logger.warning( f"{req.name}=={resolved_version}: failed to bootstrap: {type(err).__name__}: {err}" ) # Remove failed node from graph since bootstrap didn't complete self.ctx.dependency_graph.remove_dependency(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/bootstrapper.py` around lines 351 - 387, When bootstrap fails in multiple_versions mode we remove the node from the graph but forget to clear the "seen" marker, so future attempts skip this version via _has_been_seen; modify the exception handler in the block wrapping _bootstrap_impl to call whatever clears the seen state (the inverse of _mark_as_seen) for the given req, resolved_version, and build_sdist_only before appending to _failed_versions and returning; ensure this cleanup runs only when self.multiple_versions is true (and similarly for test_mode if applicable) so subsequent retries won't be short-circuited by the stale seen marker.
♻️ Duplicate comments (3)
src/fromager/bootstrapper.py (1)
287-302:⚠️ Potential issue | 🟠 MajorKeep the empty-resolution failure inside the test-mode error path.
resolve_versions(..., return_all_versions=True)can return[]. Right now theRuntimeErroris raised after thetry/except, so--test-modeaborts instead of recording a resolution failure and continuing.Proposed fix
try: resolved_versions = self.resolve_versions( req=req, req_type=req_type, return_all_versions=self.multiple_versions, ) + if not resolved_versions: + raise RuntimeError(f"Could not resolve any versions for {req}") if self.multiple_versions: logger.info(f"resolved {len(resolved_versions)} version(s) for {req}") except Exception as err: if not self.test_mode: raise self._record_test_mode_failure(req, None, err, "resolution") return - - # Check if resolution returned no versions - if not resolved_versions: - raise RuntimeError(f"Could not resolve any versions for {req}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/bootstrapper.py` around lines 287 - 302, The code raises RuntimeError for empty resolved_versions outside the try/except which bypasses test-mode handling; modify the empty-resolution check so it respects self.test_mode: after calling resolve_versions (where resolved_versions may be []), if not resolved_versions then if not self.test_mode raise RuntimeError(f"Could not resolve any versions for {req}") else call self._record_test_mode_failure(req, None, RuntimeError(f"Could not resolve any versions for {req}"), "resolution") and return; reference resolve_versions, resolved_versions, self.test_mode, and _record_test_mode_failure to locate and update the logic.e2e/test_bootstrap_multiple_versions.sh (1)
11-12:⚠️ Potential issue | 🟡 MinorUse single quotes in the trap.
The current trap expands
$constraints_filewhen the trap is set, not on exit. That can leave the temp file cleanup tied to the wrong value.Proposed fix
constraints_file=$(mktemp) -trap "rm -f $constraints_file" EXIT +trap 'rm -f "$constraints_file"' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_bootstrap_multiple_versions.sh` around lines 11 - 12, The trap currently interpolates $constraints_file at trap setup time, which can capture the wrong filename; change the trap command so it defers expansion by using single quotes around the command (i.e., update the trap line that references constraints_file and EXIT) so the removal command runs with the actual value of the constraints_file created by mktemp at script exit.src/fromager/bootstrap_requirement_resolver.py (1)
107-116:⚠️ Potential issue | 🟠 MajorGuard empty resolution results before indexing.
Both branches index
[0]. If a specifier matches nothing,return_all_versions=Truecan legitimately produce[], and the default path then crashes withIndexErrorinstead of raising a clear resolution error.Proposed fix
cached_result = self.get_cached_resolution(req, pre_built) if cached_result is not None: logger.debug(f"resolved {req} from cache") - return cached_result if return_all_versions else [cached_result[0]] + if return_all_versions: + return cached_result + if not cached_result: + raise ValueError(f"No matching versions found for {req}") + return [cached_result[0]] @@ results = self._resolve(req, req_type, parent_req, pre_built) # Cache the result self.cache_resolution(req, pre_built, results) - return results if return_all_versions else [results[0]] + if return_all_versions: + return results + if not results: + raise ValueError(f"No matching versions found for {req}") + return [results[0]]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/bootstrap_requirement_resolver.py` around lines 107 - 116, The code currently indexes [0] on cached_result and results without checking for emptiness causing IndexError when no versions match; in the resolve flow (function/method using cached_result, self._resolve and self.cache_resolution) guard both branches by checking if cached_result (when not None) or results is empty and raise a clear resolution exception (e.g., a ResolutionError or ValueError) before attempting to return [cached_result[0]] or [results[0]]; ensure return_all_versions still returns [] when requested and that the error is raised only for the single-version return path when there are no matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_bootstrap_requirement_resolver_multiple.py`:
- Around line 167-172: The test currently only checks membership of
Version("2.0") and Version("1.0") in versions extracted from results but claims
the list is "sorted highest first"; change the assertions to verify order
explicitly by asserting that versions equals the expected ordered list (e.g.
first Version("2.0"), then Version("1.0")) or by asserting versions[0] is
Version("2.0") and versions[1] is Version("1.0"); update the assertions that
reference results and Version to reflect this exact-order check so the test
fails on ordering regressions.
---
Outside diff comments:
In `@src/fromager/bootstrapper.py`:
- Around line 351-387: When bootstrap fails in multiple_versions mode we remove
the node from the graph but forget to clear the "seen" marker, so future
attempts skip this version via _has_been_seen; modify the exception handler in
the block wrapping _bootstrap_impl to call whatever clears the seen state (the
inverse of _mark_as_seen) for the given req, resolved_version, and
build_sdist_only before appending to _failed_versions and returning; ensure this
cleanup runs only when self.multiple_versions is true (and similarly for
test_mode if applicable) so subsequent retries won't be short-circuited by the
stale seen marker.
---
Duplicate comments:
In `@e2e/test_bootstrap_multiple_versions.sh`:
- Around line 11-12: The trap currently interpolates $constraints_file at trap
setup time, which can capture the wrong filename; change the trap command so it
defers expansion by using single quotes around the command (i.e., update the
trap line that references constraints_file and EXIT) so the removal command runs
with the actual value of the constraints_file created by mktemp at script exit.
In `@src/fromager/bootstrap_requirement_resolver.py`:
- Around line 107-116: The code currently indexes [0] on cached_result and
results without checking for emptiness causing IndexError when no versions
match; in the resolve flow (function/method using cached_result, self._resolve
and self.cache_resolution) guard both branches by checking if cached_result
(when not None) or results is empty and raise a clear resolution exception
(e.g., a ResolutionError or ValueError) before attempting to return
[cached_result[0]] or [results[0]]; ensure return_all_versions still returns []
when requested and that the error is raised only for the single-version return
path when there are no matches.
In `@src/fromager/bootstrapper.py`:
- Around line 287-302: The code raises RuntimeError for empty resolved_versions
outside the try/except which bypasses test-mode handling; modify the
empty-resolution check so it respects self.test_mode: after calling
resolve_versions (where resolved_versions may be []), if not resolved_versions
then if not self.test_mode raise RuntimeError(f"Could not resolve any versions
for {req}") else call self._record_test_mode_failure(req, None,
RuntimeError(f"Could not resolve any versions for {req}"), "resolution") and
return; reference resolve_versions, resolved_versions, self.test_mode, and
_record_test_mode_failure to locate and update the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 599e8fc0-f085-4d49-a89d-61cde10ca115
📒 Files selected for processing (11)
e2e/ci_bootstrap_suite.she2e/test_bootstrap_multiple_versions.shsrc/fromager/bootstrap_requirement_resolver.pysrc/fromager/bootstrapper.pysrc/fromager/commands/bootstrap.pysrc/fromager/commands/lint_requirements.pysrc/fromager/dependency_graph.pytests/test_bootstrap_requirement_resolver.pytests/test_bootstrap_requirement_resolver_multiple.pytests/test_bootstrap_test_mode.pytests/test_bootstrapper.py
✅ Files skipped from review due to trivial changes (3)
- e2e/ci_bootstrap_suite.sh
- tests/test_bootstrap_requirement_resolver.py
- tests/test_bootstrapper.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/fromager/commands/lint_requirements.py
- tests/test_bootstrap_test_mode.py
- src/fromager/commands/bootstrap.py
- src/fromager/dependency_graph.py
LalatenduMohanty
approved these changes
Apr 10, 2026
dhellmann
approved these changes
Apr 10, 2026
Member
|
@rd4398 go ahead and resolve the conversation threads when have read them and are ready to merge this. |
This was referenced Apr 10, 2026
Contributor
Author
|
@mergify merge |
Contributor
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
Contributor
Author
|
@mergify refresh |
Contributor
✅ Pull request refreshed |
Member
|
@mergify rebase |
…ng versions Add --multiple-versions flag to bootstrap command that enables bootstrapping all versions matching a requirement specification, rather than only the highest version. This is useful for creating comprehensive build environments with multiple versions of the same package. Key changes: - Add return_all_versions parameter to BootstrapRequirementResolver.resolve() with type-safe overloads using @overload decorators - Modify Bootstrapper.bootstrap() to iterate over all resolved versions when --multiple-versions flag is enabled - Implement continue-on-error behavior: failed versions are logged, tracked, and reported at the end without stopping the bootstrap process - Add DependencyGraph.remove_dependency() to clean up failed nodes from graph - Apply recursively to entire dependency chain (not just top-level) Testing: - Add 4 unit tests for resolver return_all_versions behavior - Add unit test for continue-on-error and graph cleanup behavior - Add e2e test using tomli>=2.0,<2.1 with constraints to verify multiple versions are bootstrapped successfully Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
Contributor
|
Deprecation notice: This pull request comes from a fork and was rebased using |
Contributor
✅ Branch has been successfully rebased |
LalatenduMohanty
force-pushed
the
bootstrap-multiple-version-flag
branch
from
72aad14 to
30e2a28
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add --multiple-versions flag to bootstrap command that enables bootstrapping all versions matching a requirement specification, rather than only the highest version. This is useful for creating comprehensive build environments with multiple versions of the same package.
Key changes:
Testing:
Closes: #1036