[GitHub] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r319333297 ## File path: python/mxnet/symbol/symbol.py ## @@ -1437,6 +1437,54 @@ def _gen_atomic_symbol(self): return Symbol(handle) +def optimize_for(self, backend, args=None, **kwargs): Review comment: I agree with you on having support for passing shape/type/stype dicts just like simple_bind. I will add that support in a later PR. Since we accept kwargs now, it would not be a breaking API change. I understand your concern about having a more explicit api, but I prefer keeping it this way for the following reason. Users are expected to know what arguments are required by the backend. The backend errors out if the required arguments are not provided whether or not we have an explicit flag. At this point, it's not a choice for users to not infer shape/type/stype when the backend requires it, or vice versa. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r318211857 ## File path: python/mxnet/symbol/symbol.py ## @@ -1437,6 +1437,54 @@ def _gen_atomic_symbol(self): return Symbol(handle) +def optimize_for(self, backend, args=None, **kwargs): Review comment: Enabled partial inference by removing `HandleInferShapeError` check. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API
URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r317867454
##
File path: src/c_api/c_api_symbolic.cc
##
@@ -1199,3 +1200,73 @@ int MXShallowCopySymbol(SymbolHandle src, SymbolHandle*
out) {
*out = out_sym;
API_END_HANDLE_ERROR(delete out_sym);
}
+
+int MXOptimizeForBackend(SymbolHandle sym_handle,
+ const char* backend_name,
+ SymbolHandle* ret_sym_handle,
+ const mx_uint len,
+ NDArrayHandle* in_args_handle,
+ const mx_uint num_options,
+ const char** keys,
+ const char** vals) {
+ nnvm::Symbol *s = new nnvm::Symbol();
+ API_BEGIN();
+ nnvm::Symbol *sym = static_cast(sym_handle);
+ *s = sym->Copy();
+ nnvm::Graph g = Symbol2Graph(*s);
+ if (len) {
+NDArray **in_args_ptr = reinterpret_cast(in_args_handle);
+Context default_ctx = in_args_ptr[0]->ctx();
+mxnet::ShapeVector arg_shapes(len);
+nnvm::DTypeVector arg_dtypes(len);
+StorageTypeVector arg_stypes(len);
+for (mx_uint i = 0; i < len; i++) {
+ const auto &in_arg = *(in_args_ptr[i]);
+ arg_shapes[i] = in_arg.shape();
+ arg_dtypes[i] = in_arg.dtype();
+ arg_stypes[i] = in_arg.storage_type();
+ CHECK(in_arg.ctx() == default_ctx)
+ << "args[" << i << "] is on context: " << in_arg.ctx()
+ << ", whereas args[0] is on context: " << default_ctx
+ << ". All args must be on the same context.";
+}
+const auto& indexed_graph = g.indexed_graph();
+const auto num_forward_inputs = indexed_graph.input_nodes().size();
+g.attrs["context"] = std::make_shared(
+exec::ContextVector(indexed_graph.num_nodes(), default_ctx));
+// infer shapes
+g = exec::InferShape(std::move(g), std::move(arg_shapes), "__shape__");
+if (g.GetAttr("shape_num_unknown_nodes") != 0U) {
Review comment:
Removed `HandleInferShapeError` check, partial infer shapes allowed now.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API
URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r317867454
##
File path: src/c_api/c_api_symbolic.cc
##
@@ -1199,3 +1200,73 @@ int MXShallowCopySymbol(SymbolHandle src, SymbolHandle*
out) {
*out = out_sym;
API_END_HANDLE_ERROR(delete out_sym);
}
+
+int MXOptimizeForBackend(SymbolHandle sym_handle,
+ const char* backend_name,
+ SymbolHandle* ret_sym_handle,
+ const mx_uint len,
+ NDArrayHandle* in_args_handle,
+ const mx_uint num_options,
+ const char** keys,
+ const char** vals) {
+ nnvm::Symbol *s = new nnvm::Symbol();
+ API_BEGIN();
+ nnvm::Symbol *sym = static_cast(sym_handle);
+ *s = sym->Copy();
+ nnvm::Graph g = Symbol2Graph(*s);
+ if (len) {
+NDArray **in_args_ptr = reinterpret_cast(in_args_handle);
+Context default_ctx = in_args_ptr[0]->ctx();
+mxnet::ShapeVector arg_shapes(len);
+nnvm::DTypeVector arg_dtypes(len);
+StorageTypeVector arg_stypes(len);
+for (mx_uint i = 0; i < len; i++) {
+ const auto &in_arg = *(in_args_ptr[i]);
+ arg_shapes[i] = in_arg.shape();
+ arg_dtypes[i] = in_arg.dtype();
+ arg_stypes[i] = in_arg.storage_type();
+ CHECK(in_arg.ctx() == default_ctx)
+ << "args[" << i << "] is on context: " << in_arg.ctx()
+ << ", whereas args[0] is on context: " << default_ctx
+ << ". All args must be on the same context.";
+}
+const auto& indexed_graph = g.indexed_graph();
+const auto num_forward_inputs = indexed_graph.input_nodes().size();
+g.attrs["context"] = std::make_shared(
+exec::ContextVector(indexed_graph.num_nodes(), default_ctx));
+// infer shapes
+g = exec::InferShape(std::move(g), std::move(arg_shapes), "__shape__");
+if (g.GetAttr("shape_num_unknown_nodes") != 0U) {
Review comment:
Removed `HandleInferShapeError` check allowing for partial infer shapes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API
URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r317783611
##
File path: src/operator/subgraph/build_subgraph.cc
##
@@ -572,27 +572,30 @@ void CreateSubgraphNode(nnvm::Graph* g,
}
const SubgraphPropertyPtr& subg_prop =
g->GetAttr("subgraph_property");
nnvm::NodePtr n = subg_prop->CreateSubgraphNode(sym, subgraph_selector,
subgraph_id);
-
- // Connect the external nodes to the subgraph node.
- subg_prop->ConnectSubgraphOutputs(n, &output_entries);
- subg_prop->ConnectSubgraphInputs(n, &input_entries, &orig_input_entries);
-
- const auto& indexed_graph = g->indexed_graph();
- for (size_t i = 0; i < n->inputs.size(); ++i) {
-auto& e = n->inputs[i];
-// update entry_top_order_map with newly created orig_input_entries
-auto it = entry_top_order_map->find(input_entries[i]);
-CHECK(it != entry_top_order_map->end());
-entry_top_order_map->emplace(&e, it->second);
-// update input entries' source simple nodes' outputs map
-nnvm::Node* node = e.node.get();
-if (indexed_graph.exist(node)) {
- const auto nid = indexed_graph.node_id(node);
- BiDirectedNode* sn = simple_nodes[nid].get();
- for (BiDirectedNode* dest_node : subgraph_nodes) {
-sn->outputs.erase(dest_node->node);
+ // CreateSubgraphNode returns NULL if subgraph property determines that
subgraph is sub-optimal
+ // In that case, subgraph node is not created and graph is not modified
+ if (n) {
+// Connect the external nodes to the subgraph node.
+subg_prop->ConnectSubgraphOutputs(n, &output_entries);
+subg_prop->ConnectSubgraphInputs(n, &input_entries, &orig_input_entries);
+
+const auto& indexed_graph = g->indexed_graph();
+for (size_t i = 0; i < n->inputs.size(); ++i) {
+ auto& e = n->inputs[i];
+ // update entry_top_order_map with newly created orig_input_entries
+ auto it = entry_top_order_map->find(input_entries[i]);
+ CHECK(it != entry_top_order_map->end());
+ entry_top_order_map->emplace(&e, it->second);
+ // update input entries' source simple nodes' outputs map
+ nnvm::Node* node = e.node.get();
+ if (indexed_graph.exist(node)) {
+const auto nid = indexed_graph.node_id(node);
+BiDirectedNode* sn = simple_nodes[nid].get();
+for (BiDirectedNode* dest_node : subgraph_nodes) {
+ sn->outputs.erase(dest_node->node);
+}
+sn->outputs[n.get()].push_back(i);
}
- sn->outputs[n.get()].push_back(i);
}
}
#if DEBUG_SUBGRAPH
Review comment:
`output_entries` is set
[here](https://github.com/apache/incubator-mxnet/blob/master/src/operator/subgraph/build_subgraph.cc#L565),
before subgraph node is created. We update `output_entries` if subgraph node
is created. If subgraph node is not created, `output_entries` is not updated
and we print the same values again.
I am adding a log message for whether the node is created and
`output_entries` updated.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API
URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r317331906
##
File path: src/c_api/c_api_symbolic.cc
##
@@ -1199,3 +1200,73 @@ int MXShallowCopySymbol(SymbolHandle src, SymbolHandle*
out) {
*out = out_sym;
API_END_HANDLE_ERROR(delete out_sym);
}
+
+int MXOptimizeForBackend(SymbolHandle sym_handle,
+ const char* backend_name,
+ SymbolHandle* ret_sym_handle,
+ const mx_uint len,
+ NDArrayHandle* in_args_handle,
+ const mx_uint num_options,
+ const char** keys,
+ const char** vals) {
+ nnvm::Symbol *s = new nnvm::Symbol();
+ API_BEGIN();
+ nnvm::Symbol *sym = static_cast(sym_handle);
+ *s = sym->Copy();
+ nnvm::Graph g = Symbol2Graph(*s);
+ if (len) {
+NDArray **in_args_ptr = reinterpret_cast(in_args_handle);
+Context default_ctx = in_args_ptr[0]->ctx();
+mxnet::ShapeVector arg_shapes(len);
+nnvm::DTypeVector arg_dtypes(len);
+StorageTypeVector arg_stypes(len);
+for (mx_uint i = 0; i < len; i++) {
+ const auto &in_arg = *(in_args_ptr[i]);
+ arg_shapes[i] = in_arg.shape();
+ arg_dtypes[i] = in_arg.dtype();
+ arg_stypes[i] = in_arg.storage_type();
+ CHECK(in_arg.ctx() == default_ctx)
+ << "args[" << i << "] is on context: " << in_arg.ctx()
+ << ", whereas args[0] is on context: " << default_ctx
+ << ". All args must be on the same context.";
+}
+const auto& indexed_graph = g.indexed_graph();
+const auto num_forward_inputs = indexed_graph.input_nodes().size();
+g.attrs["context"] = std::make_shared(
+exec::ContextVector(indexed_graph.num_nodes(), default_ctx));
+// infer shapes
+g = exec::InferShape(std::move(g), std::move(arg_shapes), "__shape__");
+if (g.GetAttr("shape_num_unknown_nodes") != 0U) {
+ common::HandleInferShapeError(num_forward_inputs, indexed_graph,
+g.GetAttr("shape"));
+}
+// infer dtypes
+g = exec::InferType(std::move(g), std::move(arg_dtypes), "__dtype__");
+if (g.GetAttr("dtype_num_unknown_nodes") != 0U) {
+ common::HandleInferTypeError(num_forward_inputs, indexed_graph,
+ g.GetAttr("dtype"));
+}
+// infer stypes
+g = exec::InferStorageType(std::move(g), std::move(arg_stypes),
"__storage_type__");
+if (g.GetAttr("storage_type_num_unknown_nodes") != 0U) {
+ common::HandleInferStorageTypeError(num_forward_inputs, indexed_graph,
+
g.GetAttr("storage_type"));
+}
+ }
+ std::vector> options_map;
+ for (mx_uint i = 0; i < num_options; ++i) {
+options_map.emplace_back(keys[i], vals[i]);
+ }
+ const auto backend =
mxnet::op::SubgraphBackendRegistry::Get()->GetSubgraphBackend(backend_name);
+ const auto& subgraph_prop_list = backend->GetSubgraphProperties();
+ for (auto property : subgraph_prop_list) {
+property->PrePartition(g, options_map);
+g.attrs["subgraph_property"] = std::make_shared(property);
+g = ApplyPass(std::move(g), "BuildSubgraph");
+g.attrs.erase("subgraph_property");
+property->PostPartition(g);
+s->outputs = g.outputs;
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API
URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r317325297
##
File path: src/c_api/c_api_symbolic.cc
##
@@ -1199,3 +1200,73 @@ int MXShallowCopySymbol(SymbolHandle src, SymbolHandle*
out) {
*out = out_sym;
API_END_HANDLE_ERROR(delete out_sym);
}
+
+int MXOptimizeForBackend(SymbolHandle sym_handle,
+ const char* backend_name,
+ SymbolHandle* ret_sym_handle,
+ const mx_uint len,
+ NDArrayHandle* in_args_handle,
+ const mx_uint num_options,
+ const char** keys,
+ const char** vals) {
+ nnvm::Symbol *s = new nnvm::Symbol();
+ API_BEGIN();
+ nnvm::Symbol *sym = static_cast(sym_handle);
+ *s = sym->Copy();
+ nnvm::Graph g = Symbol2Graph(*s);
+ if (len) {
+NDArray **in_args_ptr = reinterpret_cast(in_args_handle);
+Context default_ctx = in_args_ptr[0]->ctx();
+mxnet::ShapeVector arg_shapes(len);
+nnvm::DTypeVector arg_dtypes(len);
+StorageTypeVector arg_stypes(len);
+for (mx_uint i = 0; i < len; i++) {
+ const auto &in_arg = *(in_args_ptr[i]);
+ arg_shapes[i] = in_arg.shape();
+ arg_dtypes[i] = in_arg.dtype();
+ arg_stypes[i] = in_arg.storage_type();
+ CHECK(in_arg.ctx() == default_ctx)
+ << "args[" << i << "] is on context: " << in_arg.ctx()
+ << ", whereas args[0] is on context: " << default_ctx
+ << ". All args must be on the same context.";
+}
+const auto& indexed_graph = g.indexed_graph();
+const auto num_forward_inputs = indexed_graph.input_nodes().size();
+g.attrs["context"] = std::make_shared(
+exec::ContextVector(indexed_graph.num_nodes(), default_ctx));
+// infer shapes
+g = exec::InferShape(std::move(g), std::move(arg_shapes), "__shape__");
+if (g.GetAttr("shape_num_unknown_nodes") != 0U) {
+ common::HandleInferShapeError(num_forward_inputs, indexed_graph,
+g.GetAttr("shape"));
+}
+// infer dtypes
+g = exec::InferType(std::move(g), std::move(arg_dtypes), "__dtype__");
+if (g.GetAttr("dtype_num_unknown_nodes") != 0U) {
+ common::HandleInferTypeError(num_forward_inputs, indexed_graph,
+ g.GetAttr("dtype"));
+}
+// infer stypes
+g = exec::InferStorageType(std::move(g), std::move(arg_stypes),
"__storage_type__");
+if (g.GetAttr("storage_type_num_unknown_nodes") != 0U) {
+ common::HandleInferStorageTypeError(num_forward_inputs, indexed_graph,
+
g.GetAttr("storage_type"));
+}
+ }
+ std::vector> options_map;
+ for (mx_uint i = 0; i < num_options; ++i) {
+options_map.emplace_back(keys[i], vals[i]);
+ }
+ const auto backend =
mxnet::op::SubgraphBackendRegistry::Get()->GetSubgraphBackend(backend_name);
+ const auto& subgraph_prop_list = backend->GetSubgraphProperties();
+ for (auto property : subgraph_prop_list) {
+property->PrePartition(g, options_map);
+g.attrs["subgraph_property"] = std::make_shared(property);
+g = ApplyPass(std::move(g), "BuildSubgraph");
+g.attrs.erase("subgraph_property");
+property->PostPartition(g);
+s->outputs = g.outputs;
Review comment:
Agreed. I will make this change.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r317239988 ## File path: tests/python/unittest/test_subgraph_op.py ## @@ -146,11 +146,135 @@ def get_executor(sym, subgraph_backend=None, op_names=None, original_exec=None): for i in range(len(outputs1)): assert_almost_equal((outputs1[i] - outputs2[i]).abs().sum().asnumpy(), np.zeros(shape=(1,))) +def _check_subgraph_exe5(sym, subgraph_backend, op_names): +"""Call optimize_for to trigger graph partitioning without infer shapes/types before, +then simple_bind and compare results of the partitioned sym and the original sym.""" +# simple_bind +exe1 = sym.simple_bind(ctx=mx.current_context(), grad_req='null') +input_names = sym.list_inputs() +for name in input_names: +if name in exe1.arg_dict: +exe1.arg_dict[name][:] = mx.nd.random.uniform(shape=exe1.arg_dict[name].shape) +else: +assert name in exe1.aux_dict +exe1.aux_dict[name][:] = mx.nd.random.uniform(shape=exe1.aux_dict[name].shape) +exe1.forward() + +# partition before simple_bind +check_call(_LIB.MXSetSubgraphPropertyOpNames(c_str(subgraph_backend), mx_uint(len(op_names)), + c_str_array(op_names))) +part_sym = sym.optimize_for(subgraph_backend) + check_call(_LIB.MXRemoveSubgraphPropertyOpNames(c_str(subgraph_backend))) +exe2 = part_sym.simple_bind(ctx=mx.current_context(), grad_req='null') +for name in input_names: Review comment: The loop assigns random inputs to executor generated using simple_bind. Both executors share the same set of inputs. Refactored the code and added comments. This is an automated message from the Apache Git Service. To respond to the message, please log on to 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] [incubator-mxnet] mseth10 commented on a change in pull request #15886: Graph Partition API
mseth10 commented on a change in pull request #15886: Graph Partition API
URL: https://github.com/apache/incubator-mxnet/pull/15886#discussion_r317238730
##
File path: src/operator/subgraph/subgraph_property.h
##
@@ -221,6 +222,12 @@ class SubgraphProperty {
return nullptr;
}
+ virtual void PrePartition(const nnvm::Graph& g,
+const std::vector>& options_map) {}
+
+ virtual void PostPartition(const nnvm::Graph& g,
+const std::vector>& options_map) {}
Review comment:
Removed options_map from PostPartition
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
