From 8af34805d0c9cab070bd6c47571a339fa10932d3 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Sun, 6 Feb 2022 20:45:34 -0800 Subject: [PATCH] Fix native cycle detection error message. (#14381) As described in #13744, #13370 broke the error message for native cycle detection, by reporting unrelated nodes. Fix the issue (node ids for the running subgraph were being used with the underlying graph when rendering the path) and expand the test. Fixes #13744. [ci skip-build-wheels] --- src/rust/engine/graph/src/lib.rs | 21 ++++++++---- src/rust/engine/graph/src/node.rs | 11 ++++--- src/rust/engine/graph/src/tests.rs | 12 +++---- src/rust/engine/src/nodes.rs | 52 +++++++++++++++--------------- 4 files changed, 52 insertions(+), 44 deletions(-) diff --git a/src/rust/engine/graph/src/lib.rs b/src/rust/engine/graph/src/lib.rs index 4bfb39ed6dc..a6ed4f0cc4d 100644 --- a/src/rust/engine/graph/src/lib.rs +++ b/src/rust/engine/graph/src/lib.rs @@ -144,13 +144,19 @@ impl InnerGraph { let (running_candidate, should_terminate) = if let Some(dirty_candidate) = running_scc .iter() .filter(|&id| self.pg[running_graph[*id]].is_cleaning()) - .max() + .max_by_key(|&id| running_graph[*id]) { // Nodes are being cleaned: clear the highest id entry. (dirty_candidate, false) } else { // There are no nodes being cleaned: terminate the Running node with the highest id. - (running_scc.iter().max().unwrap(), true) + ( + running_scc + .iter() + .max_by_key(|&id| running_graph[*id]) + .unwrap(), + true, + ) }; test_trace_log!( @@ -168,7 +174,7 @@ impl InnerGraph { // predecessor (which as a fellow member of the SCC, must also be reachable). let running_predecessor = running_graph .neighbors_directed(*running_candidate, Direction::Incoming) - .next() + .find(|id| running_scc.contains(id)) .unwrap(); let running_path: Vec<_> = petgraph::algo::all_simple_paths( &running_graph, @@ -184,11 +190,12 @@ impl InnerGraph { let candidate = running_graph[*running_candidate]; if should_terminate { // Render the error, and terminate the Node with it. - let path_strs = running_path + let path = running_path .into_iter() - .map(|rni| self.pg[rni].node().to_string()) - .collect(); - self.pg[candidate].terminate(N::Error::cyclic(path_strs)); + .map(|rni| self.pg[running_graph[rni]].node()) + .collect::>(); + let error = N::cyclic_error(&path); + self.pg[candidate].terminate(error); } else { // Else, clear. let node = self.pg[candidate].node().clone(); diff --git a/src/rust/engine/graph/src/node.rs b/src/rust/engine/graph/src/node.rs index 211b03b8b4b..a597ebf26f1 100644 --- a/src/rust/engine/graph/src/node.rs +++ b/src/rust/engine/graph/src/node.rs @@ -53,6 +53,12 @@ pub trait Node: Clone + Debug + Display + Eq + Hash + Send + 'static { fn cacheable_item(&self, _item: &Self::Item) -> bool { self.cacheable() } + + /// + /// Creates an error instance that represents that a Node dependency was cyclic along the given + /// path. + /// + fn cyclic_error(path: &[&Self]) -> Self::Error; } pub trait NodeError: Clone + Debug + Eq + Send + Sync { @@ -61,11 +67,6 @@ pub trait NodeError: Clone + Debug + Eq + Send + Sync { /// Graph (generally while running). /// fn invalidated() -> Self; - - /// - /// Creates an instance that represents that a Node dependency was cyclic along the given path. - /// - fn cyclic(path: Vec) -> Self; } /// diff --git a/src/rust/engine/graph/src/tests.rs b/src/rust/engine/graph/src/tests.rs index ca7b4930816..b034c40019c 100644 --- a/src/rust/engine/graph/src/tests.rs +++ b/src/rust/engine/graph/src/tests.rs @@ -624,7 +624,7 @@ async fn cyclic_failure() { assert_eq!( graph.create(TNode::new(2), &context).await, - Err(TError::Cyclic) + Err(TError::Cyclic(vec![0, 2, 1])) ); } @@ -740,6 +740,10 @@ impl Node for TNode { fn cacheable(&self) -> bool { self.cacheable } + + fn cyclic_error(path: &[&Self]) -> Self::Error { + TError::Cyclic(path.iter().map(|n| n.id).collect()) + } } impl std::fmt::Display for TNode { @@ -991,15 +995,11 @@ impl Drop for AbortGuard { #[derive(Clone, Debug, Eq, PartialEq)] enum TError { - Cyclic, + Cyclic(Vec), Invalidated, } impl NodeError for TError { fn invalidated() -> Self { TError::Invalidated } - - fn cyclic(_path: Vec) -> Self { - TError::Cyclic - } } diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 4b52864941c..7f2c5ba193b 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -1569,6 +1569,32 @@ impl Node for NodeKey { _ => true, } } + + fn cyclic_error(path: &[&NodeKey]) -> Failure { + let mut path = path.iter().map(|n| n.to_string()).collect::>(); + if !path.is_empty() { + path[0] += " <-"; + path.push(path[0].clone()); + } + let gil = Python::acquire_gil(); + let url = externs::doc_url( + gil.python(), + "targets#dependencies-and-dependency-inference", + ); + throw(format!( + "The dependency graph contained a cycle:\ + \n\n \ + {}\ + \n\n\ + If the dependencies in the above path are for your BUILD targets, you may need to use more \ + granular targets or replace BUILD target dependencies with file dependencies. If they are \ + not for your BUILD targets, then please file a Github issue!\ + \n\n\ + See {} for more information.", + path.join("\n "), + url + )) + } } impl Display for NodeKey { @@ -1613,32 +1639,6 @@ impl NodeError for Failure { fn invalidated() -> Failure { Failure::Invalidated } - - fn cyclic(mut path: Vec) -> Failure { - let path_len = path.len(); - if path_len > 1 { - path[0] += " <-"; - path[path_len - 1] += " <-" - } - let gil = Python::acquire_gil(); - let url = externs::doc_url( - gil.python(), - "targets#dependencies-and-dependency-inference", - ); - throw(format!( - "The dependency graph contained a cycle:\ - \n\n \ - {}\ - \n\n\ - If the dependencies in the above path are for your BUILD targets, you may need to use more \ - granular targets or replace BUILD target dependencies with file dependencies. If they are \ - not for your BUILD targets, then please file a Github issue!\ - \n\n\ - See {} for more information.", - path.join("\n "), - url - )) - } } #[derive(Clone, Debug, Eq, PartialEq)]