You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/07/23 17:46:18 UTC
[GitHub] [incubator-mxnet] samskalicky opened a new pull request #18779: [WIP] Support extra inputs for subgraph ops
samskalicky opened a new pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779
## Description ##
Support additional inputs to custom subgraph ops that are not direct dependencies to ops in the subgraph. This will enable various use cases: custom control flow ops, custom ops that maintain a state that should be saved/loaded, etc.
WIP, more description, tests, examples to come....
## Checklist ##
### Essentials ###
Please feel free to remove inapplicable items for your PR.
- [ ] The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant [JIRA issue](https://issues.apache.org/jira/projects/MXNET/issues) created (except PRs with tiny changes)
- [ ] Changes are complete (i.e. I finished coding on this PR)
- [ ] All changes have test coverage:
- Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
- Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
- Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
- [ ] Code is well-documented:
- For user-facing API changes, API doc string has been updated.
- For new C++ functions in header files, their functionalities and arguments are documented.
- For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
- Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
- [ ] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
### Changes ###
- [ ] Feature1, tests, (and when applicable, API doc)
- [ ] Feature2, tests, (and when applicable, API doc)
## Comments ##
- If this change is a backward incompatible change, why must this change be made.
- Interesting edge cases to note here
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #18779: Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r470258583
##########
File path: include/mxnet/lib_api.h
##########
@@ -748,62 +759,410 @@ struct JsonParser {
return JsonVal();
}
// generic parse function
- JsonVal parse(const std::string& json, unsigned int *idx) {
+ static JsonVal parse(const std::string& json, unsigned int *idx) {
JsonVal ret;
while (*idx < json.size()) {
if (json[*idx] == '"') {
++(*idx);
- ret = parse_string(json, idx);
+ ret = JsonVal::parse_string(json, idx);
} else if (json[*idx] >= '0' && json[*idx] <= '9') {
- ret = parse_num(json, idx);
+ ret = JsonVal::parse_num(json, idx);
} else if (json[*idx] == '[') {
++(*idx);
- ret = parse_list(json, idx);
+ ret = JsonVal::parse_list(json, idx);
} else if (json[*idx] == '{') {
++(*idx);
- ret = parse_map(json, idx);
+ ret = JsonVal::parse_map(json, idx);
} else if (json[*idx] == ']' || json[*idx] == '}') {return ret;}
if (ret.type != ERR) return ret;
++(*idx);
}
return ret;
}
- // convert JSON object back to JSON-compatible string
- std::string dump(const JsonVal &val) {
+ // debug function to convert data structure to a debugstring
+ std::string toString() const {
std::string ret;
- switch (val.type) {
+ switch (type) {
case ERR:
ret = "json(Error)";
break;
case STR:
- ret = "\"" + val.str + "\"";
+ ret = "json(STR:" + str + ")";
break;
case NUM:
- ret = val.str;
+ ret = "json(INT:" + str + ")";
break;
case LIST:
- ret = "[";
- for (unsigned i=0; i < val.list.size(); i++) {
- auto &item = val.list[i];
- ret += dump(item);
- if (i < val.list.size()-1)
- ret += ",";
- }
- ret += "]";
+ ret = "json(LIST:[";
+ for (auto &item : list)
+ ret += item.toString() + ",";
+ ret += "])";
break;
case MAP:
- ret = "{";
- unsigned cnt = 0;
- for (auto &item : val.map) {
- ret += dump(item.first) + " : " + dump(item.second);
- if (cnt++ < val.map.size()-1)
- ret += ",";
- }
- ret += "}";
+ ret = "json(MAP:{";
+ for (auto &item : map)
+ ret += item.first.toString() + " : " + item.second.toString() + ",";
+ ret += "})";
break;
}
return ret;
}
+ JsonType type;
+ int num;
+ std::string str;
+ std::vector<JsonVal> list;
+ std::map<JsonVal, JsonVal> map;
+};
+
+/*!
+ * \brief Graph utility to parse serialized subgraph symbol
+ */
+class Node;
+class Graph;
+
+// Representation of an input/output to a node
+struct NodeEntry {
+ Node* node; // other node thats producing/consuming inputs/outputs
+ int entry; // entry index from other node (ie. output index from producing node)
+};
+
+// Representation of a node in the graph
+class Node {
+ public:
+ Node() {tensor = nullptr;}
+ // internally set passResource to enable tensor allocation for graph passes
+ void _setPassResource(PassResource* res_) {res = res_;}
+ /* \brief allocate an arg tensor for this node */
+ void alloc_arg(const std::vector<int64_t>& shapes,
+ const MXContext &ctx, MXDType dtype) {
+ if (!res)
+ throw std::runtime_error(
+ "Node not initialized. Cannot use alloc_arg outside of graph passes.");
+ tensor = res->alloc_arg(name, shapes, ctx, dtype);
+ }
+ /* \brief allocate an aux tensor for this node */
+ void alloc_aux(const std::vector<int64_t>& shapes,
+ const MXContext &ctx, MXDType dtype) {
+ if (!res)
+ throw std::runtime_error(
+ "Node not initialized. Cannot use alloc_aux outside of graph passes.");
+ tensor = res->alloc_aux(name, shapes, ctx, dtype);
+ }
+ std::string op; // operator name (ie. Convolution)
+ std::string name; // unique node name (ie. conv_0 or conv_1)
+ MXTensor* tensor; // tensor data for input nodes
+ std::vector<NodeEntry> inputs; // set of inputs to the node
+ std::vector<NodeEntry> outputs; // set of outputs from the node
+ std::vector<Graph*> subgraphs; // set of subgraphs within this node
+ std::unordered_map<std::string, std::string> attrs; // node attributes
+
+ private:
+ PassResource* res;
+};
+
+// Representation of the graph
+class Graph {
+ public:
+ Graph() : res(nullptr) {}
+ /* \brief deleted nodes when deleting the graph */
+ ~Graph() {
+ for (int i = 0; i < nodes.size(); i++)
+ delete nodes[i];
+ }
+
+ /* \brief create a graph object from an unparsed string */
+ static Graph* fromString(const std::string& json) {
+ JsonVal val = JsonVal::parse(json);
+ return fromJson(val);
+ }
+
+ /* \brief create a graph object from a parsed JSON object */
+ static Graph* fromJson(JsonVal val) {
+ // get nodes list
+ JsonVal nodes = val.map[JsonVal("nodes")];
+ Graph *g = new Graph();
+
+ std::map<int, Node*> nodeMap;
+ // loop over nodes
+ for (int i = 0; i < nodes.list.size(); i++) {
+ Node* n = new Node();
+ g->nodes.push_back(n);
+ JsonVal node = nodes.list[i];
+
+ // set the op info
+ n->op = node.map[JsonVal("op")].str;
+ n->name = node.map[JsonVal("name")].str;
+
+ // if op is null it is an input to the graph
+ if (n->op.compare("null") == 0)
+ g->inputs.push_back(n);
+
+ // set attrs
+ JsonVal attributes = node.map[JsonVal("attrs")];
+ for (auto& kv : attributes.map) {
+ n->attrs[kv.first.str] = kv.second.str;
+ }
+
+ // set subgraphs, parsing each into a graph
+ if (node.map.count(JsonVal("subgraphs")) > 0) {
+ JsonVal subgraphs = node.map[JsonVal("subgraphs")];
+ for (auto &subgraph : subgraphs.list) {
+ n->subgraphs.push_back(fromJson(subgraph));
+ }
+ }
+
+ // set node inputs
+ JsonVal node_inputs = node.map[JsonVal("inputs")];
+ n->inputs.resize(node_inputs.list.size());
+ for (int j = 0; j < node_inputs.list.size(); j++) {
+ JsonVal input = node_inputs.list[j];
+ NodeEntry& entry = n->inputs[j];
+ // get pointer to other node
+ entry.node = nodeMap[input.list[0].num];
+ // get the other node's output index
+ entry.entry = input.list[1].num;
+ // set other nodes output as connected to this node
+ entry.node->outputs.push_back({n, j});
+ }
+ nodeMap[i] = n;
+ }
+
+ // set graph level outputs
+ JsonVal& heads = val.map[JsonVal("heads")];
+ g->outputs.resize(heads.list.size());
+ for (int i = 0; i < heads.list.size(); i++) {
+ JsonVal head = heads.list[i];
+ g->outputs[i].node = nodeMap[head.list[0].num];
+ g->outputs[i].entry = head.list[1].num;
+ }
+
+ // add all attributes to the graph
+ for (auto& kv : val.map) {
+ if (kv.first.str.compare("nodes") != 0 &&
+ kv.first.str.compare("heads") != 0 &&
+ kv.first.str.compare("node_row_ptr") != 0 &&
+ kv.first.str.compare("arg_nodes") != 0) {
+ g->attrs[kv.first.str] = kv.second;
+ }
+ }
+ return g;
+ }
+
+ /* \brief convert graph object back to JSON object */
+ JsonVal toJson() {
+ // top level object is a map
+ JsonVal val(MAP);
+
+ // add attributes
+ for (auto& kv : attrs) {
+ val.map[JsonVal(kv.first)] = kv.second;
+ }
+
+ // sort graph nodes in topological order, create mapping of node to index
+ std::map<Node*, int> nodeMap;
+ std::vector<Node*> sorted = topological_sort();
+ // nodes are in reverse topological order in the vector (back is first)
+ // so loop from end to front over the vector 'sorted'
+ for (int i = sorted.size()-1; i >= 0; i--) {
+ nodeMap[sorted[i]] = sorted.size()-1-i;
+ }
+
+ // create node_row_ptr entry
+ val.map[JsonVal("node_row_ptr")] = JsonVal(LIST);
+ JsonVal& node_row_ptr = val.map[JsonVal("node_row_ptr")];
+ for (int i = 0; i < nodes.size(); i++)
+ node_row_ptr.list.push_back(JsonVal(i));
+
+ // add all input nodes
+ val.map[JsonVal("arg_nodes")] = JsonVal(LIST);
+ JsonVal& arg_nodes = val.map[JsonVal("arg_nodes")];
+ for (int i = 0; i < inputs.size(); i++)
+ arg_nodes.list.push_back(JsonVal(nodeMap[inputs[i]]));
+
+ // add all output nodes
+ val.map[JsonVal("heads")] = JsonVal(LIST);
+ JsonVal& heads = val.map[JsonVal("heads")];
+ for (int i = 0; i < outputs.size(); i++) {
+ heads.list.push_back(JsonVal(LIST));
+ JsonVal& out = heads.list[i];
+ out.list.push_back(JsonVal(nodeMap[outputs[i].node]));
+ out.list.push_back(JsonVal(outputs[i].entry));
+ out.list.push_back(JsonVal(0));
+ }
+
+ // add all graph nodes
+ val.map[JsonVal("nodes")] = JsonVal(LIST);
+ JsonVal& nodes_ = val.map[JsonVal("nodes")];
+ for (int i = sorted.size()-1; i >= 0; i--) {
+ // each node is a map
+ nodes_.list.push_back(JsonVal(MAP));
+ Node* n = sorted[i];
+ JsonVal& n_ = nodes_.list[nodes_.list.size()-1];
+
+ n_.map[JsonVal("op")] = JsonVal(n->op);
+ n_.map[JsonVal("name")] = JsonVal(n->name);
+ n_.map[JsonVal("inputs")] = JsonVal(LIST);
+
+ // add inputs for this node
+ JsonVal& inputs_ = n_.map[JsonVal("inputs")];
+ for (int j = 0; j < n->inputs.size(); j++) {
+ inputs_.list.push_back(JsonVal(LIST));
+ NodeEntry& entry = n->inputs[j];
+ JsonVal& in = inputs_.list[j];
+ in.list.push_back(JsonVal(nodeMap[entry.node]));
+ in.list.push_back(JsonVal(entry.entry));
+ in.list.push_back(JsonVal(0));
+ }
+
+ // add subgraphs for this node, convert each back to JSON
+ if (n->subgraphs.size() > 0) {
+ n_.map[JsonVal("subgraphs")] = JsonVal(LIST);
+ JsonVal &subgraphs_ = n_.map[JsonVal("subgraphs")];
+ for (Graph *subgraph : n->subgraphs) {
+ subgraphs_.list.push_back(subgraph->toJson());
+ }
+ }
+
+ // add attributes for this node
+ n_.map[JsonVal("attrs")] = JsonVal(MAP);
+ JsonVal& attrs_ = n_.map[JsonVal("attrs")];
+ for (auto& kv : n->attrs) {
+ attrs_.map[JsonVal(kv.first)] = JsonVal(kv.second);
+ }
+ }
+ return val;
+ }
+
+ /* \brief convert graph object to JSON string */
+ std::string toString() {
+ return toJson().dump();
+ }
+
+ /* \brief visits a node "n" */
+ void _dfs_util(Node* n, std::unordered_set<Node*>* to_visit,
+ std::function<void(Node*)> handler) const {
+ to_visit->erase(n); // remove node now that we're visiting it
+ for (NodeEntry& e : n->outputs) {
+ Node* o = e.node;
+ if (to_visit->count(o) != 0) {
+ _dfs_util(o, to_visit, handler); // visit neighbor
+ }
+ }
+ handler(n); // post-order visit this node
+ }
+
+ /* \brief post-order DFS graph traversal */
+ void DFS(std::function<void(Node*)> handler) const {
+ std::unordered_set<Node*> to_visit;
+ // put all nodes in set to visit
+ for (auto& n : nodes)
+ to_visit.insert(n);
+ // visit all inputs first
+ for (auto& i : inputs)
+ if (to_visit.count(i) != 0)
+ _dfs_util(i, &to_visit, handler);
+ // visit any nodes left
+ while (to_visit.size() > 0)
+ _dfs_util(*(to_visit.begin()), &to_visit, handler);
+ }
+
+ /* \brief sort graph nodes in topological order */
+ std::vector<Node*> topological_sort() const {
+ std::vector<Node*> sorted;
+ auto handler = [&](Node* n) {
+ sorted.push_back(n); // when visiting each node, add it in order to the vector
+ };
+ DFS(handler);
+ return sorted;
+ }
+
+ /* \brief print out graph details */
+ void print(int indent = 0) const {
+ std::string space = "";
+ for (int i = 0; i < indent; i++) space+=" ";
+
+ std::cout << space << "########### Graph #############" << std::endl;
+ std::cout << space << "attributes: " << std::endl;
+ for (auto &kv : attrs)
+ std::cout << space << "\t" << kv.first << " : " << kv.second.str << std::endl;
+ std::cout << space << "inputs: " << inputs.size() << std::endl;
+ std::cout << space << "outputs: " << outputs.size() << std::endl;
+ std::cout << space << "nodes: " << nodes.size() << std::endl;
+ std::vector<Node*> sorted = topological_sort();
+ // loop over each node and print out its inputs/outputs
+ for (int i = sorted.size()-1; i >= 0; i--) {
+ std::cout << space << "Node: " << sorted[i]->name << std::endl;
+ for (int j = 0; j < sorted[i]->inputs.size(); j++) {
+ std::cout << space << "\tInput: " << sorted[i]->inputs[j].node->name << " "
+ << sorted[i]->inputs[j].entry << std::endl;
+ }
+ for (int j = 0; j < sorted[i]->outputs.size(); j++) {
+ std::cout << space << "\tOutput: " << sorted[i]->outputs[j].node->name << " "
+ << sorted[i]->outputs[j].entry << std::endl;
+ }
+ if (sorted[i]->subgraphs.size() > 0) {
+ for (auto &subgraph : sorted[i]->subgraphs) {
+ std::cout << space << "\tSubgraph:" << std::endl;
+ subgraph->print(indent+2);
+ }
+ }
+ }
+ std::cout << space << "###############################" << std::endl;
+ }
+
+ /* \brief add a new node to this graph */
+ Node* addNode(const std::string& name, const std::string& op) {
+ Node* n = new Node();
+ n->name = name;
+ n->op = op;
+ if (res)
+ n->_setPassResource(res);
+ return n;
+ }
+ /* \brief get node at index in graph */
+ Node* getNode(size_t idx) {
Review comment:
one is const
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #18779: Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r468738020
##########
File path: include/mxnet/lib_api.h
##########
@@ -748,62 +759,410 @@ struct JsonParser {
return JsonVal();
}
// generic parse function
- JsonVal parse(const std::string& json, unsigned int *idx) {
+ static JsonVal parse(const std::string& json, unsigned int *idx) {
JsonVal ret;
while (*idx < json.size()) {
if (json[*idx] == '"') {
++(*idx);
- ret = parse_string(json, idx);
+ ret = JsonVal::parse_string(json, idx);
} else if (json[*idx] >= '0' && json[*idx] <= '9') {
- ret = parse_num(json, idx);
+ ret = JsonVal::parse_num(json, idx);
} else if (json[*idx] == '[') {
++(*idx);
- ret = parse_list(json, idx);
+ ret = JsonVal::parse_list(json, idx);
} else if (json[*idx] == '{') {
++(*idx);
- ret = parse_map(json, idx);
+ ret = JsonVal::parse_map(json, idx);
} else if (json[*idx] == ']' || json[*idx] == '}') {return ret;}
if (ret.type != ERR) return ret;
++(*idx);
}
return ret;
}
- // convert JSON object back to JSON-compatible string
- std::string dump(const JsonVal &val) {
+ // debug function to convert data structure to a debugstring
+ std::string toString() const {
std::string ret;
- switch (val.type) {
+ switch (type) {
case ERR:
ret = "json(Error)";
break;
case STR:
- ret = "\"" + val.str + "\"";
+ ret = "json(STR:" + str + ")";
break;
case NUM:
- ret = val.str;
+ ret = "json(INT:" + str + ")";
break;
case LIST:
- ret = "[";
- for (unsigned i=0; i < val.list.size(); i++) {
- auto &item = val.list[i];
- ret += dump(item);
- if (i < val.list.size()-1)
- ret += ",";
- }
- ret += "]";
+ ret = "json(LIST:[";
+ for (auto &item : list)
+ ret += item.toString() + ",";
+ ret += "])";
break;
case MAP:
- ret = "{";
- unsigned cnt = 0;
- for (auto &item : val.map) {
- ret += dump(item.first) + " : " + dump(item.second);
- if (cnt++ < val.map.size()-1)
- ret += ",";
- }
- ret += "}";
+ ret = "json(MAP:{";
+ for (auto &item : map)
+ ret += item.first.toString() + " : " + item.second.toString() + ",";
+ ret += "})";
break;
}
return ret;
}
+ JsonType type;
+ int num;
+ std::string str;
+ std::vector<JsonVal> list;
+ std::map<JsonVal, JsonVal> map;
+};
+
+/*!
+ * \brief Graph utility to parse serialized subgraph symbol
+ */
+class Node;
+class Graph;
+
+// Representation of an input/output to a node
+struct NodeEntry {
+ Node* node; // other node thats producing/consuming inputs/outputs
+ int entry; // entry index from other node (ie. output index from producing node)
+};
+
+// Representation of a node in the graph
+class Node {
+ public:
+ Node() {tensor = nullptr;}
+ // internally set passResource to enable tensor allocation for graph passes
+ void _setPassResource(PassResource* res_) {res = res_;}
+ /* \brief allocate an arg tensor for this node */
+ void alloc_arg(const std::vector<int64_t>& shapes,
+ const MXContext &ctx, MXDType dtype) {
+ if (!res)
+ throw std::runtime_error(
+ "Node not initialized. Cannot use alloc_arg outside of graph passes.");
+ tensor = res->alloc_arg(name, shapes, ctx, dtype);
+ }
+ /* \brief allocate an aux tensor for this node */
+ void alloc_aux(const std::vector<int64_t>& shapes,
+ const MXContext &ctx, MXDType dtype) {
+ if (!res)
+ throw std::runtime_error(
+ "Node not initialized. Cannot use alloc_aux outside of graph passes.");
+ tensor = res->alloc_aux(name, shapes, ctx, dtype);
+ }
+ std::string op; // operator name (ie. Convolution)
+ std::string name; // unique node name (ie. conv_0 or conv_1)
+ MXTensor* tensor; // tensor data for input nodes
+ std::vector<NodeEntry> inputs; // set of inputs to the node
+ std::vector<NodeEntry> outputs; // set of outputs from the node
+ std::vector<Graph*> subgraphs; // set of subgraphs within this node
+ std::unordered_map<std::string, std::string> attrs; // node attributes
+
+ private:
+ PassResource* res;
+};
+
+// Representation of the graph
+class Graph {
+ public:
+ Graph() : res(nullptr) {}
+ /* \brief deleted nodes when deleting the graph */
+ ~Graph() {
+ for (int i = 0; i < nodes.size(); i++)
+ delete nodes[i];
+ }
+
+ /* \brief create a graph object from an unparsed string */
+ static Graph* fromString(const std::string& json) {
+ JsonVal val = JsonVal::parse(json);
+ return fromJson(val);
+ }
+
+ /* \brief create a graph object from a parsed JSON object */
+ static Graph* fromJson(JsonVal val) {
+ // get nodes list
+ JsonVal nodes = val.map[JsonVal("nodes")];
+ Graph *g = new Graph();
+
+ std::map<int, Node*> nodeMap;
+ // loop over nodes
+ for (int i = 0; i < nodes.list.size(); i++) {
+ Node* n = new Node();
+ g->nodes.push_back(n);
+ JsonVal node = nodes.list[i];
+
+ // set the op info
+ n->op = node.map[JsonVal("op")].str;
+ n->name = node.map[JsonVal("name")].str;
+
+ // if op is null its an input to the graph
Review comment:
fixed!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r462563043
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -331,6 +332,46 @@ REGISTER_PARTITIONER(mySelect)
.setCreateSelector("strategy1", createSelector)
.setReviewSubgraph("strategy1", myReviewSubgraph);
+/* \brief a basic pass that adds a new input for subgraph ops */
+MXReturnValue addInputPass(const std::string& in_graph, const std::string** out_graph,
+ const std::unordered_map<std::string, std::string>& options,
+ const std::unordered_map<std::string, MXTensor>& args,
+ const std::unordered_map<std::string, MXTensor>& aux,
+ const PassResource& res) {
+ // convert graph from JSON string to Graph/Node data structure
+ Graph *g = Graph::fromString(in_graph);
+ //find node with '_custom_subgraph_op' op type
+ for(Node* n : g->nodes) {
+ if(n->op.compare("_custom_subgraph_op") == 0) {
+ //set extra input
+ n->attrs[MX_STR_EXTRA_INPUTS] = std::to_string(1);
+
+ //create a new input Node
+ Node* input = new Node();
+ std::string input_name = n->name + "_input";
+ input->name = input_name;
+ input->op = "null";
+ //add a new node in graph
+ g->nodes.push_back(input);
+ g->inputs.push_back(input);
+ //connect new input to node
+ input->outputs.push_back({n,(int)(n->inputs.size())});
+ //connect node to new input
+ n->inputs.push_back({input,0});
+ // add a corresponding tensor for this input
+ MXTensor* arg_ = res.alloc_arg(input_name,{1},MXContext::CPU(0),kFloat32);
+ }
+ }
+
+ //convert back to JSON string from Graph/Node
+ *out_graph = new std::string(g->toString());
+ return MX_SUCCESS;
+}
+
+REGISTER_PASS(addInputPass)
Review comment:
Graph passes usage through optimize_for is already merged in #17885. We're not changing how graph passes are called in this PR
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #18779: Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-672106954
> Thanks Sam, I think the way to add new params is very clear
> just couple of questions / minor things about documentation on lib_pass/README.md
>
> * is it required the double connection between nodes? Here:
>
> ```
> n1->outputs.push_back({n2,1});
> n2->inputs.push_back({n1,0});
> ```
>
> will not be enough to do something like
> ` n2->inputs[1].node = n1`
>
> * Can you add the same figure in README as you have here?
> * Example call for adding params (alloc_arg) here has a different signature as in lib_pass/README.md (+- input_name)
Thanks Moises, I updated the PR description to fix the alloc_arg discrepancy and added the figure to the README. Per our offline discussion, `Graph` class has bidirectional connections so users should create connection in both ways when they add new nodes.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] Kh4L commented on a change in pull request #18779: Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r468341942
##########
File path: include/mxnet/lib_api.h
##########
@@ -748,62 +759,410 @@ struct JsonParser {
return JsonVal();
}
// generic parse function
- JsonVal parse(const std::string& json, unsigned int *idx) {
+ static JsonVal parse(const std::string& json, unsigned int *idx) {
JsonVal ret;
while (*idx < json.size()) {
if (json[*idx] == '"') {
++(*idx);
- ret = parse_string(json, idx);
+ ret = JsonVal::parse_string(json, idx);
} else if (json[*idx] >= '0' && json[*idx] <= '9') {
- ret = parse_num(json, idx);
+ ret = JsonVal::parse_num(json, idx);
} else if (json[*idx] == '[') {
++(*idx);
- ret = parse_list(json, idx);
+ ret = JsonVal::parse_list(json, idx);
} else if (json[*idx] == '{') {
++(*idx);
- ret = parse_map(json, idx);
+ ret = JsonVal::parse_map(json, idx);
} else if (json[*idx] == ']' || json[*idx] == '}') {return ret;}
if (ret.type != ERR) return ret;
++(*idx);
}
return ret;
}
- // convert JSON object back to JSON-compatible string
- std::string dump(const JsonVal &val) {
+ // debug function to convert data structure to a debugstring
+ std::string toString() const {
std::string ret;
- switch (val.type) {
+ switch (type) {
case ERR:
ret = "json(Error)";
break;
case STR:
- ret = "\"" + val.str + "\"";
+ ret = "json(STR:" + str + ")";
break;
case NUM:
- ret = val.str;
+ ret = "json(INT:" + str + ")";
break;
case LIST:
- ret = "[";
- for (unsigned i=0; i < val.list.size(); i++) {
- auto &item = val.list[i];
- ret += dump(item);
- if (i < val.list.size()-1)
- ret += ",";
- }
- ret += "]";
+ ret = "json(LIST:[";
+ for (auto &item : list)
+ ret += item.toString() + ",";
+ ret += "])";
break;
case MAP:
- ret = "{";
- unsigned cnt = 0;
- for (auto &item : val.map) {
- ret += dump(item.first) + " : " + dump(item.second);
- if (cnt++ < val.map.size()-1)
- ret += ",";
- }
- ret += "}";
+ ret = "json(MAP:{";
+ for (auto &item : map)
+ ret += item.first.toString() + " : " + item.second.toString() + ",";
+ ret += "})";
break;
}
return ret;
}
+ JsonType type;
+ int num;
+ std::string str;
+ std::vector<JsonVal> list;
+ std::map<JsonVal, JsonVal> map;
+};
+
+/*!
+ * \brief Graph utility to parse serialized subgraph symbol
+ */
+class Node;
+class Graph;
+
+// Representation of an input/output to a node
+struct NodeEntry {
+ Node* node; // other node thats producing/consuming inputs/outputs
+ int entry; // entry index from other node (ie. output index from producing node)
+};
+
+// Representation of a node in the graph
+class Node {
+ public:
+ Node() {tensor = nullptr;}
+ // internally set passResource to enable tensor allocation for graph passes
+ void _setPassResource(PassResource* res_) {res = res_;}
+ /* \brief allocate an arg tensor for this node */
+ void alloc_arg(const std::vector<int64_t>& shapes,
+ const MXContext &ctx, MXDType dtype) {
+ if (!res)
+ throw std::runtime_error(
+ "Node not initialized. Cannot use alloc_arg outside of graph passes.");
+ tensor = res->alloc_arg(name, shapes, ctx, dtype);
+ }
+ /* \brief allocate an aux tensor for this node */
+ void alloc_aux(const std::vector<int64_t>& shapes,
+ const MXContext &ctx, MXDType dtype) {
+ if (!res)
+ throw std::runtime_error(
+ "Node not initialized. Cannot use alloc_aux outside of graph passes.");
+ tensor = res->alloc_aux(name, shapes, ctx, dtype);
+ }
+ std::string op; // operator name (ie. Convolution)
+ std::string name; // unique node name (ie. conv_0 or conv_1)
+ MXTensor* tensor; // tensor data for input nodes
+ std::vector<NodeEntry> inputs; // set of inputs to the node
+ std::vector<NodeEntry> outputs; // set of outputs from the node
+ std::vector<Graph*> subgraphs; // set of subgraphs within this node
+ std::unordered_map<std::string, std::string> attrs; // node attributes
+
+ private:
+ PassResource* res;
+};
+
+// Representation of the graph
+class Graph {
+ public:
+ Graph() : res(nullptr) {}
+ /* \brief deleted nodes when deleting the graph */
+ ~Graph() {
+ for (int i = 0; i < nodes.size(); i++)
+ delete nodes[i];
+ }
+
+ /* \brief create a graph object from an unparsed string */
+ static Graph* fromString(const std::string& json) {
+ JsonVal val = JsonVal::parse(json);
+ return fromJson(val);
+ }
+
+ /* \brief create a graph object from a parsed JSON object */
+ static Graph* fromJson(JsonVal val) {
+ // get nodes list
+ JsonVal nodes = val.map[JsonVal("nodes")];
+ Graph *g = new Graph();
+
+ std::map<int, Node*> nodeMap;
+ // loop over nodes
+ for (int i = 0; i < nodes.list.size(); i++) {
+ Node* n = new Node();
+ g->nodes.push_back(n);
+ JsonVal node = nodes.list[i];
+
+ // set the op info
+ n->op = node.map[JsonVal("op")].str;
+ n->name = node.map[JsonVal("name")].str;
+
+ // if op is null its an input to the graph
Review comment:
Nit: `it's`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r462563043
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -331,6 +332,46 @@ REGISTER_PARTITIONER(mySelect)
.setCreateSelector("strategy1", createSelector)
.setReviewSubgraph("strategy1", myReviewSubgraph);
+/* \brief a basic pass that adds a new input for subgraph ops */
+MXReturnValue addInputPass(const std::string& in_graph, const std::string** out_graph,
+ const std::unordered_map<std::string, std::string>& options,
+ const std::unordered_map<std::string, MXTensor>& args,
+ const std::unordered_map<std::string, MXTensor>& aux,
+ const PassResource& res) {
+ // convert graph from JSON string to Graph/Node data structure
+ Graph *g = Graph::fromString(in_graph);
+ //find node with '_custom_subgraph_op' op type
+ for(Node* n : g->nodes) {
+ if(n->op.compare("_custom_subgraph_op") == 0) {
+ //set extra input
+ n->attrs[MX_STR_EXTRA_INPUTS] = std::to_string(1);
+
+ //create a new input Node
+ Node* input = new Node();
+ std::string input_name = n->name + "_input";
+ input->name = input_name;
+ input->op = "null";
+ //add a new node in graph
+ g->nodes.push_back(input);
+ g->inputs.push_back(input);
+ //connect new input to node
+ input->outputs.push_back({n,(int)(n->inputs.size())});
+ //connect node to new input
+ n->inputs.push_back({input,0});
+ // add a corresponding tensor for this input
+ MXTensor* arg_ = res.alloc_arg(input_name,{1},MXContext::CPU(0),kFloat32);
+ }
+ }
+
+ //convert back to JSON string from Graph/Node
+ *out_graph = new std::string(g->toString());
+ return MX_SUCCESS;
+}
+
+REGISTER_PASS(addInputPass)
Review comment:
Graph passes usage through optimize_for is already merged in #17885. We're not changing how graph passes work in this PR
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -331,6 +330,46 @@ REGISTER_PARTITIONER(mySelect)
.setCreateSelector("strategy1", createSelector)
.setReviewSubgraph("strategy1", myReviewSubgraph);
+/* \brief a basic pass that adds a new input for subgraph ops */
+MXReturnValue addInputPass(const std::string& in_graph, const std::string** out_graph,
Review comment:
To add a new input you have to do 2 things:
1. Create the new node and add it to the graph:
```
Node* input = new Node();
input->name = "my_new_input";
input->op = "null";
g->nodes.push_back(input);
g->inputs.push_back(input);
```
2. Create a new tensor to correspond to the new node:
```
MXTensor* arg_ = res.alloc_arg("my_new_input",{1},MXContext::CPU(0),kFloat32);
```
Notice that the node name and tensor name must match.
> The new input has unknown and arbitrary tensor shape.
> Do we handle that somewhere ?
>
In step 2 above you specify the shape when creating the tensor. In the example its `{1}`.
> Also is this added to both the
> a) subgraph_op and
> b) the subgraph attr of the subgraph_op ?
>
> We probably do not want that in the latter.
>
After creating the node in the graph and the corresponding tensor, you'll probably want to connect that node to your subgraph node to do anything interesting with it like:
```
//connect new input to node
input->outputs.push_back({n,(int)(n->inputs.size())});
//connect node to new input
n->inputs.push_back({input,0});
```
Where `n` is your subgraph node.
Also, you'll need to set the attr on your subgraph op to make sure this input doesnt participate in shape/type inference like: `n->attrs[MX_STR_EXTRA_INPUTS] = std::to_string(1);`
> Also how is shape handled ?
> The input tensor can be an take arbitrary and unknown shape at this point in the flow.
> Once known, this shape is unknown to user and needs to be remembered.
> Therefore, this op likely needs the shape attr with a byte dtype.
> Do these already work with this update ? or is it upto the user to add ?
The example above shows setting `kFloat32` but you can also set:
```
enum MXDType {
kFloat32 = 0,
kFloat64 = 1,
kFloat16 = 2,
kUint8 = 3,
kInt32 = 4,
kInt8 = 5,
kInt64 = 6,
};
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-670843114
@eric-haibin-lin @MoisesHer @Kh4L @ptrendx @PatricZhao @ZhennanQin @mseth10 @rondogency All changes are now complete, docs are updated. Anxiously awaiting your review/feedback. Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] eric-haibin-lin commented on a change in pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
eric-haibin-lin commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r461288648
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1225,6 +1226,7 @@ def hybridize(self, active=True, backend=None, backend_opts=None, **kwargs):
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
+ clear : clears any previous optimizations
Review comment:
Is there a case where we want to call `hybridize(clear=False)`?
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1225,6 +1226,7 @@ def hybridize(self, active=True, backend=None, backend_opts=None, **kwargs):
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
+ clear : clears any previous optimizations
Review comment:
cool got it
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-665911763
Todo: https://github.com/apache/incubator-mxnet/pull/18815/files#diff-3ac18ad0381b998b34f8d31f6a566dc8
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
mseth10 commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-669774583
> @mseth10 just pushed some changes, take a look and let me know what you think. Now conversion from JSON string to Graph object happens in the callSupportedOps function, and the Graph object is passed to the supportedOps API directly. So users dont even need to know about JSON strings.
@samskalicky looks good to me, it's definitely a better user experience.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on pull request #18779: Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-674284570
great work!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] MoisesHer commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
MoisesHer commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-664816682
> @eric-haibin-lin @MoisesHer what do you think about changing the graph pass API from taking a String (symbol JSON) to a Graph object like:
From a user perspective (someone defining a pass) I think a graph is preferred. I guess the user will not have to re-implement class Graph anymore, and will skip conversion from String <-> graph
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky edited a comment on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-670334981
> > @eric-haibin-lin @MoisesHer what do you think about changing the graph pass API from taking a String (symbol JSON) to a Graph object like:
>
> From a user perspective (someone defining a pass) I think a graph is preferred. I guess the user will not have to re-implement class Graph anymore, and will skip conversion from String <-> graph
Can you review again? updated Graph to embed the args/aux as tensors within the appropriate Node's. And made it so that you can just call `node->alloc_arg/aux()` to allocate a tensor for a node in the graph. Per our offline discussion this simplifies graph passes significantly.
https://github.com/apache/incubator-mxnet/pull/18779/files#diff-6620d12b32554c7f5ae8a6d1b789aea3R324-R332
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
mseth10 commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-666549341
> @mseth10 @rondogency should we change `supportedOps` to use the `Graph` class too instead of passing the JSON string like:
>
> ```
> MXReturnValue mySupportedOps(const Graph& g,
> std::vector<int>* ids,
> const std::unordered_map<std::string, std::string>& options)
> ```
Would we need to provide users with a Graph parser then, just like we provide a JsonParser currently?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-663143189
Hey @samskalicky , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [centos-cpu, windows-gpu, centos-gpu, unix-cpu, sanity, edge, windows-cpu, clang, unix-gpu, website, miscellaneous]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] MoisesHer commented on pull request #18779: Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
MoisesHer commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-671738680
Thanks Sam, I think the way to add new params is very clear
just couple of questions / minor things about documentation on lib_pass/README.md
- is it required the double connection between nodes? Here:
```
n1->outputs.push_back({n2,1});
n2->inputs.push_back({n1,0});
```
will not be enough to do something like
` n2->inputs[1].node = n1`
- Can you add the same figure in README as you have here?
- Example call for adding params (alloc_arg) here has a different signature as in lib_pass/README.md (+- input_name)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky merged pull request #18779: Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky merged pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-669019248
@mseth10 just pushed some changes, take a look and let me know what you think. Now conversion from JSON string to Graph object happens in the callSupportedOps function, and the Graph object is passed to the supportedOps API directly. So users dont even need to know about JSON strings.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-670334981
> > @eric-haibin-lin @MoisesHer what do you think about changing the graph pass API from taking a String (symbol JSON) to a Graph object like:
>
> From a user perspective (someone defining a pass) I think a graph is preferred. I guess the user will not have to re-implement class Graph anymore, and will skip conversion from String <-> graph
Can you review again? updated Graph to embed the args/aux as tensors within the appropriate Node's. And made it so that you can just call `node->alloc_arg/aux()` to allocate a tensor for a node in the graph. Per our offline discussion this simplifies graph passes significantly.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-664723802
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] HahTK commented on a change in pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
HahTK commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r460655282
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -331,6 +330,46 @@ REGISTER_PARTITIONER(mySelect)
.setCreateSelector("strategy1", createSelector)
.setReviewSubgraph("strategy1", myReviewSubgraph);
+/* \brief a basic pass that adds a new input for subgraph ops */
+MXReturnValue addInputPass(const std::string& in_graph, const std::string** out_graph,
Review comment:
The new input has unknown and arbitrary tensor shape.
Do we handle that somewhere ?
Also is this added to both the
a) subgraph_op and
b) the subgraph attr of the subgraph_op ?
We probably do not want that in the latter.
Also how is shape handled ?
The input tensor can be an take arbitrary and unknown shape at this point in the flow.
Once known, this shape is unknown to user and needs to be remembered.
Therefore, this op likely needs the shape attr with a byte dtype.
Do these already work with this update ? or is it upto the user to add ?
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -331,6 +332,46 @@ REGISTER_PARTITIONER(mySelect)
.setCreateSelector("strategy1", createSelector)
.setReviewSubgraph("strategy1", myReviewSubgraph);
+/* \brief a basic pass that adds a new input for subgraph ops */
+MXReturnValue addInputPass(const std::string& in_graph, const std::string** out_graph,
+ const std::unordered_map<std::string, std::string>& options,
+ const std::unordered_map<std::string, MXTensor>& args,
+ const std::unordered_map<std::string, MXTensor>& aux,
+ const PassResource& res) {
+ // convert graph from JSON string to Graph/Node data structure
+ Graph *g = Graph::fromString(in_graph);
+ //find node with '_custom_subgraph_op' op type
+ for(Node* n : g->nodes) {
+ if(n->op.compare("_custom_subgraph_op") == 0) {
+ //set extra input
+ n->attrs[MX_STR_EXTRA_INPUTS] = std::to_string(1);
+
+ //create a new input Node
+ Node* input = new Node();
+ std::string input_name = n->name + "_input";
+ input->name = input_name;
+ input->op = "null";
+ //add a new node in graph
+ g->nodes.push_back(input);
+ g->inputs.push_back(input);
+ //connect new input to node
+ input->outputs.push_back({n,(int)(n->inputs.size())});
+ //connect node to new input
+ n->inputs.push_back({input,0});
+ // add a corresponding tensor for this input
+ MXTensor* arg_ = res.alloc_arg(input_name,{1},MXContext::CPU(0),kFloat32);
+ }
+ }
+
+ //convert back to JSON string from Graph/Node
+ *out_graph = new std::string(g->toString());
+ return MX_SUCCESS;
+}
+
+REGISTER_PASS(addInputPass)
Review comment:
I propose, not overloading the optime_for() path to apply custom passes.
Instead we should just have an API to apply_pass() or apply_pass_for().
This makes the intent clear and unambiguous.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r461313717
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1225,6 +1226,7 @@ def hybridize(self, active=True, backend=None, backend_opts=None, **kwargs):
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
+ clear : clears any previous optimizations
Review comment:
Yes, Previously hybridize used to clear the cachedOp and cached_graph on every call. In this PR we add a new argument clear that defaults to True to preserve current behavior. When set to False it allows re-using the previous cached_graph to chain multiple optimization calls together. Heres an example:
```
net = nn.SymbolBlock(sym, inputs)
net.optimize_for(x, backend="opt1")
net.optimize_for(x, backend="opt2", clear=False)
```
##########
File path: python/mxnet/gluon/block.py
##########
@@ -1225,6 +1226,7 @@ def hybridize(self, active=True, backend=None, backend_opts=None, **kwargs):
The name of backend, as registered in `SubgraphBackendRegistry`, default None
backend_opts : dict of user-specified options to pass to the backend for partitioning, optional
Passed on to `PrePartition` and `PostPartition` functions of `SubgraphProperty`
+ clear : clears any previous optimizations
Review comment:
Maybe the 1st call to `optimize_for` is to partition the graph, and the 2nd call would be to run a custom graph pass for example.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] samskalicky commented on pull request #18779: [WIP] Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#issuecomment-663755289
@MoisesHer @Kh4L @HahTK for review
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-mxnet] mseth10 commented on a change in pull request #18779: Support extra inputs for subgraph ops
Posted by GitBox <gi...@apache.org>.
mseth10 commented on a change in pull request #18779:
URL: https://github.com/apache/incubator-mxnet/pull/18779#discussion_r470249438
##########
File path: include/mxnet/lib_api.h
##########
@@ -748,62 +759,410 @@ struct JsonParser {
return JsonVal();
}
// generic parse function
- JsonVal parse(const std::string& json, unsigned int *idx) {
+ static JsonVal parse(const std::string& json, unsigned int *idx) {
JsonVal ret;
while (*idx < json.size()) {
if (json[*idx] == '"') {
++(*idx);
- ret = parse_string(json, idx);
+ ret = JsonVal::parse_string(json, idx);
} else if (json[*idx] >= '0' && json[*idx] <= '9') {
- ret = parse_num(json, idx);
+ ret = JsonVal::parse_num(json, idx);
} else if (json[*idx] == '[') {
++(*idx);
- ret = parse_list(json, idx);
+ ret = JsonVal::parse_list(json, idx);
} else if (json[*idx] == '{') {
++(*idx);
- ret = parse_map(json, idx);
+ ret = JsonVal::parse_map(json, idx);
} else if (json[*idx] == ']' || json[*idx] == '}') {return ret;}
if (ret.type != ERR) return ret;
++(*idx);
}
return ret;
}
- // convert JSON object back to JSON-compatible string
- std::string dump(const JsonVal &val) {
+ // debug function to convert data structure to a debugstring
+ std::string toString() const {
std::string ret;
- switch (val.type) {
+ switch (type) {
case ERR:
ret = "json(Error)";
break;
case STR:
- ret = "\"" + val.str + "\"";
+ ret = "json(STR:" + str + ")";
break;
case NUM:
- ret = val.str;
+ ret = "json(INT:" + str + ")";
break;
case LIST:
- ret = "[";
- for (unsigned i=0; i < val.list.size(); i++) {
- auto &item = val.list[i];
- ret += dump(item);
- if (i < val.list.size()-1)
- ret += ",";
- }
- ret += "]";
+ ret = "json(LIST:[";
+ for (auto &item : list)
+ ret += item.toString() + ",";
+ ret += "])";
break;
case MAP:
- ret = "{";
- unsigned cnt = 0;
- for (auto &item : val.map) {
- ret += dump(item.first) + " : " + dump(item.second);
- if (cnt++ < val.map.size()-1)
- ret += ",";
- }
- ret += "}";
+ ret = "json(MAP:{";
+ for (auto &item : map)
+ ret += item.first.toString() + " : " + item.second.toString() + ",";
+ ret += "})";
break;
}
return ret;
}
+ JsonType type;
+ int num;
+ std::string str;
+ std::vector<JsonVal> list;
+ std::map<JsonVal, JsonVal> map;
+};
+
+/*!
+ * \brief Graph utility to parse serialized subgraph symbol
+ */
+class Node;
+class Graph;
+
+// Representation of an input/output to a node
+struct NodeEntry {
+ Node* node; // other node thats producing/consuming inputs/outputs
+ int entry; // entry index from other node (ie. output index from producing node)
+};
+
+// Representation of a node in the graph
+class Node {
+ public:
+ Node() {tensor = nullptr;}
+ // internally set passResource to enable tensor allocation for graph passes
+ void _setPassResource(PassResource* res_) {res = res_;}
+ /* \brief allocate an arg tensor for this node */
+ void alloc_arg(const std::vector<int64_t>& shapes,
+ const MXContext &ctx, MXDType dtype) {
+ if (!res)
+ throw std::runtime_error(
+ "Node not initialized. Cannot use alloc_arg outside of graph passes.");
+ tensor = res->alloc_arg(name, shapes, ctx, dtype);
+ }
+ /* \brief allocate an aux tensor for this node */
+ void alloc_aux(const std::vector<int64_t>& shapes,
+ const MXContext &ctx, MXDType dtype) {
+ if (!res)
+ throw std::runtime_error(
+ "Node not initialized. Cannot use alloc_aux outside of graph passes.");
+ tensor = res->alloc_aux(name, shapes, ctx, dtype);
+ }
+ std::string op; // operator name (ie. Convolution)
+ std::string name; // unique node name (ie. conv_0 or conv_1)
+ MXTensor* tensor; // tensor data for input nodes
+ std::vector<NodeEntry> inputs; // set of inputs to the node
+ std::vector<NodeEntry> outputs; // set of outputs from the node
+ std::vector<Graph*> subgraphs; // set of subgraphs within this node
+ std::unordered_map<std::string, std::string> attrs; // node attributes
+
+ private:
+ PassResource* res;
+};
+
+// Representation of the graph
+class Graph {
+ public:
+ Graph() : res(nullptr) {}
+ /* \brief deleted nodes when deleting the graph */
+ ~Graph() {
+ for (int i = 0; i < nodes.size(); i++)
+ delete nodes[i];
+ }
+
+ /* \brief create a graph object from an unparsed string */
+ static Graph* fromString(const std::string& json) {
+ JsonVal val = JsonVal::parse(json);
+ return fromJson(val);
+ }
+
+ /* \brief create a graph object from a parsed JSON object */
+ static Graph* fromJson(JsonVal val) {
+ // get nodes list
+ JsonVal nodes = val.map[JsonVal("nodes")];
+ Graph *g = new Graph();
+
+ std::map<int, Node*> nodeMap;
+ // loop over nodes
+ for (int i = 0; i < nodes.list.size(); i++) {
+ Node* n = new Node();
+ g->nodes.push_back(n);
+ JsonVal node = nodes.list[i];
+
+ // set the op info
+ n->op = node.map[JsonVal("op")].str;
+ n->name = node.map[JsonVal("name")].str;
+
+ // if op is null it is an input to the graph
+ if (n->op.compare("null") == 0)
+ g->inputs.push_back(n);
+
+ // set attrs
+ JsonVal attributes = node.map[JsonVal("attrs")];
+ for (auto& kv : attributes.map) {
+ n->attrs[kv.first.str] = kv.second.str;
+ }
+
+ // set subgraphs, parsing each into a graph
+ if (node.map.count(JsonVal("subgraphs")) > 0) {
+ JsonVal subgraphs = node.map[JsonVal("subgraphs")];
+ for (auto &subgraph : subgraphs.list) {
+ n->subgraphs.push_back(fromJson(subgraph));
+ }
+ }
+
+ // set node inputs
+ JsonVal node_inputs = node.map[JsonVal("inputs")];
+ n->inputs.resize(node_inputs.list.size());
+ for (int j = 0; j < node_inputs.list.size(); j++) {
+ JsonVal input = node_inputs.list[j];
+ NodeEntry& entry = n->inputs[j];
+ // get pointer to other node
+ entry.node = nodeMap[input.list[0].num];
+ // get the other node's output index
+ entry.entry = input.list[1].num;
+ // set other nodes output as connected to this node
+ entry.node->outputs.push_back({n, j});
+ }
+ nodeMap[i] = n;
+ }
+
+ // set graph level outputs
+ JsonVal& heads = val.map[JsonVal("heads")];
+ g->outputs.resize(heads.list.size());
+ for (int i = 0; i < heads.list.size(); i++) {
+ JsonVal head = heads.list[i];
+ g->outputs[i].node = nodeMap[head.list[0].num];
+ g->outputs[i].entry = head.list[1].num;
+ }
+
+ // add all attributes to the graph
+ for (auto& kv : val.map) {
+ if (kv.first.str.compare("nodes") != 0 &&
+ kv.first.str.compare("heads") != 0 &&
+ kv.first.str.compare("node_row_ptr") != 0 &&
+ kv.first.str.compare("arg_nodes") != 0) {
+ g->attrs[kv.first.str] = kv.second;
+ }
+ }
+ return g;
+ }
+
+ /* \brief convert graph object back to JSON object */
+ JsonVal toJson() {
+ // top level object is a map
+ JsonVal val(MAP);
+
+ // add attributes
+ for (auto& kv : attrs) {
+ val.map[JsonVal(kv.first)] = kv.second;
+ }
+
+ // sort graph nodes in topological order, create mapping of node to index
+ std::map<Node*, int> nodeMap;
+ std::vector<Node*> sorted = topological_sort();
+ // nodes are in reverse topological order in the vector (back is first)
+ // so loop from end to front over the vector 'sorted'
+ for (int i = sorted.size()-1; i >= 0; i--) {
+ nodeMap[sorted[i]] = sorted.size()-1-i;
+ }
+
+ // create node_row_ptr entry
+ val.map[JsonVal("node_row_ptr")] = JsonVal(LIST);
+ JsonVal& node_row_ptr = val.map[JsonVal("node_row_ptr")];
+ for (int i = 0; i < nodes.size(); i++)
+ node_row_ptr.list.push_back(JsonVal(i));
+
+ // add all input nodes
+ val.map[JsonVal("arg_nodes")] = JsonVal(LIST);
+ JsonVal& arg_nodes = val.map[JsonVal("arg_nodes")];
+ for (int i = 0; i < inputs.size(); i++)
+ arg_nodes.list.push_back(JsonVal(nodeMap[inputs[i]]));
+
+ // add all output nodes
+ val.map[JsonVal("heads")] = JsonVal(LIST);
+ JsonVal& heads = val.map[JsonVal("heads")];
+ for (int i = 0; i < outputs.size(); i++) {
+ heads.list.push_back(JsonVal(LIST));
+ JsonVal& out = heads.list[i];
+ out.list.push_back(JsonVal(nodeMap[outputs[i].node]));
+ out.list.push_back(JsonVal(outputs[i].entry));
+ out.list.push_back(JsonVal(0));
+ }
+
+ // add all graph nodes
+ val.map[JsonVal("nodes")] = JsonVal(LIST);
+ JsonVal& nodes_ = val.map[JsonVal("nodes")];
+ for (int i = sorted.size()-1; i >= 0; i--) {
+ // each node is a map
+ nodes_.list.push_back(JsonVal(MAP));
+ Node* n = sorted[i];
+ JsonVal& n_ = nodes_.list[nodes_.list.size()-1];
+
+ n_.map[JsonVal("op")] = JsonVal(n->op);
+ n_.map[JsonVal("name")] = JsonVal(n->name);
+ n_.map[JsonVal("inputs")] = JsonVal(LIST);
+
+ // add inputs for this node
+ JsonVal& inputs_ = n_.map[JsonVal("inputs")];
+ for (int j = 0; j < n->inputs.size(); j++) {
+ inputs_.list.push_back(JsonVal(LIST));
+ NodeEntry& entry = n->inputs[j];
+ JsonVal& in = inputs_.list[j];
+ in.list.push_back(JsonVal(nodeMap[entry.node]));
+ in.list.push_back(JsonVal(entry.entry));
+ in.list.push_back(JsonVal(0));
+ }
+
+ // add subgraphs for this node, convert each back to JSON
+ if (n->subgraphs.size() > 0) {
+ n_.map[JsonVal("subgraphs")] = JsonVal(LIST);
+ JsonVal &subgraphs_ = n_.map[JsonVal("subgraphs")];
+ for (Graph *subgraph : n->subgraphs) {
+ subgraphs_.list.push_back(subgraph->toJson());
+ }
+ }
+
+ // add attributes for this node
+ n_.map[JsonVal("attrs")] = JsonVal(MAP);
+ JsonVal& attrs_ = n_.map[JsonVal("attrs")];
+ for (auto& kv : n->attrs) {
+ attrs_.map[JsonVal(kv.first)] = JsonVal(kv.second);
+ }
+ }
+ return val;
+ }
+
+ /* \brief convert graph object to JSON string */
+ std::string toString() {
+ return toJson().dump();
+ }
+
+ /* \brief visits a node "n" */
+ void _dfs_util(Node* n, std::unordered_set<Node*>* to_visit,
+ std::function<void(Node*)> handler) const {
+ to_visit->erase(n); // remove node now that we're visiting it
+ for (NodeEntry& e : n->outputs) {
+ Node* o = e.node;
+ if (to_visit->count(o) != 0) {
+ _dfs_util(o, to_visit, handler); // visit neighbor
+ }
+ }
+ handler(n); // post-order visit this node
+ }
+
+ /* \brief post-order DFS graph traversal */
+ void DFS(std::function<void(Node*)> handler) const {
+ std::unordered_set<Node*> to_visit;
+ // put all nodes in set to visit
+ for (auto& n : nodes)
+ to_visit.insert(n);
+ // visit all inputs first
+ for (auto& i : inputs)
+ if (to_visit.count(i) != 0)
+ _dfs_util(i, &to_visit, handler);
+ // visit any nodes left
+ while (to_visit.size() > 0)
+ _dfs_util(*(to_visit.begin()), &to_visit, handler);
+ }
+
+ /* \brief sort graph nodes in topological order */
+ std::vector<Node*> topological_sort() const {
+ std::vector<Node*> sorted;
+ auto handler = [&](Node* n) {
+ sorted.push_back(n); // when visiting each node, add it in order to the vector
+ };
+ DFS(handler);
+ return sorted;
+ }
+
+ /* \brief print out graph details */
+ void print(int indent = 0) const {
+ std::string space = "";
+ for (int i = 0; i < indent; i++) space+=" ";
+
+ std::cout << space << "########### Graph #############" << std::endl;
+ std::cout << space << "attributes: " << std::endl;
+ for (auto &kv : attrs)
+ std::cout << space << "\t" << kv.first << " : " << kv.second.str << std::endl;
+ std::cout << space << "inputs: " << inputs.size() << std::endl;
+ std::cout << space << "outputs: " << outputs.size() << std::endl;
+ std::cout << space << "nodes: " << nodes.size() << std::endl;
+ std::vector<Node*> sorted = topological_sort();
+ // loop over each node and print out its inputs/outputs
+ for (int i = sorted.size()-1; i >= 0; i--) {
+ std::cout << space << "Node: " << sorted[i]->name << std::endl;
+ for (int j = 0; j < sorted[i]->inputs.size(); j++) {
+ std::cout << space << "\tInput: " << sorted[i]->inputs[j].node->name << " "
+ << sorted[i]->inputs[j].entry << std::endl;
+ }
+ for (int j = 0; j < sorted[i]->outputs.size(); j++) {
+ std::cout << space << "\tOutput: " << sorted[i]->outputs[j].node->name << " "
+ << sorted[i]->outputs[j].entry << std::endl;
+ }
+ if (sorted[i]->subgraphs.size() > 0) {
+ for (auto &subgraph : sorted[i]->subgraphs) {
+ std::cout << space << "\tSubgraph:" << std::endl;
+ subgraph->print(indent+2);
+ }
+ }
+ }
+ std::cout << space << "###############################" << std::endl;
+ }
+
+ /* \brief add a new node to this graph */
+ Node* addNode(const std::string& name, const std::string& op) {
+ Node* n = new Node();
+ n->name = name;
+ n->op = op;
+ if (res)
+ n->_setPassResource(res);
+ return n;
+ }
+ /* \brief get node at index in graph */
+ Node* getNode(size_t idx) {
Review comment:
why do we need two `getNode` functions?
----------------------------------------------------------------
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:
users@infra.apache.org