From e23ed8a68374f0cdb485b52f58136ab978546771 Mon Sep 17 00:00:00 2001 From: Andre Lustosa Date: Fri, 18 Sep 2026 09:52:58 -0400 Subject: [PATCH] fix(bootstrap): forget seen-markers of cascade-removed dependencies In multiple-versions mode a failed version is removed from the graph together with every descendant that becomes orphaned, including ones that already completed bootstrap with their build-system edges. Only the failed package's own seen-markers were cleared, so when an orphaned descendant was encountered again it was re-added as a bare node by Start and skipped as already seen. graph.json then carried the node with no build edges and build-parallel scheduled it in round 1 before its build backend was available. Make remove_dependency return the removed nodes and clear the seen-markers for every one of them, so a re-encountered orphan goes through PrepareSource again. Closes #1335 Co-Authored-By: Claude Opus Signed-off-by: Andre Lustosa --- src/fromager/bootstrapper/_bootstrapper.py | 24 +++-- src/fromager/dependency_graph.py | 12 ++- tests/test_bootstrapper_iterative.py | 105 +++++++++++++++++++++ tests/test_dependency_graph.py | 19 +++- 4 files changed, 151 insertions(+), 9 deletions(-) diff --git a/src/fromager/bootstrapper/_bootstrapper.py b/src/fromager/bootstrapper/_bootstrapper.py index a12e67532..7ab6fb17e 100644 --- a/src/fromager/bootstrapper/_bootstrapper.py +++ b/src/fromager/bootstrapper/_bootstrapper.py @@ -641,6 +641,16 @@ def mark_as_seen( # Mark wheel seen only for wheel build self._seen_requirements.add(self._resolved_key(req, version, "wheel")) + def _forget_seen(self, name: NormalizedName, version: Version) -> None: + """Drop every seen-marker for name==version, whatever extras or type.""" + version_str = str(version) + stale = { + key + for key in self._seen_requirements + if key[0] == name and key[2] == version_str + } + self._seen_requirements.difference_update(stale) + def has_been_seen( self, req: Requirement, @@ -1021,13 +1031,15 @@ def _handle_phase_error( err, f"failed during {type(item).phase} phase", ) - self.ctx.dependency_graph.remove_dependency(pkg_name, wi.resolved_version) - self._seen_requirements.discard( - self._resolved_key(wi.req, wi.resolved_version, "sdist") - ) - self._seen_requirements.discard( - self._resolved_key(wi.req, wi.resolved_version, "wheel") + removed_nodes = self.ctx.dependency_graph.remove_dependency( + pkg_name, wi.resolved_version ) + # The failed version may not be in the graph yet. Descendants + # removed with it lost their edges; forget them too so a later + # encounter goes through PrepareSource again. + self._forget_seen(pkg_name, wi.resolved_version) + for node in removed_nodes: + self._forget_seen(node.canonicalized_name, node.version) self._write_graph_async() return [] diff --git a/src/fromager/dependency_graph.py b/src/fromager/dependency_graph.py index 6d8aa3496..2584d3cc3 100644 --- a/src/fromager/dependency_graph.py +++ b/src/fromager/dependency_graph.py @@ -364,7 +364,7 @@ def remove_dependency( self, req_name: NormalizedName, req_version: Version, - ) -> None: + ) -> list[DependencyNode]: """Remove a dependency node and any orphaned descendants from the graph. Removes the node and all edges pointing to it. Child nodes that have @@ -373,11 +373,16 @@ def remove_dependency( Args: req_name: Canonical name of the package req_version: Version of the package + + Returns: + The removed nodes: the requested node first, then the orphaned + descendants. Empty if the node is not in the graph. """ + removed: list[DependencyNode] = [] key = f"{req_name}=={req_version}" if key not in self.nodes: logger.debug(f"Cannot remove {key} - not in graph") - return + return removed queue: collections.deque[str] = collections.deque([key]) @@ -392,6 +397,7 @@ def remove_dependency( logger.debug(f"Removing failed dependency {key} from graph") deleted_node = self.nodes[key] + removed.append(deleted_node) # Remove references to this node from its direct children children = [] @@ -424,6 +430,8 @@ def remove_dependency( if child.key != ROOT and child.key in self.nodes and not child.parents: queue.append(child.key) + return removed + def get_dependency_edges( self, match_dep_types: list[RequirementType] | None = None ) -> typing.Iterable[DependencyEdge]: diff --git a/tests/test_bootstrapper_iterative.py b/tests/test_bootstrapper_iterative.py index 29fb89bff..c52555e2d 100644 --- a/tests/test_bootstrapper_iterative.py +++ b/tests/test_bootstrapper_iterative.py @@ -1066,6 +1066,111 @@ def test_multiple_versions_logs_phase( assert "prepare-build phase" in caplog.text assert "compile error" in caplog.text + def test_multiple_versions_forgets_cascade_removed_descendants( + self, tmp_context: WorkContext + ) -> None: + bt = bootstrapper.Bootstrapper(tmp_context, multiple_versions=True) + bt.why = [] + graph = tmp_context.dependency_graph + req_a = Requirement("a") + req_b = Requirement("b[cli]>=1") + req_backend = Requirement("backend") + v1 = Version("1.0") + graph.add_dependency( + parent_name=None, + parent_version=None, + req_type=RequirementType.TOP_LEVEL, + req=req_a, + req_version=v1, + download_url="https://example.com/a-1.0.tar.gz", + ) + graph.add_dependency( + parent_name=canonicalize_name("a"), + parent_version=v1, + req_type=RequirementType.BUILD_SYSTEM, + req=req_b, + req_version=v1, + download_url="https://example.com/b-1.0.tar.gz", + ) + graph.add_dependency( + parent_name=canonicalize_name("b"), + parent_version=v1, + req_type=RequirementType.BUILD_SYSTEM, + req=req_backend, + req_version=v1, + download_url="https://example.com/backend-1.0.tar.gz", + ) + for req in (req_a, req_b, req_backend): + bt.mark_as_seen(req, v1) + + item = _make_build_item(req="a", version="1.0", phase=BootstrapPhase.BUILD) + bt._handle_phase_error(item, ValueError("build failed")) + + # a and its orphaned descendants are gone from the graph and forgotten + assert "b==1.0" not in graph.nodes + assert "backend==1.0" not in graph.nodes + assert not bt.has_been_seen(Requirement("b"), v1) + assert not bt.has_been_seen(req_b, v1) + assert not bt.has_been_seen(req_backend, v1) + assert not bt.has_failed_version(canonicalize_name("b"), v1) + + # b encountered again is processed, not skipped as already seen + req_c = Requirement("c") + graph.add_dependency( + parent_name=None, + parent_version=None, + req_type=RequirementType.TOP_LEVEL, + req=req_c, + req_version=v1, + download_url="https://example.com/c-1.0.tar.gz", + ) + start_b = Start( + WorkItem( + req=Requirement("b"), + req_type=RequirementType.INSTALL, + why_snapshot=[], + parent=(req_c, v1), + source_url="https://example.com/b-1.0.tar.gz", + resolved_version=v1, + ) + ) + result = start_b.run(bt) + assert len(result) == 1 + assert isinstance(result[0], PrepareSource) + + # PrepareSource finds the cached wheel and restores the build-system + # edge from the requirements file extracted from it, without a rebuild + prepare_b = result[0] + unpacked = tmp_context.work_dir / "b-1.0" + unpacked.mkdir(parents=True) + unpacked.joinpath("build-system-requirements.txt").write_text("backend\n") + cached_wheel = tmp_context.wheels_build / "b-1.0-py3-none-any.whl" + prepare_b.bg_future = _make_resolved_future( + PreparedSourceData( + sdist_root_dir=unpacked / "b-1.0", + cached_wheel_filename=cached_wheel, + ) + ) + with patch("fromager.build_environment.BuildEnvironment", return_value=Mock()): + prepare_result = prepare_b.run(bt) + assert prepare_b.work_item.cached_wheel_filename == cached_wheel + resolve_items = [it for it in prepare_result if isinstance(it, Resolve)] + assert [str(it.work_item.req) for it in resolve_items] == ["backend"] + assert resolve_items[0].work_item.req_type == RequirementType.BUILD_SYSTEM + Start( + WorkItem( + req=req_backend, + req_type=RequirementType.BUILD_SYSTEM, + why_snapshot=[], + parent=(Requirement("b"), v1), + source_url="https://example.com/backend-1.0.tar.gz", + resolved_version=v1, + ) + ).run(bt) + assert [n.key for n in graph.nodes["b==1.0"].iter_build_requirements()] == [ + "backend==1.0" + ] + # -- Normal mode errors -- def test_normal_mode_raises(self, tmp_context: WorkContext) -> None: diff --git a/tests/test_dependency_graph.py b/tests/test_dependency_graph.py index c4086f4a4..71f71a829 100644 --- a/tests/test_dependency_graph.py +++ b/tests/test_dependency_graph.py @@ -702,6 +702,23 @@ def test_remove_dependency_nonexistent() -> None: graph = _build_graph(("ROOT", "a", "toplevel")) node_count = len(graph.nodes) - graph.remove_dependency(canonicalize_name("nonexistent"), Version("1.0")) + removed = graph.remove_dependency(canonicalize_name("nonexistent"), Version("1.0")) + assert removed == [] assert len(graph.nodes) == node_count + + +def test_remove_dependency_returns_removed_nodes() -> None: + graph = _build_graph( + ("ROOT", "a", "toplevel"), + ("ROOT", "d", "toplevel"), + ("a", "b", "build-system"), + ("b", "c", "build-backend"), + ("a", "shared", "install"), + ("d", "shared", "install"), + ) + + removed = graph.remove_dependency(canonicalize_name("a"), Version("1.0")) + + assert [n.key for n in removed] == ["a==1.0", "b==1.0", "c==1.0"] + assert "shared==1.0" in graph.nodes