[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-14 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r210167394
 
 

 ##
 File path: src/operator/subgraph/default_subgraph_property.h
 ##
 @@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef MXNET_OPERATOR_SUBGRAPH_DEFAULT_SUBGRAPH_PROPERTY_H_
+#define MXNET_OPERATOR_SUBGRAPH_DEFAULT_SUBGRAPH_PROPERTY_H_
+
+#include 
+#include 
+#include "./common.h"
+#include "./subgraph_property.h"
+
+namespace mxnet {
+namespace op {
+
+/*
+ * This selects nodes for a subgraph that only contains operators
+ * in a given set and it visits nodes via both input and output links.
+ */
+class ContainOpSelector: public SubgraphSelector {
+ public:
+  explicit ContainOpSelector(const std::unordered_set& op_names)
+: op_names_(op_names) {}
+
+  virtual bool Select(const nnvm::Node &n) {
+return !n.is_variable() && op_names_.count(n.op()->name);
+  }
+
+  virtual bool SelectInput(const nnvm::Node &n, const nnvm::Node &new_node) {
+return !new_node.is_variable() && op_names_.count(new_node.op()->name);
+  }
+
+  virtual bool SelectOutput(const nnvm::Node &n, const nnvm::Node &new_node) {
+return !new_node.is_variable() && op_names_.count(new_node.op()->name);
+  }
+ private:
+  const std::unordered_set& op_names_;
+};
+
+/*
+ * This subgraph property finds a subgraph whose nodes have only operators
+ * within a set. The operators in the subgraph will be executed by 
_default_subgraph_op.
+ */
+class DefaultSubgraphProperty: public SubgraphProperty {
+ public:
+  static SubgraphPropertyPtr Create() { return 
std::make_shared(); }
+  virtual nnvm::NodePtr CreateSubgraphNode(const nnvm::Symbol &sym,
+   const int subgraph_id = 0) const {
+nnvm::NodePtr n = nnvm::Node::Create();
+n->attrs.op = Op::Get("_default_subgraph_op");
+n->attrs.name = "_default_subgraph_op" + std::to_string(subgraph_id);
+n->attrs.subgraphs.push_back(std::make_shared(sym));
 
 Review comment:
   `n` is a newly created node and its `attrs.subgraphs` is empty before 
`push_back` is called. It's actually up to you to decide which index is for the 
`sym` in your specific subgraph property. Just make sure you get the correct 
subgraph in your subgraph operator implementation.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-14 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r210168425
 
 

 ##
 File path: src/operator/subgraph/default_subgraph_property.h
 ##
 @@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef MXNET_OPERATOR_SUBGRAPH_DEFAULT_SUBGRAPH_PROPERTY_H_
+#define MXNET_OPERATOR_SUBGRAPH_DEFAULT_SUBGRAPH_PROPERTY_H_
+
+#include 
+#include 
+#include "./common.h"
+#include "./subgraph_property.h"
+
+namespace mxnet {
+namespace op {
+
+/*
+ * This selects nodes for a subgraph that only contains operators
+ * in a given set and it visits nodes via both input and output links.
+ */
+class ContainOpSelector: public SubgraphSelector {
+ public:
+  explicit ContainOpSelector(const std::unordered_set& op_names)
+: op_names_(op_names) {}
+
+  virtual bool Select(const nnvm::Node &n) {
+return !n.is_variable() && op_names_.count(n.op()->name);
+  }
+
+  virtual bool SelectInput(const nnvm::Node &n, const nnvm::Node &new_node) {
+return !new_node.is_variable() && op_names_.count(new_node.op()->name);
+  }
+
+  virtual bool SelectOutput(const nnvm::Node &n, const nnvm::Node &new_node) {
+return !new_node.is_variable() && op_names_.count(new_node.op()->name);
+  }
+ private:
+  const std::unordered_set& op_names_;
+};
+
+/*
+ * This subgraph property finds a subgraph whose nodes have only operators
+ * within a set. The operators in the subgraph will be executed by 
_default_subgraph_op.
+ */
+class DefaultSubgraphProperty: public SubgraphProperty {
+ public:
+  static SubgraphPropertyPtr Create() { return 
std::make_shared(); }
+  virtual nnvm::NodePtr CreateSubgraphNode(const nnvm::Symbol &sym,
+   const int subgraph_id = 0) const {
+nnvm::NodePtr n = nnvm::Node::Create();
+n->attrs.op = Op::Get("_default_subgraph_op");
+n->attrs.name = "_default_subgraph_op" + std::to_string(subgraph_id);
+n->attrs.subgraphs.push_back(std::make_shared(sym));
 
 Review comment:
   As far as I know, there is no fixed convention of doing this. @zheng-da 
   I don't think you need to worry about this if you can ensure the consistency 
between your specific subgraph property and subgraph operator.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-14 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r210170495
 
 

 ##
 File path: src/operator/subgraph/default_subgraph_property.h
 ##
 @@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef MXNET_OPERATOR_SUBGRAPH_DEFAULT_SUBGRAPH_PROPERTY_H_
+#define MXNET_OPERATOR_SUBGRAPH_DEFAULT_SUBGRAPH_PROPERTY_H_
+
+#include 
+#include 
+#include "./common.h"
+#include "./subgraph_property.h"
+
+namespace mxnet {
+namespace op {
+
+/*
+ * This selects nodes for a subgraph that only contains operators
+ * in a given set and it visits nodes via both input and output links.
+ */
+class ContainOpSelector: public SubgraphSelector {
+ public:
+  explicit ContainOpSelector(const std::unordered_set& op_names)
+: op_names_(op_names) {}
+
+  virtual bool Select(const nnvm::Node &n) {
+return !n.is_variable() && op_names_.count(n.op()->name);
+  }
+
+  virtual bool SelectInput(const nnvm::Node &n, const nnvm::Node &new_node) {
+return !new_node.is_variable() && op_names_.count(new_node.op()->name);
+  }
+
+  virtual bool SelectOutput(const nnvm::Node &n, const nnvm::Node &new_node) {
+return !new_node.is_variable() && op_names_.count(new_node.op()->name);
+  }
+ private:
+  const std::unordered_set& op_names_;
+};
+
+/*
+ * This subgraph property finds a subgraph whose nodes have only operators
+ * within a set. The operators in the subgraph will be executed by 
_default_subgraph_op.
+ */
+class DefaultSubgraphProperty: public SubgraphProperty {
+ public:
+  static SubgraphPropertyPtr Create() { return 
std::make_shared(); }
+  virtual nnvm::NodePtr CreateSubgraphNode(const nnvm::Symbol &sym,
+   const int subgraph_id = 0) const {
+nnvm::NodePtr n = nnvm::Node::Create();
+n->attrs.op = Op::Get("_default_subgraph_op");
+n->attrs.name = "_default_subgraph_op" + std::to_string(subgraph_id);
+n->attrs.subgraphs.push_back(std::make_shared(sym));
 
 Review comment:
   One thing you can do to enforce the consistency is defining some enums as 
indices of forward graph, backward graph, fused graph, etc in 
`attrs.subgraphs`, and always use those enums to get the desired subgraphs. It 
also increases the code readability.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-21 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r211707508
 
 

 ##
 File path: src/operator/subgraph/subgraph_property.h
 ##
 @@ -0,0 +1,132 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef MXNET_OPERATOR_SUBGRAPH_SUBGRAPH_PROPERTY_H_
+#define MXNET_OPERATOR_SUBGRAPH_SUBGRAPH_PROPERTY_H_
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+namespace mxnet {
+namespace op {
+
+/*
+ * This provides criteria for selecting nodes in a subgraph.
+ * When a node is passed to this object, the selection criteria may be changed.
+ * We can also specify what links we should use when traversing the neighbor
+ * nodes.
+ */
+class SubgraphSelector {
+ public:
+  virtual ~SubgraphSelector() {}
+  // Determine if the node should be selected for a subgraph.
+  virtual bool Select(const nnvm::Node &n) = 0;
+  // Determine if the input node should be selected for a subgraph.
+  virtual bool SelectInput(const nnvm::Node &n, const nnvm::Node &new_node) = 
0;
+  // Determine if the output node should be selected for a subgraph.
+  virtual bool SelectOutput(const nnvm::Node &n, const nnvm::Node &new_node) = 
0;
+  // Post processes pre-selected subgraph nodes. Return a list of nodes that
+  // users want to keep in subgraph(s).
+  virtual std::vector Filter(const std::vector& 
candidates) {
+return candidates;
+  }
+};
+
+using SubgraphSelectorPtr = std::shared_ptr;
+
+/*!
+ * \brief This provides a set of properties for partitioning a graph into 
subgraphs,
+ * reconstructing a new graph from the subgraphs and creating a subgraph
+ * operator to execute the subgraph.
+ */
+class SubgraphProperty {
+ public:
+  // the criteria of selecting the subgraph nodes.
+  virtual SubgraphSelectorPtr CreateSubgraphSelector() const = 0;
+  // create an nnvm node for a given subgraph. Here users can customize how to
+  // execute the operators in the subgraph.
+  virtual nnvm::NodePtr CreateSubgraphNode(const nnvm::Symbol &s,
+   const int subgraph_id = 0) const = 
0;
+  // set an attr with name in the attr map
+  template
+  SubgraphProperty& SetAttr(const std::string& name, const T& value) {
+attrs_[name] = std::make_shared(value);
+return *this;
+  }
+  // get the attr with the name
+  template
+  const T& GetAttr(const std::string& name) const {
+auto it = attrs_.find(name);
+CHECK(it != attrs_.end()) << "Cannot find attribute " << name << " in 
SubgraphProperty";
+return nnvm::get(*it->second);
+  }
+ protected:
+  std::unordered_map> attrs_;
+};
+
+using SubgraphPropertyPtr = std::shared_ptr;
+
+class SubgraphPropertyRegistry {
+ public:
+  typedef SubgraphPropertyPtr (*SubgraphPropertyCreateFn)(void);
+  static SubgraphPropertyRegistry* Get() {
+static SubgraphPropertyRegistry inst;
+return &inst;
+  }
+
+  SubgraphPropertyPtr CreateSubgraphProperty(const std::string& name) {
+auto it = prop_fn_map_.find(name);
+CHECK(it != prop_fn_map_.end()) << "SubgraphProperty " << name
+<< " is not found in 
SubgraphPropertyRegistry";
+return it->second();
+  }
+
+  SubgraphPropertyCreateFn __REGISTER__(const std::string& name, 
SubgraphPropertyCreateFn fn) {
+CHECK_EQ(prop_fn_map_.count(name), 0U) << "Subgraph property " << name
+   << " has been registered";
+prop_fn_map_[name] = fn;
+return prop_fn_map_[name];
+  }
+
+ private:
+  SubgraphPropertyRegistry() = default;
+  SubgraphPropertyRegistry(const SubgraphPropertyRegistry&) = delete;
+  SubgraphPropertyRegistry(SubgraphPropertyRegistry&&) = delete;
+  SubgraphPropertyRegistry& operator=(const SubgraphPropertyRegistry&) = 
delete;
+  std::unordered_map prop_fn_map_;
+};
+
+// This op name set is for setting the names of operators that should be 
grouped into
+// subgraphs. In practice, every backend accelerator should have a predefined 
name set.
+// This set is only used for the testing purpose.
+// key: property name, value: op name set
+typedef dmlc::ThreadLocalStore>>
+  Subgrap

[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-21 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r211721302
 
 

 ##
 File path: src/operator/subgraph/subgraph_property.h
 ##
 @@ -0,0 +1,132 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef MXNET_OPERATOR_SUBGRAPH_SUBGRAPH_PROPERTY_H_
+#define MXNET_OPERATOR_SUBGRAPH_SUBGRAPH_PROPERTY_H_
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+namespace mxnet {
+namespace op {
+
+/*
+ * This provides criteria for selecting nodes in a subgraph.
+ * When a node is passed to this object, the selection criteria may be changed.
+ * We can also specify what links we should use when traversing the neighbor
+ * nodes.
+ */
+class SubgraphSelector {
+ public:
+  virtual ~SubgraphSelector() {}
+  // Determine if the node should be selected for a subgraph.
+  virtual bool Select(const nnvm::Node &n) = 0;
+  // Determine if the input node should be selected for a subgraph.
+  virtual bool SelectInput(const nnvm::Node &n, const nnvm::Node &new_node) = 
0;
+  // Determine if the output node should be selected for a subgraph.
+  virtual bool SelectOutput(const nnvm::Node &n, const nnvm::Node &new_node) = 
0;
+  // Post processes pre-selected subgraph nodes. Return a list of nodes that
+  // users want to keep in subgraph(s).
+  virtual std::vector Filter(const std::vector& 
candidates) {
+return candidates;
+  }
+};
+
+using SubgraphSelectorPtr = std::shared_ptr;
+
+/*!
+ * \brief This provides a set of properties for partitioning a graph into 
subgraphs,
+ * reconstructing a new graph from the subgraphs and creating a subgraph
+ * operator to execute the subgraph.
+ */
+class SubgraphProperty {
+ public:
+  // the criteria of selecting the subgraph nodes.
+  virtual SubgraphSelectorPtr CreateSubgraphSelector() const = 0;
+  // create an nnvm node for a given subgraph. Here users can customize how to
+  // execute the operators in the subgraph.
+  virtual nnvm::NodePtr CreateSubgraphNode(const nnvm::Symbol &s,
+   const int subgraph_id = 0) const = 
0;
+  // set an attr with name in the attr map
+  template
+  SubgraphProperty& SetAttr(const std::string& name, const T& value) {
+attrs_[name] = std::make_shared(value);
+return *this;
+  }
+  // get the attr with the name
+  template
+  const T& GetAttr(const std::string& name) const {
+auto it = attrs_.find(name);
+CHECK(it != attrs_.end()) << "Cannot find attribute " << name << " in 
SubgraphProperty";
+return nnvm::get(*it->second);
+  }
+ protected:
+  std::unordered_map> attrs_;
+};
+
+using SubgraphPropertyPtr = std::shared_ptr;
+
+class SubgraphPropertyRegistry {
+ public:
+  typedef SubgraphPropertyPtr (*SubgraphPropertyCreateFn)(void);
+  static SubgraphPropertyRegistry* Get() {
+static SubgraphPropertyRegistry inst;
+return &inst;
+  }
+
+  SubgraphPropertyPtr CreateSubgraphProperty(const std::string& name) {
+auto it = prop_fn_map_.find(name);
+CHECK(it != prop_fn_map_.end()) << "SubgraphProperty " << name
+<< " is not found in 
SubgraphPropertyRegistry";
+return it->second();
+  }
+
+  SubgraphPropertyCreateFn __REGISTER__(const std::string& name, 
SubgraphPropertyCreateFn fn) {
+CHECK_EQ(prop_fn_map_.count(name), 0U) << "Subgraph property " << name
+   << " has been registered";
+prop_fn_map_[name] = fn;
+return prop_fn_map_[name];
+  }
+
+ private:
+  SubgraphPropertyRegistry() = default;
+  SubgraphPropertyRegistry(const SubgraphPropertyRegistry&) = delete;
+  SubgraphPropertyRegistry(SubgraphPropertyRegistry&&) = delete;
+  SubgraphPropertyRegistry& operator=(const SubgraphPropertyRegistry&) = 
delete;
+  std::unordered_map prop_fn_map_;
+};
+
+// This op name set is for setting the names of operators that should be 
grouped into
+// subgraphs. In practice, every backend accelerator should have a predefined 
name set.
+// This set is only used for the testing purpose.
+// key: property name, value: op name set
+typedef dmlc::ThreadLocalStore>>
+  Subgrap

[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-22 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r212172201
 
 

 ##
 File path: src/imperative/imperative_utils.h
 ##
 @@ -436,7 +436,8 @@ inline void PushFComputeEx(const FComputeEx& fn,
   }
 };
 
-  if (exec_type == ExecType::kCrossDeviceCopy) {
+  if (exec_type == ExecType::kCrossDeviceCopy
+  || exec_type == ExecType::kSubgraphExec) {
 
 Review comment:
   I think this line is added by you. Nevertheless, isn't 
`_default_subgraph_op` a stateful operator? This should be needed to make sure 
this operator executes in the main thread in `CachedOp`.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-22 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r212172459
 
 

 ##
 File path: src/operator/subgraph/default_subgraph_op.cc
 ##
 @@ -0,0 +1,113 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+*   http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied.  See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+
+#include 
+#include "./common.h"
+#include "../../imperative/imperative_utils.h"
+#include "../../imperative/cached_op.h"
+
+namespace mxnet {
+namespace op {
+
+#define DEBUG_SUBGRAPH 0
+
+class DefaultSubgraphOperator {
+ public:
+  explicit DefaultSubgraphOperator(const Symbol& sym) : subgraph_sym_(sym) {
+subgraph_exec_.reset(new CachedOp(sym, {{"static_alloc", "true"},
+{"static_shape", "true"}}));
+  }
+
+  void Forward(const OpContext& ctx,
+   const std::vector& inputs,
+   const std::vector& req,
+   const std::vector& outputs);
+  void Backward(const OpContext& ctx,
+const std::vector& inputs,
+const std::vector& req,
+const std::vector& outputs) {
+LOG(FATAL) << "Not implemented";
+  }
+
+ private:
+  nnvm::Symbol subgraph_sym_;
+  CachedOpPtr subgraph_exec_;
+};
+
+void DefaultSubgraphOperator::Forward(const OpContext& ctx,
+  const std::vector& inputs,
+  const std::vector& req,
+  const std::vector& outputs) {
+  std::vector tmp_inputs = inputs;
+  std::vector input_ptrs;
+  input_ptrs.reserve(inputs.size());
+  for (auto& nd : tmp_inputs) {
+input_ptrs.push_back(&nd);
+  }
+  std::vector tmp_outputs = outputs;
+  std::vector output_ptrs;
+  for (auto& nd : tmp_outputs) {
+output_ptrs.push_back(&nd);
+  }
+#if DEBUG_SUBGRAPH
+  for (size_t i = 0; i < inputs.size(); ++i) {
+LOG(INFO) << "inputs[" << i << "].version = " << inputs[i].version();
+  }
+  for (size_t i = 0; i < outputs.size(); ++i) {
+LOG(INFO) << "outputs[" << i << "].version = " << outputs[i].version();
+  }
+#endif
 
 Review comment:
   As discussed offline, we are going to keep the debugging code for a while.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-22 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r212175821
 
 

 ##
 File path: src/ndarray/ndarray.cc
 ##
 @@ -39,6 +39,7 @@
 #include "../operator/tensor/matrix_op-inl.h"
 #include "../operator/tensor/init_op.h"
 #include "../operator/nn/mkldnn/mkldnn_base-inl.h"
+#include "../engine/engine_impl.h"
 
 Review comment:
   Removed.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-23 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r212363518
 
 

 ##
 File path: src/operator/subgraph/default_subgraph_op.cc
 ##
 @@ -0,0 +1,113 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements.  See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership.  The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License.  You may obtain a copy of the License at
+*
+*   http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing,
+* software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+* KIND, either express or implied.  See the License for the
+* specific language governing permissions and limitations
+* under the License.
+*/
+
+#include 
+#include "./common.h"
+#include "../../imperative/imperative_utils.h"
+#include "../../imperative/cached_op.h"
+
+namespace mxnet {
+namespace op {
+
+#define DEBUG_SUBGRAPH 0
+
+class DefaultSubgraphOperator {
+ public:
+  explicit DefaultSubgraphOperator(const Symbol& sym) : subgraph_sym_(sym) {
+subgraph_exec_.reset(new CachedOp(sym, {{"static_alloc", "true"},
+{"static_shape", "true"}}));
+  }
+
+  void Forward(const OpContext& ctx,
+   const std::vector& inputs,
+   const std::vector& req,
+   const std::vector& outputs);
+  void Backward(const OpContext& ctx,
+const std::vector& inputs,
+const std::vector& req,
+const std::vector& outputs) {
+LOG(FATAL) << "Not implemented";
+  }
+
+ private:
+  nnvm::Symbol subgraph_sym_;
+  CachedOpPtr subgraph_exec_;
+};
+
+void DefaultSubgraphOperator::Forward(const OpContext& ctx,
+  const std::vector& inputs,
+  const std::vector& req,
+  const std::vector& outputs) {
+  std::vector tmp_inputs = inputs;
+  std::vector input_ptrs;
+  input_ptrs.reserve(inputs.size());
+  for (auto& nd : tmp_inputs) {
+input_ptrs.push_back(&nd);
+  }
+  std::vector tmp_outputs = outputs;
+  std::vector output_ptrs;
+  for (auto& nd : tmp_outputs) {
+output_ptrs.push_back(&nd);
+  }
+#if DEBUG_SUBGRAPH
+  for (size_t i = 0; i < inputs.size(); ++i) {
+LOG(INFO) << "inputs[" << i << "].version = " << inputs[i].version();
+  }
+  for (size_t i = 0; i < outputs.size(); ++i) {
+LOG(INFO) << "outputs[" << i << "].version = " << outputs[i].version();
+  }
+#endif
 
 Review comment:
   Ah, I see. Removed now.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-24 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r212713464
 
 

 ##
 File path: src/executor/graph_executor.cc
 ##
 @@ -1428,6 +1430,146 @@ GraphExecutor::CachedSegOpr 
GraphExecutor::CreateCachedSegOpr(size_t topo_start,
 iter->c_str());
   return ret;
 }
+
+// Infer shapes, dtypes, stypes, contexts for the forward graph
+static nnvm::Graph InferForwardAttrs(nnvm::Graph g,
+ nnvm::ShapeVector arg_shapes,
+ nnvm::DTypeVector arg_dtypes,
+ StorageTypeVector arg_stypes,
+ const Context& default_ctx,
+ const std::map& 
ctx_map,
+ const std::vector& in_arg_ctxes,
+ const std::vector& 
aux_state_ctxes) {
+  const auto& indexed_graph = g.indexed_graph();
+  const auto num_forward_inputs = indexed_graph.input_nodes().size();
+  g = AssignContext(g, default_ctx, ctx_map, in_arg_ctxes, {},
+   aux_state_ctxes, {}, num_forward_inputs, g.outputs.size());
+  g = InferShape(std::move(g), std::move(arg_shapes), "__shape__");
+  if (g.GetAttr("shape_num_unknown_nodes") != 0U) {
+HandleInferShapeError(num_forward_inputs, indexed_graph,
+  g.GetAttr("shape"));
+  }
+  g = InferType(std::move(g), std::move(arg_dtypes), "__dtype__");
+  if (g.GetAttr("dtype_num_unknown_nodes") != 0U) {
+HandleInferTypeError(num_forward_inputs, indexed_graph,
+ g.GetAttr("dtype"));
+  }
+  g = InferStorageType(std::move(g), std::move(arg_stypes), 
"__storage_type__");
+  if (g.GetAttr("storage_type_num_unknown_nodes") != 0U) {
+HandleInferStorageTypeError(num_forward_inputs, indexed_graph,
+g.GetAttr("storage_type"));
+  }
+  return g;
+}
+
+// Given input attr arrays, partition the graph using the backend name equal 
to prop_name.
+// This is a common function for bind and simple_bind flows.
+static nnvm::Symbol PartitionGraph(const nnvm::Symbol& src,
+   const std::string& prop_name,
+   const nnvm::ShapeVector& arg_shapes,
+   const nnvm::DTypeVector& arg_dtypes,
+   const StorageTypeVector& arg_stypes,
+   const Context& default_ctx,
+   const std::map& 
ctx_map,
+   const std::vector& in_arg_ctxes,
+   const std::vector& 
aux_state_ctxes) {
+  auto subgraph_prop = 
op::SubgraphPropertyRegistry::Get()->CreateSubgraphProperty(prop_name);
 
 Review comment:
   There is a check inside `CreateSubgraphProperty`.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-24 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r212716344
 
 

 ##
 File path: src/engine/naive_engine.cc
 ##
 @@ -165,14 +178,22 @@ class NaiveEngine final : public Engine {
 }
 CHECK(this->req_completed_)
 << "NaiveEngine only support synchronize Push so far";
+// increment var version
+for (auto var : mutable_vars) {
 
 Review comment:
   Good catch. Should move this before `exec_fun` runs.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-24 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r212716658
 
 

 ##
 File path: src/executor/graph_executor.cc
 ##
 @@ -1428,6 +1430,146 @@ GraphExecutor::CachedSegOpr 
GraphExecutor::CreateCachedSegOpr(size_t topo_start,
 iter->c_str());
   return ret;
 }
+
+// Infer shapes, dtypes, stypes, contexts for the forward graph
+static nnvm::Graph InferForwardAttrs(nnvm::Graph g,
+ nnvm::ShapeVector arg_shapes,
+ nnvm::DTypeVector arg_dtypes,
+ StorageTypeVector arg_stypes,
+ const Context& default_ctx,
+ const std::map& 
ctx_map,
+ const std::vector& in_arg_ctxes,
+ const std::vector& 
aux_state_ctxes) {
+  const auto& indexed_graph = g.indexed_graph();
+  const auto num_forward_inputs = indexed_graph.input_nodes().size();
+  g = AssignContext(g, default_ctx, ctx_map, in_arg_ctxes, {},
+   aux_state_ctxes, {}, num_forward_inputs, g.outputs.size());
+  g = InferShape(std::move(g), std::move(arg_shapes), "__shape__");
+  if (g.GetAttr("shape_num_unknown_nodes") != 0U) {
+HandleInferShapeError(num_forward_inputs, indexed_graph,
+  g.GetAttr("shape"));
+  }
+  g = InferType(std::move(g), std::move(arg_dtypes), "__dtype__");
+  if (g.GetAttr("dtype_num_unknown_nodes") != 0U) {
+HandleInferTypeError(num_forward_inputs, indexed_graph,
+ g.GetAttr("dtype"));
+  }
+  g = InferStorageType(std::move(g), std::move(arg_stypes), 
"__storage_type__");
+  if (g.GetAttr("storage_type_num_unknown_nodes") != 0U) {
+HandleInferStorageTypeError(num_forward_inputs, indexed_graph,
+g.GetAttr("storage_type"));
+  }
+  return g;
+}
+
+// Given input attr arrays, partition the graph using the backend name equal 
to prop_name.
+// This is a common function for bind and simple_bind flows.
+static nnvm::Symbol PartitionGraph(const nnvm::Symbol& src,
+   const std::string& prop_name,
+   const nnvm::ShapeVector& arg_shapes,
+   const nnvm::DTypeVector& arg_dtypes,
+   const StorageTypeVector& arg_stypes,
+   const Context& default_ctx,
+   const std::map& 
ctx_map,
+   const std::vector& in_arg_ctxes,
+   const std::vector& 
aux_state_ctxes) {
+  auto subgraph_prop = 
op::SubgraphPropertyRegistry::Get()->CreateSubgraphProperty(prop_name);
+  nnvm::Symbol ret = src.Copy();
+  nnvm::Graph g;
+  g.outputs = ret.outputs;
+  g = InferForwardAttrs(g, arg_shapes, arg_dtypes, arg_stypes, default_ctx,
+ctx_map, in_arg_ctxes, aux_state_ctxes);
+  subgraph_prop->SetAttr("graph", g);
+  auto it = op::SubgraphPropertyOpNameSet::Get()->find(prop_name);
+  // assign a op name set to the subgraph property if it has been provided by 
users
+  if (it != op::SubgraphPropertyOpNameSet::Get()->end()) {
+LOG(INFO) << "SubgraphPropertyOpNameSet for subgraph property " << 
prop_name
+  << " has been assigned a value. Please make sure it is 
initialized"
+ " only for the testing purpose.";
+subgraph_prop->SetAttr("op_names", it->second);
+  }
+  g.attrs["subgraph_property"] = 
std::make_shared(std::move(subgraph_prop));
+  g = ApplyPass(std::move(g), "PartitionGraph");
+  ret.outputs = g.outputs;
+  return ret;
+}
+
+// Given input attr dicts, partition the graph using the backend name equal to 
prop_name.
+// This is for simple_bind flow.
+static nnvm::Symbol PartitionGraph(const nnvm::Symbol& src,
+   const std::string& prop_name,
+   const std::unordered_map& arg_shape_map,
+   const std::unordered_map& 
arg_dtype_map,
+   const std::unordered_map& 
arg_stype_map,
+   const Context& default_ctx,
+   const std::map& 
ctx_map,
+   const std::vector& in_arg_ctxes,
+   const std::vector& 
aux_state_ctxes) {
+  const std::vector input_names = 
src.ListInputNames(Symbol::kAll);
+  nnvm::ShapeVector arg_shapes(input_names.size(), TShape());
+  nnvm::DTypeVector arg_dtypes(input_names.size(), -1);
+  StorageTypeVector arg_stypes(input_names.size(), kUndefinedStorage);
+  for (size_t i = 0; i < input_names.size(); ++i) {
+auto it1 = arg_shape_map.find(input_names[i]);
+if (arg_shape_map.end() != it1) 

[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-24 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r212760058
 
 

 ##
 File path: src/operator/subgraph/default_subgraph_property.cc
 ##
 @@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include 
+#include 
+#include "./common.h"
+#include "./subgraph_property.h"
+
+namespace mxnet {
+namespace op {
+
+/*
+ * This selects nodes for a subgraph that only contains operators
+ * in a given set and it visits nodes via both input and output links.
+ */
+class ContainOpSelector: public SubgraphSelector {
+ public:
+  explicit ContainOpSelector(const std::unordered_set& op_names)
+: op_names_(op_names) {}
+
+  virtual bool Select(const nnvm::Node &n) {
+return !n.is_variable() && op_names_.count(n.op()->name);
+  }
+
+  virtual bool SelectInput(const nnvm::Node &n, const nnvm::Node &new_node) {
+return !new_node.is_variable() && op_names_.count(new_node.op()->name);
+  }
+
+  virtual bool SelectOutput(const nnvm::Node &n, const nnvm::Node &new_node) {
+return !new_node.is_variable() && op_names_.count(new_node.op()->name);
+  }
+ private:
+  const std::unordered_set& op_names_;
+};
+
+/*
+ * This subgraph property finds a subgraph whose nodes have only operators
+ * within a set. The operators in the subgraph will be executed by 
_default_subgraph_op.
+ */
+class DefaultSubgraphProperty: public SubgraphProperty {
+ public:
+  static SubgraphPropertyPtr Create() { return 
std::make_shared(); }
+  virtual nnvm::NodePtr CreateSubgraphNode(const nnvm::Symbol &sym,
+   const int subgraph_id = 0) const {
+nnvm::NodePtr n = nnvm::Node::Create();
+n->attrs.op = Op::Get("_default_subgraph_op");
+n->attrs.name = "_default_subgraph_op" + std::to_string(subgraph_id);
+n->attrs.subgraphs.push_back(std::make_shared(sym));
+return n;
+  }
+  virtual SubgraphSelectorPtr CreateSubgraphSelector() const {
+return std::make_shared(
+this->GetAttr>("op_names"));
 
 Review comment:
   It's not meant for users to use and is there only for the flexible testing 
purpose. Furthermore, template method cannot be virtual. But I still cannot 
understand your use case. How different could SetAttr and GetAttr be in a 
subclass than the base?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-28 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r213502485
 
 

 ##
 File path: src/executor/graph_executor.cc
 ##
 @@ -42,6 +43,7 @@ using namespace mxnet::common;
 GraphExecutor::GraphExecutor() {
   log_verbose_ = dmlc::GetEnv("MXNET_EXEC_VERBOSE_LOGGING", false);
   need_grad_ = false;
+  subgraph_property_ = dmlc::GetEnv("MXNET_SUBGRAPH_BACKEND", std::string());
 
 Review comment:
   Right now, there is no real accelerator integrated with MXNet. Intel team is 
going to submit their work on MKLDNN using this API set. We can add this evn 
var along with their PR.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-28 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r213502853
 
 

 ##
 File path: src/operator/subgraph/common.h
 ##
 @@ -0,0 +1,237 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef MXNET_OPERATOR_SUBGRAPH_COMMON_H_
+#define MXNET_OPERATOR_SUBGRAPH_COMMON_H_
+
+#include 
+#include 
+#include 
+#include "../elemwise_op_common.h"
+#include "../../executor/exec_pass.h"
+
+namespace mxnet {
+namespace op {
+
+inline uint32_t DefaultSubgraphOpNumInputs(const nnvm::NodeAttrs& attrs) {
+  const nnvm::Symbol& sym = *attrs.subgraphs[0];
+  return sym.ListInputNames(nnvm::Symbol::kAll).size();
+}
+
+inline uint32_t DefaultSubgraphOpNumOutputs(const nnvm::NodeAttrs& attrs) {
+  const nnvm::Symbol& sym = *attrs.subgraphs[0];
+  return sym.ListOutputNames().size();
+}
+
+inline std::vector DefaultSubgraphOpListInputs(const 
nnvm::NodeAttrs& attrs) {
+  const nnvm::Symbol& sym = *attrs.subgraphs[0];
+  return sym.ListInputNames(nnvm::Symbol::kAll);
+}
+
+inline std::vector DefaultSubgraphOpListOutputs(const 
nnvm::NodeAttrs& attrs) {
+  const nnvm::Symbol& sym = *attrs.subgraphs[0];
+  return sym.ListOutputNames();
+}
+
+inline bool DefaultSubgraphOpShape(const nnvm::NodeAttrs& attrs,
+   std::vector *in_shapes,
+   std::vector *out_shapes) {
+  using namespace exec;
+  const nnvm::Symbol& subgraph_sym = *attrs.subgraphs[0];
+  nnvm::Graph g;
+  g.outputs = subgraph_sym.outputs;
+  const auto& idx_g = g.indexed_graph();
+  CHECK_EQ(idx_g.input_nodes().size(), in_shapes->size());
+  CHECK_EQ(idx_g.outputs().size(), out_shapes->size());
+
+  // Put the input and output shapes to the shape vector.
+  nnvm::ShapeVector shapes(idx_g.num_node_entries());
+  const auto &input_nids = idx_g.input_nodes();
+  CHECK_EQ(input_nids.size(), in_shapes->size());
+  for (size_t i = 0; i < in_shapes->size(); i++) {
+auto eid = idx_g.entry_id(input_nids[i], 0);
+shapes[eid] = in_shapes->at(i);
+  }
+  CHECK_EQ(g.outputs.size(), out_shapes->size());
+  for (size_t i = 0; i < out_shapes->size(); i++) {
+auto eid = idx_g.entry_id(g.outputs[i]);
+shapes[eid] = out_shapes->at(i);
+  }
+
+  // Infer shape of the graph.
+  g.attrs["shape"] = std::make_shared(std::move(shapes));
+  g = exec::InferShape(std::move(g));
+
+  // Copy the inferred shape back to the input shapes and the output shapes.
+  shapes = g.GetAttr("shape");
+  // assign to in_shapes
+  for (size_t i = 0; i < in_shapes->size(); ++i) {
+const auto eid = idx_g.entry_id(input_nids[i], 0);
+SHAPE_ASSIGN_CHECK(*in_shapes, i, shapes[eid]);
+  }
+  // assign to out_shapes
+  for (size_t i = 0; i < g.outputs.size(); ++i) {
+const auto eid = idx_g.entry_id(g.outputs[i]);
+SHAPE_ASSIGN_CHECK(*out_shapes, i, shapes[eid]);
+  }
+  // Check if we have inferred the shapes correctly.
+  return g.GetAttr("shape_num_unknown_nodes") == 0;
+}
+
+inline bool DefaultSubgraphOpType(const nnvm::NodeAttrs& attrs,
+  std::vector *in_types,
+  std::vector *out_types) {
+  const nnvm::Symbol& subgraph_sym = *attrs.subgraphs[0];
+  nnvm::Graph g;
+  g.outputs = subgraph_sym.outputs;
+  const auto& idx_g = g.indexed_graph();
+  CHECK_EQ(idx_g.input_nodes().size(), in_types->size());
+  CHECK_EQ(idx_g.outputs().size(), out_types->size());
+
+  // Put the input and output data types to the dtype vector.
+  nnvm::DTypeVector types(idx_g.num_node_entries(), -1);
+  const auto &input_nids = idx_g.input_nodes();
+  CHECK_EQ(input_nids.size(), in_types->size());
+  for (size_t i = 0; i < in_types->size(); i++) {
+auto eid = idx_g.entry_id(input_nids[i], 0);
+types[eid] = in_types->at(i);
+  }
+  CHECK_EQ(g.outputs.size(), out_types->size());
+  for (size_t i = 0; i < out_types->size(); i++) {
+auto eid = idx_g.entry_id(g.outputs[i]);
+types[eid] = out_types->at(i);
+  }
+
+  // Infer data type of the graph.
+  g.attrs["dtype"] = std::make_shared(std::move(types));
+  g = exec::InferType(std::move(g));
+
+  types = g.GetAtt

[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-28 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r213502884
 
 

 ##
 File path: src/operator/subgraph/subgraph_property.h
 ##
 @@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef MXNET_OPERATOR_SUBGRAPH_SUBGRAPH_PROPERTY_H_
+#define MXNET_OPERATOR_SUBGRAPH_SUBGRAPH_PROPERTY_H_
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+namespace mxnet {
+namespace op {
+
+/*
+ * This provides criteria for selecting nodes in a subgraph.
+ * When a node is passed to this object, the selection criteria may be changed.
+ * We can also specify what links we should use when traversing the neighbor
+ * nodes.
+ */
+class SubgraphSelector {
+ public:
+  virtual ~SubgraphSelector() {}
+  // Determine if the node should be selected for a subgraph.
+  virtual bool Select(const nnvm::Node &n) = 0;
+  // Determine if the input node should be selected for a subgraph.
+  virtual bool SelectInput(const nnvm::Node &n, const nnvm::Node &new_node) = 
0;
 
 Review comment:
   Done.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-28 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r213503015
 
 

 ##
 File path: src/operator/subgraph/partition_graph.cc
 ##
 @@ -0,0 +1,774 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2018 by Contributors
+ * \file partition_graph.cc
+ * \brief
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "./subgraph_property.h"
+
+namespace nnvm {
+NodePtr CreateVariableNode(const std::string& name);
+}
+
+namespace mxnet {
+
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::NodePtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+#define DEBUG_SUBGRAPH 0
+
+namespace sg {  // sg stands for subgraph
+
+struct SimpleNode;
+using SimpleNodePtr = std::shared_ptr;
+
+/*!
+ * \brief Node of the undirected graph which replicates the network structures
+ * of the computational graph. It is used to ease the graph traversal for 
finding
+ * subgraphs.
+ */
+struct SimpleNode {
+  static SimpleNodePtr Create() {
+return std::make_shared();
+  }
+  SimpleNode() : label(-1), node(nullptr) {}
+  /*! subgraph label */
+  int label;
+  /*! the original node in the computational graph it references*/
+  nnvm::Node* node;
+  /*!
+   * \brief output nodes of the current node
+   * key is node ptr and value is an array of indices standing for the entry 
indices
+   * in key->inputs whose source is the current node.
+   */
+  std::unordered_map> outputs;
+};  // struct SimpleNode
+
+#if DEBUG_SUBGRAPH
+void PrintSubgraph(const std::vector& simple_nodes) {
+  std::string op_names = "";
+  for (size_t i = 0; i < simple_nodes.size(); ++i) {
+op_names += simple_nodes[i]->node->attrs.name + ' ';
+  }
+  LOG(INFO) << "Subgraph node names: " << op_names;
+}
+
+void PrintNodeEntry(const nnvm::NodeEntry& entry) {
+  std::string ret = "NodeEntry: node_name=" + entry.node->attrs.name
++ ", index=" + std::to_string(entry.index) + ", version=" + 
std::to_string(entry.version);
+  LOG(INFO) << ret;
+}
+
+void PrintNodeEntries(const std::vector& entries) {
+  for (size_t i = 0; i < entries.size(); ++i) {
+PrintNodeEntry(*entries[i]);
+  }
+}
+#endif
+
+/*!
+ * \brief Given a MXNet computational graph, create an undirected graph from 
it.
+ * \param g the MXNet computational graph
+ * \param simple_nodes the nodes of undirected graph in top sorted order
+ */
+void CreateSimpleGraph(const Graph& g,
+   std::vector* simple_nodes) {
+  const auto& indexed_graph = g.indexed_graph();
+  simple_nodes->reserve(indexed_graph.num_nodes());
+  DFSVisit(g.outputs, [&](const NodePtr& node) {
+SimpleNodePtr sn = SimpleNode::Create();
+sn->node = node.get();
+for (size_t i = 0; i < sn->node->inputs.size(); ++i) {
+  const auto& e = sn->node->inputs[i];
+  const auto input_nid = indexed_graph.node_id(e.node.get());
+  CHECK_LT(input_nid, simple_nodes->size());
+  auto& input_node_outputs = (*simple_nodes)[input_nid]->outputs;
+  auto it = input_node_outputs.find(sn->node);
+  if (it == input_node_outputs.end()) {
+input_node_outputs.emplace(sn->node, std::vector{i});
+  } else {
+it->second.push_back(i);
+  }
+}
+simple_nodes->emplace_back(std::move(sn));
+  });
+}
+
+/*!
+ * \brief Reset labels of the subgraph nodes to the original state
+ * and clear the vector of subgraph nodes.
+ */
+void ResetNodeLabels(const nnvm::Graph& g,
+ const std::vector& simple_nodes,
+ std::vector* subgraph_nodes) {
+  for (auto n : *subgraph_nodes) {
+const auto nid = g.indexed_graph().node_id(n);
+simple_nodes[nid]->label = -1;
+  }
+  subgraph_nodes->clear();
+}
+
+/*!
+ * \brief This function traverses the nodes in a computation graph from a 
starting
+ * node following the input edges and output edges, and marks all nodes that
+ * can be accessed from the starting node. Before the function returns,
+ * it will conduct checking whether there is a loop between the potential 
subgraph
+ * and the outside nodes. If so, add the node that should 

[GitHub] reminisce commented on a change in pull request #12157: Subgraph API for integrating accelerators with MXNet

2018-08-28 Thread GitBox
reminisce commented on a change in pull request #12157: Subgraph API for 
integrating accelerators with MXNet
URL: https://github.com/apache/incubator-mxnet/pull/12157#discussion_r213503117
 
 

 ##
 File path: src/operator/subgraph/partition_graph.cc
 ##
 @@ -0,0 +1,774 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ *  Copyright (c) 2018 by Contributors
+ * \file partition_graph.cc
+ * \brief
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "./subgraph_property.h"
+
+namespace nnvm {
+NodePtr CreateVariableNode(const std::string& name);
+}
+
+namespace mxnet {
+
+namespace op {
+
+using nnvm::Symbol;
+using nnvm::Node;
+using nnvm::NodePtr;
+using nnvm::NodeEntry;
+using nnvm::Graph;
+
+#define DEBUG_SUBGRAPH 0
+
+namespace sg {  // sg stands for subgraph
+
+struct SimpleNode;
+using SimpleNodePtr = std::shared_ptr;
+
+/*!
+ * \brief Node of the undirected graph which replicates the network structures
+ * of the computational graph. It is used to ease the graph traversal for 
finding
+ * subgraphs.
+ */
+struct SimpleNode {
+  static SimpleNodePtr Create() {
+return std::make_shared();
+  }
+  SimpleNode() : label(-1), node(nullptr) {}
+  /*! subgraph label */
+  int label;
+  /*! the original node in the computational graph it references*/
+  nnvm::Node* node;
+  /*!
+   * \brief output nodes of the current node
+   * key is node ptr and value is an array of indices standing for the entry 
indices
+   * in key->inputs whose source is the current node.
+   */
+  std::unordered_map> outputs;
+};  // struct SimpleNode
+
+#if DEBUG_SUBGRAPH
+void PrintSubgraph(const std::vector& simple_nodes) {
+  std::string op_names = "";
+  for (size_t i = 0; i < simple_nodes.size(); ++i) {
+op_names += simple_nodes[i]->node->attrs.name + ' ';
+  }
+  LOG(INFO) << "Subgraph node names: " << op_names;
+}
+
+void PrintNodeEntry(const nnvm::NodeEntry& entry) {
+  std::string ret = "NodeEntry: node_name=" + entry.node->attrs.name
++ ", index=" + std::to_string(entry.index) + ", version=" + 
std::to_string(entry.version);
+  LOG(INFO) << ret;
+}
+
+void PrintNodeEntries(const std::vector& entries) {
+  for (size_t i = 0; i < entries.size(); ++i) {
+PrintNodeEntry(*entries[i]);
+  }
+}
+#endif
+
+/*!
+ * \brief Given a MXNet computational graph, create an undirected graph from 
it.
+ * \param g the MXNet computational graph
+ * \param simple_nodes the nodes of undirected graph in top sorted order
+ */
+void CreateSimpleGraph(const Graph& g,
+   std::vector* simple_nodes) {
+  const auto& indexed_graph = g.indexed_graph();
+  simple_nodes->reserve(indexed_graph.num_nodes());
+  DFSVisit(g.outputs, [&](const NodePtr& node) {
+SimpleNodePtr sn = SimpleNode::Create();
+sn->node = node.get();
+for (size_t i = 0; i < sn->node->inputs.size(); ++i) {
+  const auto& e = sn->node->inputs[i];
+  const auto input_nid = indexed_graph.node_id(e.node.get());
+  CHECK_LT(input_nid, simple_nodes->size());
+  auto& input_node_outputs = (*simple_nodes)[input_nid]->outputs;
+  auto it = input_node_outputs.find(sn->node);
+  if (it == input_node_outputs.end()) {
+input_node_outputs.emplace(sn->node, std::vector{i});
+  } else {
+it->second.push_back(i);
+  }
+}
+simple_nodes->emplace_back(std::move(sn));
+  });
+}
+
+/*!
+ * \brief Reset labels of the subgraph nodes to the original state
+ * and clear the vector of subgraph nodes.
+ */
+void ResetNodeLabels(const nnvm::Graph& g,
+ const std::vector& simple_nodes,
+ std::vector* subgraph_nodes) {
+  for (auto n : *subgraph_nodes) {
+const auto nid = g.indexed_graph().node_id(n);
+simple_nodes[nid]->label = -1;
+  }
+  subgraph_nodes->clear();
+}
+
+/*!
+ * \brief This function traverses the nodes in a computation graph from a 
starting
+ * node following the input edges and output edges, and marks all nodes that
+ * can be accessed from the starting node. Before the function returns,
+ * it will conduct checking whether there is a loop between the potential 
subgraph
+ * and the outside nodes. If so, add the node that should