From 841653f85637b9763cbacd5af2909ccbc52d6fb9 Mon Sep 17 00:00:00 2001 From: Vishakha Agrawal Date: Wed, 31 Jul 2019 13:23:06 -0700 Subject: [PATCH 1/6] Fixed RemoveEdege() during the list iteration issue. --- .../ngraph_replace_op_utilities.cc | 13 ++++++++++--- ngraph_bridge/ngraph_capture_variables.cc | 4 ++++ ngraph_bridge/ngraph_rewrite_for_tracking.cc | 10 +++++++++- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/ngraph_bridge/enable_variable_ops/ngraph_replace_op_utilities.cc b/ngraph_bridge/enable_variable_ops/ngraph_replace_op_utilities.cc index e45104875..126149bfa 100644 --- a/ngraph_bridge/enable_variable_ops/ngraph_replace_op_utilities.cc +++ b/ngraph_bridge/enable_variable_ops/ngraph_replace_op_utilities.cc @@ -194,12 +194,16 @@ Status ReplaceVariable(Graph* graph, Node* node, Node** replacement, // Though edges will be removed when we remove the node // we specifically remove the edges to be sure Status ReplaceInputControlEdges(Graph* graph, Node* node, Node* replacement) { + std::vector edges_to_remove; for (auto edge : node->in_edges()) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); if (!edge->IsControlEdge()) continue; graph->AddEdge(edge->src(), edge->src_output(), replacement, edge->dst_input()); - graph->RemoveEdge(edge); + edges_to_remove.push_back(edge); + } + for (auto edge : edges_to_remove) { + graph->RemoveEdge(edge); } return Status::OK(); } @@ -208,6 +212,7 @@ Status ReplaceInputControlEdges(Graph* graph, Node* node, Node* replacement) { // we specifically remove the edges to be sure Status ReplaceOutputEdges(Graph* graph, Node* node, Node* replacement) { std::vector edges; + std::vector edges_to_remove; for (auto edge : node->out_edges()) { edges.push_back(edge); } @@ -216,9 +221,11 @@ Status ReplaceOutputEdges(Graph* graph, Node* node, Node* replacement) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); graph->AddEdge(replacement, edge->src_output(), edge->dst(), edge->dst_input()); - graph->RemoveEdge(edge); + edges_to_remove.push_back(edge); + } + for (auto edge : edges_to_remove) { + graph->RemoveEdge(edge); } - return Status::OK(); } diff --git a/ngraph_bridge/ngraph_capture_variables.cc b/ngraph_bridge/ngraph_capture_variables.cc index a751a8479..ea4f678d7 100644 --- a/ngraph_bridge/ngraph_capture_variables.cc +++ b/ngraph_bridge/ngraph_capture_variables.cc @@ -112,6 +112,10 @@ Status CaptureVariables(Graph* graph, const std::set skip_these_nodes) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); graph->AddEdge(replacement, edge->src_output(), edge->dst(), edge->dst_input()); + edges_to_remove.push_back(edge); + } + + for (auto edge : edges_to_remove) { graph->RemoveEdge(edge); } diff --git a/ngraph_bridge/ngraph_rewrite_for_tracking.cc b/ngraph_bridge/ngraph_rewrite_for_tracking.cc index add5871e9..4016b3829 100644 --- a/ngraph_bridge/ngraph_rewrite_for_tracking.cc +++ b/ngraph_bridge/ngraph_rewrite_for_tracking.cc @@ -32,7 +32,7 @@ namespace ngraph_bridge { // Status RewriteForTracking(Graph* graph, int graph_id) { std::vector replaced_nodes; - + std::vector edges_to_remove; for (auto node : graph->op_nodes()) { if (node->type_string() == "NGraphVariable") { NGRAPH_VLOG(4) << "Checking: " << node->name(); @@ -96,6 +96,10 @@ Status RewriteForTracking(Graph* graph, int graph_id) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); graph->AddEdge(edge->src(), edge->src_output(), replacement, edge->dst_input()); + edges_to_remove.push_back(edge); + } + + for (auto edge : edges_to_remove) { graph->RemoveEdge(edge); } @@ -107,6 +111,10 @@ Status RewriteForTracking(Graph* graph, int graph_id) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); graph->AddEdge(replacement, edge->src_output(), edge->dst(), edge->dst_input()); + edges_to_remove.push_back(edge); + } + + for (auto edge : edges_to_remove) { graph->RemoveEdge(edge); } From 9136f532e6ef787dcb0040d3a8aa0fe5c3846a65 Mon Sep 17 00:00:00 2001 From: Vishakha Agrawal Date: Wed, 31 Jul 2019 14:04:33 -0700 Subject: [PATCH 2/6] Applied code format. --- .../enable_variable_ops/ngraph_replace_op_utilities.cc | 4 ++-- ngraph_bridge/ngraph_rewrite_for_tracking.cc | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ngraph_bridge/enable_variable_ops/ngraph_replace_op_utilities.cc b/ngraph_bridge/enable_variable_ops/ngraph_replace_op_utilities.cc index 126149bfa..4f535ca5b 100644 --- a/ngraph_bridge/enable_variable_ops/ngraph_replace_op_utilities.cc +++ b/ngraph_bridge/enable_variable_ops/ngraph_replace_op_utilities.cc @@ -203,7 +203,7 @@ Status ReplaceInputControlEdges(Graph* graph, Node* node, Node* replacement) { edges_to_remove.push_back(edge); } for (auto edge : edges_to_remove) { - graph->RemoveEdge(edge); + graph->RemoveEdge(edge); } return Status::OK(); } @@ -224,7 +224,7 @@ Status ReplaceOutputEdges(Graph* graph, Node* node, Node* replacement) { edges_to_remove.push_back(edge); } for (auto edge : edges_to_remove) { - graph->RemoveEdge(edge); + graph->RemoveEdge(edge); } return Status::OK(); } diff --git a/ngraph_bridge/ngraph_rewrite_for_tracking.cc b/ngraph_bridge/ngraph_rewrite_for_tracking.cc index 4016b3829..7caae0b94 100644 --- a/ngraph_bridge/ngraph_rewrite_for_tracking.cc +++ b/ngraph_bridge/ngraph_rewrite_for_tracking.cc @@ -98,7 +98,7 @@ Status RewriteForTracking(Graph* graph, int graph_id) { edge->dst_input()); edges_to_remove.push_back(edge); } - + for (auto edge : edges_to_remove) { graph->RemoveEdge(edge); } From 45ec57fc103c49f014b3c65518f380dff4acd9c7 Mon Sep 17 00:00:00 2001 From: Vishakha Agrawal Date: Fri, 2 Aug 2019 10:41:14 -0700 Subject: [PATCH 3/6] Made changes to ngraph_rewrite. --- ngraph_bridge/ngraph_rewrite_for_tracking.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/ngraph_bridge/ngraph_rewrite_for_tracking.cc b/ngraph_bridge/ngraph_rewrite_for_tracking.cc index 7caae0b94..9736cc9d8 100644 --- a/ngraph_bridge/ngraph_rewrite_for_tracking.cc +++ b/ngraph_bridge/ngraph_rewrite_for_tracking.cc @@ -99,24 +99,22 @@ Status RewriteForTracking(Graph* graph, int graph_id) { edges_to_remove.push_back(edge); } - for (auto edge : edges_to_remove) { - graph->RemoveEdge(edge); - } std::vector edges; for (auto edge : node->out_edges()) { edges.push_back(edge); - } + } + for (auto edge : edges) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); graph->AddEdge(replacement, edge->src_output(), edge->dst(), edge->dst_input()); - edges_to_remove.push_back(edge); + graph->RemoveEdge(edge); } for (auto edge : edges_to_remove) { graph->RemoveEdge(edge); - } + } replaced_nodes.push_back(node); } else { From d94e6ff333bbc310737dc8957da60822a7512d60 Mon Sep 17 00:00:00 2001 From: Vishakha Agrawal Date: Fri, 2 Aug 2019 11:32:49 -0700 Subject: [PATCH 4/6] Apply code format --- ngraph_bridge/ngraph_rewrite_for_tracking.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/ngraph_bridge/ngraph_rewrite_for_tracking.cc b/ngraph_bridge/ngraph_rewrite_for_tracking.cc index 9736cc9d8..bacf1a276 100644 --- a/ngraph_bridge/ngraph_rewrite_for_tracking.cc +++ b/ngraph_bridge/ngraph_rewrite_for_tracking.cc @@ -99,22 +99,21 @@ Status RewriteForTracking(Graph* graph, int graph_id) { edges_to_remove.push_back(edge); } - std::vector edges; for (auto edge : node->out_edges()) { edges.push_back(edge); - } - + } + for (auto edge : edges) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); graph->AddEdge(replacement, edge->src_output(), edge->dst(), edge->dst_input()); - graph->RemoveEdge(edge); + graph->RemoveEdge(edge); } for (auto edge : edges_to_remove) { graph->RemoveEdge(edge); - } + } replaced_nodes.push_back(node); } else { From 4cc6ce36d5adc580e7a68816b8c6669deecac3a6 Mon Sep 17 00:00:00 2001 From: Shrestha Malik Date: Fri, 2 Aug 2019 16:27:02 -0700 Subject: [PATCH 5/6] Fixed bug --- ngraph_bridge/ngraph_capture_variables.cc | 8 +------- ngraph_bridge/ngraph_rewrite_for_tracking.cc | 16 +++++----------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/ngraph_bridge/ngraph_capture_variables.cc b/ngraph_bridge/ngraph_capture_variables.cc index ea4f678d7..45be4e086 100644 --- a/ngraph_bridge/ngraph_capture_variables.cc +++ b/ngraph_bridge/ngraph_capture_variables.cc @@ -100,15 +100,8 @@ Status CaptureVariables(Graph* graph, const std::set skip_these_nodes) { } // Though edges will be removed when we remove the node // we specifically remove the edges to be sure - for (auto edge : edges_to_remove) { - graph->RemoveEdge(edge); - } for (auto edge : node->out_edges()) { - edges.push_back(edge); - } - - for (auto edge : edges) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); graph->AddEdge(replacement, edge->src_output(), edge->dst(), edge->dst_input()); @@ -116,6 +109,7 @@ Status CaptureVariables(Graph* graph, const std::set skip_these_nodes) { } for (auto edge : edges_to_remove) { + NGRAPH_VLOG(4) << "Removing: " << edge->DebugString(); graph->RemoveEdge(edge); } diff --git a/ngraph_bridge/ngraph_rewrite_for_tracking.cc b/ngraph_bridge/ngraph_rewrite_for_tracking.cc index 7caae0b94..0932e381a 100644 --- a/ngraph_bridge/ngraph_rewrite_for_tracking.cc +++ b/ngraph_bridge/ngraph_rewrite_for_tracking.cc @@ -32,7 +32,7 @@ namespace ngraph_bridge { // Status RewriteForTracking(Graph* graph, int graph_id) { std::vector replaced_nodes; - std::vector edges_to_remove; + for (auto node : graph->op_nodes()) { if (node->type_string() == "NGraphVariable") { NGRAPH_VLOG(4) << "Checking: " << node->name(); @@ -90,31 +90,25 @@ Status RewriteForTracking(Graph* graph, int graph_id) { NGRAPH_VLOG(4) << "Replacing Node " << node->DebugString() << " with " << replacement->DebugString(); + std::vector edges_to_remove; // Though edges will be removed when we remove the node // we specifically remove the edges to be sure for (auto edge : node->in_edges()) { - NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); + NGRAPH_VLOG(4) << "Replacing: In Edge " << edge->DebugString(); graph->AddEdge(edge->src(), edge->src_output(), replacement, edge->dst_input()); edges_to_remove.push_back(edge); } - for (auto edge : edges_to_remove) { - graph->RemoveEdge(edge); - } - - std::vector edges; for (auto edge : node->out_edges()) { - edges.push_back(edge); - } - for (auto edge : edges) { - NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); + NGRAPH_VLOG(4) << "Replacing: OutEdge " << edge->DebugString(); graph->AddEdge(replacement, edge->src_output(), edge->dst(), edge->dst_input()); edges_to_remove.push_back(edge); } for (auto edge : edges_to_remove) { + NGRAPH_VLOG(4) << "Removing: Edges " << edge->DebugString(); graph->RemoveEdge(edge); } From ad0483074331761e326dca9ac00deb09cdf2fd5f Mon Sep 17 00:00:00 2001 From: Vishakha Agrawal Date: Wed, 21 Aug 2019 15:25:58 -0700 Subject: [PATCH 6/6] Placed the comments on right place --- ngraph_bridge/ngraph_capture_variables.cc | 4 ++-- ngraph_bridge/ngraph_rewrite_for_tracking.cc | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/ngraph_bridge/ngraph_capture_variables.cc b/ngraph_bridge/ngraph_capture_variables.cc index 45be4e086..915fa869a 100644 --- a/ngraph_bridge/ngraph_capture_variables.cc +++ b/ngraph_bridge/ngraph_capture_variables.cc @@ -98,8 +98,6 @@ Status CaptureVariables(Graph* graph, const std::set skip_these_nodes) { edge->dst_input()); edges_to_remove.push_back(edge); } - // Though edges will be removed when we remove the node - // we specifically remove the edges to be sure for (auto edge : node->out_edges()) { NGRAPH_VLOG(4) << "Replacing: " << edge->DebugString(); @@ -108,6 +106,8 @@ Status CaptureVariables(Graph* graph, const std::set skip_these_nodes) { edges_to_remove.push_back(edge); } + // Though edges will be removed when we remove the node + // we specifically remove the edges to be sure for (auto edge : edges_to_remove) { NGRAPH_VLOG(4) << "Removing: " << edge->DebugString(); graph->RemoveEdge(edge); diff --git a/ngraph_bridge/ngraph_rewrite_for_tracking.cc b/ngraph_bridge/ngraph_rewrite_for_tracking.cc index 0932e381a..c39013eee 100644 --- a/ngraph_bridge/ngraph_rewrite_for_tracking.cc +++ b/ngraph_bridge/ngraph_rewrite_for_tracking.cc @@ -91,8 +91,7 @@ Status RewriteForTracking(Graph* graph, int graph_id) { << replacement->DebugString(); std::vector edges_to_remove; - // Though edges will be removed when we remove the node - // we specifically remove the edges to be sure + for (auto edge : node->in_edges()) { NGRAPH_VLOG(4) << "Replacing: In Edge " << edge->DebugString(); graph->AddEdge(edge->src(), edge->src_output(), replacement, @@ -107,6 +106,8 @@ Status RewriteForTracking(Graph* graph, int graph_id) { edges_to_remove.push_back(edge); } + // Though edges will be removed when we remove the node + // we specifically remove the edges to be sure for (auto edge : edges_to_remove) { NGRAPH_VLOG(4) << "Removing: Edges " << edge->DebugString(); graph->RemoveEdge(edge);