feat(bootstrap): add --multiple-versions flag to bootstrap all matching versions#1035
Open
rd4398 wants to merge 1 commit intopython-wheel-build:mainfrom
Open
feat(bootstrap): add --multiple-versions flag to bootstrap all matching versions#1035rd4398 wants to merge 1 commit intopython-wheel-build:mainfrom
rd4398 wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds an opt-in multiple-versions bootstrap mode. The CLI gains 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. 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
…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>
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
Comment on lines
+363
to
+370
| # 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) |
There was a problem hiding this comment.
Stale parent references left behind after node removal.
The method removes edges from node.children but doesn't clean up node.parents on the remaining nodes that pointed to the deleted node. Nodes that had the deleted node as a parent (via edges in their parents list) will retain dangling references. This could cause issues in _collect_dependents() and similar traversals that iterate node.parents.
Proposed fix
# 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():
+ # Remove from children
filtered_children = [
edge for edge in node.children if edge.destination_node.key != key
]
node.children.clear()
node.children.extend(filtered_children)
+ # Remove from parents
+ 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 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.
There was a problem hiding this comment.
@dhellmann Yes, the issue is still present. The current code only filters node.children edges (forward references pointing to the removed node), but never cleans the parents list on the removed node's own children.
Concretely: when X is removed, any node that was a child of X still has X in its parents list. Methods like get_incoming_install_edges() and get_top_level_requirement() iterate node.parents, so they'll encounter a dangling reference to the deleted node.
Here's a more targeted fix — capture the node before deletion and only clean the affected parents entries (avoids scanning all nodes for parents cleanup):
def remove_dependency(
self,
req_name: NormalizedName,
req_version: Version,
) -> None:
key = f"{req_name}=={req_version}"
if key not in self.nodes:
logger.debug(f"Cannot remove {key} - not in graph")
return
logger.debug(f"Removing failed dependency {key} from graph")
deleted_node = self.nodes[key]
# Clean up back-references (parents) in nodes that were children of the removed node
for child_edge in deleted_node.children:
child_node = child_edge.destination_node
child_node.parents[:] = [
e for e in child_node.parents if e.destination_node.key != key
]
# Remove the node itself
del self.nodes[key]
# Remove forward edges from any node whose children pointed to the removed node
for node in self.nodes.values():
node.children[:] = [
e for e in node.children if e.destination_node.key != key
]There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| ) | ||
|
|
||
| # Return first result for backward compatibility | ||
| return results[0] |
Member
There was a problem hiding this comment.
With in fromager commands/bootstrap.py:189 is the only caller for resolve_and_add_top_level() and it check for is not None
result = bt.resolve_and_add_top_level(req)
if result is not None:
resolved_reqs.append(req)
Are you expecting other consumers of fromager using this?
Member
There was a problem hiding this comment.
@LalatenduMohanty I don't understand your comment here, can you add some more detail, please?
LalatenduMohanty
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
| constraint=self.ctx.constraints.get_constraint(req.name), | ||
| ) | ||
| return (source_url, version) | ||
| if len(results) > 1: |
Member
There was a problem hiding this comment.
I think we really want to check if we were asked to return all versions here, that way if we were and we get 0 or 1 we log that.
| ) | ||
|
|
||
| # Return first result for backward compatibility | ||
| return results[0] |
Member
There was a problem hiding this comment.
@LalatenduMohanty I don't understand your comment here, can you add some more detail, please?
Comment on lines
+363
to
+370
| # 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) |
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