Compiling multiple-outputs nodes [Pre-WIP] #90

Krovatkin · 2017-12-01T21:45:43Z

No description provided.

multiple output nodes up to SGCompiler::CompileSubgraph refining the initial impl after discussion's w/ Matt fixing compile errors formatting tests1 fixing comp errors after rebase results in compute

mbrookhart

Overall, yes, I think this is the right direction. There are some things to correct, as you mentioned, but I'm not seeing any major design flaws.

There's some code in create_layer_funcs and ParseNNVMGraph that deals with breaking "split", a multi-output mxnet op, into a number of smaller "split" ops. The emitter implementation of split is also based on breaking "split" down. We might be able to clean up the code if we can use this index idea instead, but that might also make the sgcompiler more complex.

mbrookhart · 2017-12-01T21:46:48Z

src/ngraph/ngraph_emitter.h

@@ -29,6 +29,7 @@ class Emitter {
  Emitter();
  // maps of ngraph operation generator functions
  OpEmitter ngraph_op_funcs_;
+  std::map<NodePtr, NgraphNodePtr> op_map_;


Why public?

that was an accident... Rebasing was very painful because I was modifying the same exact methods Louis moved out Graph

Ouch, yeah. Sorry.

mbrookhart · 2017-12-01T21:47:58Z

src/ngraph/ngraph_graph.cc

+    if (!in_vec(subgraph_nodes, n))
+      for (auto i : n->inputs_) {
+        if (in_vec(subgraph_nodes, i)) {
+          if (!in_vec(outNodes, i)) {  // TODO: use set


order matters, we're preserving a topological sort here, don't use set.

I think it could've worked for outNodes since we are in complete control of a mapping process and we use these indices only after we are done editing outNodes so the indices are relatively stable, but you are right I'd rather postpone testing this theory.

I see your point. Probably not worth fixing now, but good to keep in mind.

mbrookhart · 2017-12-01T21:50:54Z

src/ngraph/ngraph_graph.cc

+            //@#$ FIX
+            auto index = std::distance(
+                begin(outNodes),
+                std::find(begin(outNodes), end(outNodes),


This is interesting. I would have just used a counter...

seemed more concise back then... I'll try counter when I get stn working

I don't think it's a problem, no need to change if you don't want to.

mbrookhart · 2017-12-01T21:51:50Z

src/ngraph/ngraph_graph.cc

-                                  std::function<bool(NodePtr)> func) {
-  auto subgraph_nodes = SelectNodes(node, func);
+std::pair<std::vector<NodePtr>, std::vector<NodePtr>> FindSubgraph(
+    Graph &graph, NodePtr s, std::function<bool(NodePtr)> func) {


We switched everything from s to node a while back to make the names a little clearer

mbrookhart · 2017-12-01T21:55:08Z

src/ngraph/ngraph_graph.cc

-      auto in_tmpGraphInputs = [&tmpGraph](NodePtr n) {
+      auto in_tmpGraphInputs = [&tmpGraph](
+          NodePtr
+              n) {  //[nikolayk] 2nd time seeing this pattern why not use set?


topological sort order

mbrookhart · 2017-12-01T21:59:33Z

src/ngraph/ngraph_sgcompiler.cc

+            sub_graph->subgraph_outputs_.end(), results.begin(),
+            [this](const NodePtr X) { return this->op_map_[X]; });
+
+  auto result = std::make_shared<ngraph::op::Tuple>(results);


Isn't Core getting rid of Tuple? Will that cause issues here?

We will cross that bridge when we get there. As of now, we are free to use Tuple which in the near future will become XLATuple which can continue to use until we ready to make a switch to GetOutputElement

mbrookhart · 2017-12-01T22:00:56Z

src/ngraph/ngraph_compiler.cc

+  for (size_t i = 0; i < subgraph->subgraph_outputs_.size(); i++) {
+    nes.push_back(nnvm::NodeEntry{node, i, 0});
+  }
+


mbrookhart · 2017-12-01T22:01:35Z

src/ngraph/ngraph_compiler.cc

+
+      auto osg = std::dynamic_pointer_cast<Graph>(n);
+      auto index_of =
+          [osg](nnvm::NodeEntry ne) -> int /*note, this returns an int*/ {


:) I'm not sure the comment is needed

mbrookhart · 2017-12-01T22:02:57Z

src/ngraph/ngraph_compiler.cc

@@ -205,31 +218,41 @@ nnvm::Graph Compiler::Compile() {
      // register compiled subgraph with nnvm
      register_subgraph(sg);
      // create nnvm node
-      auto sg_node = CreateNNVMNode(sg);
+      auto sg_nodes = CreateNNVMNode(sg);


sg_node_entries would be a little clearer.

will change

mbrookhart · 2017-12-01T22:03:05Z

src/ngraph/ngraph_compiler.cc

+      }
+
+      // TODO: [nikolayk] why don't we just walk
+      // subgraph_outputs_[...]->orig_nodes_ and be done w/ the whole thing?


That would probably work.

Krovatkin · 2017-12-01T22:45:51Z

src/ngraph/ngraph_compiler.cc

@@ -205,31 +218,41 @@ nnvm::Graph Compiler::Compile() {
      // register compiled subgraph with nnvm
      register_subgraph(sg);
      // create nnvm node
-      auto sg_node = CreateNNVMNode(sg);
+      auto sg_nodes = CreateNNVMNode(sg);


will change

Krovatkin · 2017-12-01T22:46:53Z

src/ngraph/ngraph_emitter.h

@@ -29,6 +29,7 @@ class Emitter {
  Emitter();
  // maps of ngraph operation generator functions
  OpEmitter ngraph_op_funcs_;
+  std::map<NodePtr, NgraphNodePtr> op_map_;


that was an accident... Rebasing was very painful because I was modifying the same exact methods Louis moved out Graph

Krovatkin · 2017-12-01T22:52:06Z

src/ngraph/ngraph_graph.cc

+    if (!in_vec(subgraph_nodes, n))
+      for (auto i : n->inputs_) {
+        if (in_vec(subgraph_nodes, i)) {
+          if (!in_vec(outNodes, i)) {  // TODO: use set


I think it could've worked for outNodes since we are in complete control of a mapping process and we use these indices only after we are done editing outNodes so the indices are relatively stable, but you are right I'd rather postpone testing this theory.

Krovatkin · 2017-12-01T22:52:46Z

src/ngraph/ngraph_graph.cc

+            //@#$ FIX
+            auto index = std::distance(
+                begin(outNodes),
+                std::find(begin(outNodes), end(outNodes),


seemed more concise back then... I'll try counter when I get stn working

Krovatkin · 2017-12-01T22:53:00Z

src/ngraph/ngraph_graph.cc

-                                  std::function<bool(NodePtr)> func) {
-  auto subgraph_nodes = SelectNodes(node, func);
+std::pair<std::vector<NodePtr>, std::vector<NodePtr>> FindSubgraph(
+    Graph &graph, NodePtr s, std::function<bool(NodePtr)> func) {


Krovatkin · 2017-12-01T22:53:54Z

src/ngraph/ngraph_graph.cc

+              if (pos != end(outNodes)) {
+                outNodes.erase(pos);
+              }
+              i->subgraph_ = -1;  // are any of these in outNodes should we


remove the comment erasing works just fine

Krovatkin · 2017-12-01T22:55:00Z

src/ngraph/ngraph_graph.cc

@@ -235,7 +225,8 @@ void CollapseSubgraphs(Graph &graph) {
      if (node->subgraph_ == i) tmpGraph->AddNode(node);

    if (tmpGraph->nodes_.size() == 0) {
-      // if we don't find any nodes, assume we've run out of subgraphs
+      // if we don't find any nodes, assume we've run out of subgraphs TODO:
+      // [nikolayk] how did we get there?


we might want to turn this into a for-loop since we know the maximum number of sub graphs?

Krovatkin · 2017-12-01T22:56:10Z

src/ngraph/ngraph_sgcompiler.cc

@@ -49,39 +51,82 @@ void SGCompiler::CompileSubgraph(std::shared_ptr<Graph> sub_graph) {

  // map the inputs into a parameter list
  // TODO: std::transform?
-  ngraph::op::Parameters parameters;
+  ngraph::op::Parameters forward_parameters;


I need to check the comments behind this change; I didn't realize forward_parameters got renamed to parameters.

Krovatkin · 2017-12-01T22:57:07Z

src/ngraph/ngraph_sgcompiler.cc

+            sub_graph->subgraph_outputs_.end(), results.begin(),
+            [this](const NodePtr X) { return this->op_map_[X]; });
+
+  auto result = std::make_shared<ngraph::op::Tuple>(results);


We will cross that bridge when we get there. As of now, we are free to use Tuple which in the near future will become XLATuple which can continue to use until we ready to make a switch to GetOutputElement

Krovatkin · 2017-12-01T22:58:06Z

src/ngraph/ngraph_sgcompiler.cc

+
+  f = std::make_shared<ngraph::Function>(result, result->get_value_type(),
+                                         backward_parameters);
+  //////////////////////////////////////////////////////////////////////////////


this is the scariest part.

This is a combination of 2 commits.

ee9f9d3

multiple output nodes up to SGCompiler::CompileSubgraph refining the initial impl after discussion's w/ Matt fixing compile errors formatting tests1 fixing comp errors after rebase results in compute

mbrookhart suggested changes Dec 1, 2017

View reviewed changes

Krovatkin commented Dec 1, 2017

View reviewed changes

Krovatkin added 2 commits December 1, 2017 15:50

cleaning up the multi-output node logic in subgraph compiler

1dc9443

forgot the declaration of backward_parameters

179fe83

mbrookhart added the WIP label Dec 18, 2017

mbrookhart closed this Mar 15, 2018

mbrookhart deleted the krovatkin/multiple_outputs3 branch March 15, 2018 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compiling multiple-outputs nodes [Pre-WIP] #90

Compiling multiple-outputs nodes [Pre-WIP] #90

Krovatkin commented Dec 1, 2017

mbrookhart left a comment

mbrookhart Dec 1, 2017

Krovatkin Dec 1, 2017

mbrookhart Dec 1, 2017

mbrookhart Dec 1, 2017

Krovatkin Dec 1, 2017

mbrookhart Dec 1, 2017

mbrookhart Dec 1, 2017

Krovatkin Dec 1, 2017

mbrookhart Dec 1, 2017

mbrookhart Dec 1, 2017

Krovatkin Dec 1, 2017

mbrookhart Dec 1, 2017

mbrookhart Dec 1, 2017

Krovatkin Dec 1, 2017

mbrookhart Dec 1, 2017

mbrookhart Dec 1, 2017

mbrookhart Dec 1, 2017

Krovatkin Dec 1, 2017

mbrookhart Dec 1, 2017

Krovatkin Dec 1, 2017

Krovatkin Dec 1, 2017

Krovatkin Dec 1, 2017

Krovatkin Dec 1, 2017

Krovatkin Dec 1, 2017

Krovatkin Dec 1, 2017

Krovatkin Dec 1, 2017 •

edited

Loading

Krovatkin Dec 1, 2017

Krovatkin Dec 1, 2017

Krovatkin Dec 1, 2017

Compiling multiple-outputs nodes [Pre-WIP] #90

Compiling multiple-outputs nodes [Pre-WIP] #90

Conversation

Krovatkin commented Dec 1, 2017

mbrookhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Dec 1, 2017 •

edited

Loading