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/08/10 20:27:08 UTC
[GitHub] [incubator-mxnet] samskalicky opened a new pull request #18894: [1.x] Backporting #18779 to v1.x
samskalicky opened a new pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894
## Description ##
Backporting #18779 to v1.x
## 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 pull request #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#issuecomment-674579460
> You need the link fix from [7a84fe1](https://github.com/apache/incubator-mxnet/commit/7a84fe11dd7399d1c2bdd0fb679c46af69e48861)
Thanks! I created #18945 for the v1.x branch
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#issuecomment-671572695
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**: [unix-gpu, website, centos-cpu, clang, miscellaneous, unix-cpu, windows-cpu, edge, centos-gpu, windows-gpu, sanity]
***
_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] samskalicky commented on a change in pull request #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472402371
##########
File path: example/extensions/lib_custom_op/transposerowsp_lib.cc
##########
@@ -73,11 +75,11 @@ MXReturnValue forward(const std::unordered_map<std::string, std::string>& attrs,
// The data types and storage types of inputs and outputs should be the same.
if(inputs->at(0).dtype != outputs->at(0).dtype ||
inputs->at(0).stype != outputs->at(0).stype) {
- std::cout << "Error! Expected all inputs and outputs to be the same type."
- << "Found input storage type:" << inputs->at(0).stype
- << " Found output storage type:" << outputs->at(0).stype
- << " Found input data type:" << inputs->at(0).dtype
- << " Found output data type:" << outputs->at(0).dtype << std::endl;
+ MX_ERROR_MSG << "Error! Expected all inputs and outputs to be the same type."
+ << "Found input storage type:" << inputs->at(0).stype
+ << " Found output storage type:" << outputs->at(0).stype
Review comment:
> LGTM! with some nit
Thanks! will add these todos to my next 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 a change in pull request #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472514368
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -176,70 +174,42 @@ REGISTER_OP(_custom_subgraph_op)
const std::vector<std::string> op_names({"exp","log"});
-MXReturnValue mySupportedOps(const std::string& json,
+MXReturnValue mySupportedOps(const mxnet::ext::Graph* graph,
std::vector<int>* ids,
const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- //convert json string to json object
- JsonParser parser;
- JsonVal json_val = parser.parse_to_json(json);
- //get nodes list
- JsonVal nodes = json_val.map[JsonVal("nodes")];
//loop over nodes
- for(int i=0; i<nodes.list.size(); i++) {
- JsonVal node = nodes.list[i];
- JsonVal op = node.map[JsonVal("op")];
+ for(int i=0; i<graph->size(); i++) {
+ const mxnet::ext::Node *node = graph->getNode(i);
//get shape/type if available
std::string shape;
int dtype = -1;
- if(node.map.find(JsonVal("attrs")) != node.map.end()) {
- JsonVal attrs = node.map[JsonVal("attrs")];
- if(attrs.map.find(JsonVal("shape")) != attrs.map.end())
- shape = attrs.map[JsonVal("shape")].str;
- if(attrs.map.find(JsonVal("dtype")) != attrs.map.end())
- dtype = std::stoi(attrs.map[JsonVal("dtype")].str);
- }
+ if(node->attrs.count("shape") > 0)
+ shape = node->attrs.at("shape");
+ if(node->attrs.count("dtype") > 0)
+ dtype = std::stoi(node->attrs.at("dtype"));
//check if op dtype is float, and if option was specified to require float types
if((dtype == kFloat32 && options.count("reqFloat") > 0) || options.count("reqFloat") == 0) {
- //check if op is in whitelist
- if(std::find(op_names.begin(),op_names.end(),op.str.c_str()) != op_names.end()) {
- // found op in whitelist, set value to -1 to include op in any subgraph
+ //check if op is in allowlist
+ if(std::find(op_names.begin(),op_names.end(),node->op.c_str()) != op_names.end()) {
+ // found op in allowlist, set value to -1 to include op in any subgraph
ids->at(i) = -1;
}
}
}
return MX_SUCCESS;
}
-MXReturnValue myReviewSubgraph(const std::string& json, int subgraph_id, bool* accept,
- const std::unordered_map<std::string, std::string>& options,
- std::unordered_map<std::string, std::string>* attrs,
- const std::unordered_map<std::string, MXTensor>& args,
- const std::unordered_map<std::string, MXTensor>& aux) {
+MXReturnValue myReviewSubgraph(const mxnet::ext::Graph *subgraph, int subgraph_id, bool* accept,
+ const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- for (auto kv : args) {
- std::cout << "arg: " << kv.first << " ==> (";
- for (auto s : kv.second.shape)
- std::cout << s << ",";
- std::cout << ") [";
- for (int i=0; i<kv.second.size(); i++)
- std::cout << kv.second.data<float>()[i] << ", ";
- std::cout << "]" << std::endl;
- }
-
- // check if option `reqArgs` was specified, and if so check if args were provided
Review comment:
Plus, now the args are not available in a separate map, they are embedded within the Node objects in the graph (ie. Node->tensor)
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#issuecomment-674560306
I keep getting this weird Clojure failure:
```
[2020-08-16T17:41:29.383Z] lein test :only bert.bert-sentence-classification-test/train-test
[2020-08-16T17:41:29.383Z]
[2020-08-16T17:41:29.383Z] FAIL in (train-test) (bert_sentence_classification_test.clj:90)
[2020-08-16T17:41:29.383Z] accuracy
[2020-08-16T17:41:29.383Z] expected: (< 0.5 (last (m/score model {:eval-data train-data, :eval-metric (eval-metric/accuracy)})))
[2020-08-16T17:41:29.383Z] actual: (not (< 0.5 ##NaN))
[2020-08-16T17:41:29.383Z] WARN org.apache.mxnet.DataDesc: Found Undefined Layout, will use default index 0 for batch axis
```
I did a rebase, but not sure why this is failing. None of my changes affect internal operators...
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky merged pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
HahTK commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472512874
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -176,70 +174,42 @@ REGISTER_OP(_custom_subgraph_op)
const std::vector<std::string> op_names({"exp","log"});
-MXReturnValue mySupportedOps(const std::string& json,
+MXReturnValue mySupportedOps(const mxnet::ext::Graph* graph,
std::vector<int>* ids,
const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- //convert json string to json object
- JsonParser parser;
- JsonVal json_val = parser.parse_to_json(json);
- //get nodes list
- JsonVal nodes = json_val.map[JsonVal("nodes")];
//loop over nodes
- for(int i=0; i<nodes.list.size(); i++) {
- JsonVal node = nodes.list[i];
- JsonVal op = node.map[JsonVal("op")];
+ for(int i=0; i<graph->size(); i++) {
+ const mxnet::ext::Node *node = graph->getNode(i);
//get shape/type if available
std::string shape;
int dtype = -1;
- if(node.map.find(JsonVal("attrs")) != node.map.end()) {
- JsonVal attrs = node.map[JsonVal("attrs")];
- if(attrs.map.find(JsonVal("shape")) != attrs.map.end())
- shape = attrs.map[JsonVal("shape")].str;
- if(attrs.map.find(JsonVal("dtype")) != attrs.map.end())
- dtype = std::stoi(attrs.map[JsonVal("dtype")].str);
- }
+ if(node->attrs.count("shape") > 0)
+ shape = node->attrs.at("shape");
+ if(node->attrs.count("dtype") > 0)
+ dtype = std::stoi(node->attrs.at("dtype"));
//check if op dtype is float, and if option was specified to require float types
if((dtype == kFloat32 && options.count("reqFloat") > 0) || options.count("reqFloat") == 0) {
- //check if op is in whitelist
- if(std::find(op_names.begin(),op_names.end(),op.str.c_str()) != op_names.end()) {
- // found op in whitelist, set value to -1 to include op in any subgraph
+ //check if op is in allowlist
+ if(std::find(op_names.begin(),op_names.end(),node->op.c_str()) != op_names.end()) {
+ // found op in allowlist, set value to -1 to include op in any subgraph
ids->at(i) = -1;
}
}
}
return MX_SUCCESS;
}
-MXReturnValue myReviewSubgraph(const std::string& json, int subgraph_id, bool* accept,
- const std::unordered_map<std::string, std::string>& options,
- std::unordered_map<std::string, std::string>* attrs,
- const std::unordered_map<std::string, MXTensor>& args,
- const std::unordered_map<std::string, MXTensor>& aux) {
+MXReturnValue myReviewSubgraph(const mxnet::ext::Graph *subgraph, int subgraph_id, bool* accept,
+ const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- for (auto kv : args) {
- std::cout << "arg: " << kv.first << " ==> (";
- for (auto s : kv.second.shape)
- std::cout << s << ",";
- std::cout << ") [";
- for (int i=0; i<kv.second.size(); i++)
- std::cout << kv.second.data<float>()[i] << ", ";
- std::cout << "]" << std::endl;
- }
-
- // check if option `reqArgs` was specified, and if so check if args were provided
Review comment:
Should we not keep this for 1.8 which does support non gluon API ?
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#issuecomment-675292964
@HahTK @Kh4L Can you take a quick peek to make sure this PR looks good on 1.x branch? I had to make some tweaks
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472511001
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -331,12 +290,41 @@ REGISTER_PARTITIONER(mySelect)
.setCreateSelector("strategy1", createSelector)
.setReviewSubgraph("strategy1", myReviewSubgraph);
+/* \brief a basic pass that adds a new input for subgraph ops */
+MXReturnValue addInputPass(mxnet::ext::Graph *graph,
Review comment:
Sure, this is just an example. Not code users will use to do anything real.
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
Kh4L commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472391503
##########
File path: example/extensions/lib_custom_op/gemm_lib.cc
##########
@@ -159,7 +161,7 @@ MXReturnValue inferShape(const std::unordered_map<std::string, std::string>& att
unsigned kk = inshapes->at(1)[0];
unsigned m = inshapes->at(1)[1];
if (k != kk) {
- std::cout << "Exected first input axis 1 equals to second input axis 0" << std::endl;
+ MX_ERROR_MSG << "Exected first input axis 1 equals to second input axis 0";
Review comment:
```suggestion
MX_ERROR_MSG << "Expected first input axis 1 equals to second input axis 0";
```
##########
File path: example/extensions/lib_custom_op/transposerowsp_lib.cc
##########
@@ -73,11 +75,11 @@ MXReturnValue forward(const std::unordered_map<std::string, std::string>& attrs,
// The data types and storage types of inputs and outputs should be the same.
if(inputs->at(0).dtype != outputs->at(0).dtype ||
inputs->at(0).stype != outputs->at(0).stype) {
- std::cout << "Error! Expected all inputs and outputs to be the same type."
- << "Found input storage type:" << inputs->at(0).stype
- << " Found output storage type:" << outputs->at(0).stype
- << " Found input data type:" << inputs->at(0).dtype
- << " Found output data type:" << outputs->at(0).dtype << std::endl;
+ MX_ERROR_MSG << "Error! Expected all inputs and outputs to be the same type."
+ << "Found input storage type:" << inputs->at(0).stype
+ << " Found output storage type:" << outputs->at(0).stype
Review comment:
Unwanted whitespaces?
----------------------------------------------------------------
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 pull request #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
HahTK commented on pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#issuecomment-671591625
What is the timeline for this fix and MXNet 1.8 ?
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472514368
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -176,70 +174,42 @@ REGISTER_OP(_custom_subgraph_op)
const std::vector<std::string> op_names({"exp","log"});
-MXReturnValue mySupportedOps(const std::string& json,
+MXReturnValue mySupportedOps(const mxnet::ext::Graph* graph,
std::vector<int>* ids,
const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- //convert json string to json object
- JsonParser parser;
- JsonVal json_val = parser.parse_to_json(json);
- //get nodes list
- JsonVal nodes = json_val.map[JsonVal("nodes")];
//loop over nodes
- for(int i=0; i<nodes.list.size(); i++) {
- JsonVal node = nodes.list[i];
- JsonVal op = node.map[JsonVal("op")];
+ for(int i=0; i<graph->size(); i++) {
+ const mxnet::ext::Node *node = graph->getNode(i);
//get shape/type if available
std::string shape;
int dtype = -1;
- if(node.map.find(JsonVal("attrs")) != node.map.end()) {
- JsonVal attrs = node.map[JsonVal("attrs")];
- if(attrs.map.find(JsonVal("shape")) != attrs.map.end())
- shape = attrs.map[JsonVal("shape")].str;
- if(attrs.map.find(JsonVal("dtype")) != attrs.map.end())
- dtype = std::stoi(attrs.map[JsonVal("dtype")].str);
- }
+ if(node->attrs.count("shape") > 0)
+ shape = node->attrs.at("shape");
+ if(node->attrs.count("dtype") > 0)
+ dtype = std::stoi(node->attrs.at("dtype"));
//check if op dtype is float, and if option was specified to require float types
if((dtype == kFloat32 && options.count("reqFloat") > 0) || options.count("reqFloat") == 0) {
- //check if op is in whitelist
- if(std::find(op_names.begin(),op_names.end(),op.str.c_str()) != op_names.end()) {
- // found op in whitelist, set value to -1 to include op in any subgraph
+ //check if op is in allowlist
+ if(std::find(op_names.begin(),op_names.end(),node->op.c_str()) != op_names.end()) {
+ // found op in allowlist, set value to -1 to include op in any subgraph
ids->at(i) = -1;
}
}
}
return MX_SUCCESS;
}
-MXReturnValue myReviewSubgraph(const std::string& json, int subgraph_id, bool* accept,
- const std::unordered_map<std::string, std::string>& options,
- std::unordered_map<std::string, std::string>* attrs,
- const std::unordered_map<std::string, MXTensor>& args,
- const std::unordered_map<std::string, MXTensor>& aux) {
+MXReturnValue myReviewSubgraph(const mxnet::ext::Graph *subgraph, int subgraph_id, bool* accept,
+ const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- for (auto kv : args) {
- std::cout << "arg: " << kv.first << " ==> (";
- for (auto s : kv.second.shape)
- std::cout << s << ",";
- std::cout << ") [";
- for (int i=0; i<kv.second.size(); i++)
- std::cout << kv.second.data<float>()[i] << ", ";
- std::cout << "]" << std::endl;
- }
-
- // check if option `reqArgs` was specified, and if so check if args were provided
Review comment:
Plus, now the args are not available in a separate map, they are embedded within the Node objects in the graph (ie. Node->tensor). So theres not an easy way to check for args anymore
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#issuecomment-674563039
You need the link fix from https://github.com/apache/incubator-mxnet/commit/7a84fe11dd7399d1c2bdd0fb679c46af69e48861
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
HahTK commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472436593
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -331,12 +290,41 @@ REGISTER_PARTITIONER(mySelect)
.setCreateSelector("strategy1", createSelector)
.setReviewSubgraph("strategy1", myReviewSubgraph);
+/* \brief a basic pass that adds a new input for subgraph ops */
+MXReturnValue addInputPass(mxnet::ext::Graph *graph,
Review comment:
This seems to add an input to all custom subgraph ops.
Will this not cause problems if there is more than one type of custom subgraph op defined ?
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -176,70 +174,42 @@ REGISTER_OP(_custom_subgraph_op)
const std::vector<std::string> op_names({"exp","log"});
-MXReturnValue mySupportedOps(const std::string& json,
+MXReturnValue mySupportedOps(const mxnet::ext::Graph* graph,
std::vector<int>* ids,
const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- //convert json string to json object
- JsonParser parser;
- JsonVal json_val = parser.parse_to_json(json);
- //get nodes list
- JsonVal nodes = json_val.map[JsonVal("nodes")];
//loop over nodes
- for(int i=0; i<nodes.list.size(); i++) {
- JsonVal node = nodes.list[i];
- JsonVal op = node.map[JsonVal("op")];
+ for(int i=0; i<graph->size(); i++) {
+ const mxnet::ext::Node *node = graph->getNode(i);
//get shape/type if available
std::string shape;
int dtype = -1;
- if(node.map.find(JsonVal("attrs")) != node.map.end()) {
- JsonVal attrs = node.map[JsonVal("attrs")];
- if(attrs.map.find(JsonVal("shape")) != attrs.map.end())
- shape = attrs.map[JsonVal("shape")].str;
- if(attrs.map.find(JsonVal("dtype")) != attrs.map.end())
- dtype = std::stoi(attrs.map[JsonVal("dtype")].str);
- }
+ if(node->attrs.count("shape") > 0)
+ shape = node->attrs.at("shape");
+ if(node->attrs.count("dtype") > 0)
+ dtype = std::stoi(node->attrs.at("dtype"));
//check if op dtype is float, and if option was specified to require float types
if((dtype == kFloat32 && options.count("reqFloat") > 0) || options.count("reqFloat") == 0) {
- //check if op is in whitelist
- if(std::find(op_names.begin(),op_names.end(),op.str.c_str()) != op_names.end()) {
- // found op in whitelist, set value to -1 to include op in any subgraph
+ //check if op is in allowlist
+ if(std::find(op_names.begin(),op_names.end(),node->op.c_str()) != op_names.end()) {
+ // found op in allowlist, set value to -1 to include op in any subgraph
ids->at(i) = -1;
}
}
}
return MX_SUCCESS;
}
-MXReturnValue myReviewSubgraph(const std::string& json, int subgraph_id, bool* accept,
- const std::unordered_map<std::string, std::string>& options,
- std::unordered_map<std::string, std::string>* attrs,
- const std::unordered_map<std::string, MXTensor>& args,
- const std::unordered_map<std::string, MXTensor>& aux) {
+MXReturnValue myReviewSubgraph(const mxnet::ext::Graph *subgraph, int subgraph_id, bool* accept,
+ const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- for (auto kv : args) {
- std::cout << "arg: " << kv.first << " ==> (";
- for (auto s : kv.second.shape)
- std::cout << s << ",";
- std::cout << ") [";
- for (int i=0; i<kv.second.size(); i++)
- std::cout << kv.second.data<float>()[i] << ", ";
- std::cout << "]" << std::endl;
- }
-
- // check if option `reqArgs` was specified, and if so check if args were provided
Review comment:
Does the concept of required args go away for some reason ?
----------------------------------------------------------------
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 pull request #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
HahTK commented on pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#issuecomment-671591208
Large scale cloud end users would need this feature in 1.8.
I propose we add tests for this feature for all 1.x APIs as part of this backport
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#issuecomment-674355319
> Large scale cloud end users would need this feature in 1.8.
> I propose we add tests for this feature for all 1.x APIs as part of this backport
Hi @HahTK the PR on master was merged, and I cherry picked the commit into this branch. Notice that I left all the previous 1.x API tests (that were removed in the PR on master).
> What is the timeline for this fix and MXNet 1.8 ?
merging this PR should be quick, get the CI to pass, one review from someone to check for missing things in 1.x and it should be good to go.
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472514368
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -176,70 +174,42 @@ REGISTER_OP(_custom_subgraph_op)
const std::vector<std::string> op_names({"exp","log"});
-MXReturnValue mySupportedOps(const std::string& json,
+MXReturnValue mySupportedOps(const mxnet::ext::Graph* graph,
std::vector<int>* ids,
const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- //convert json string to json object
- JsonParser parser;
- JsonVal json_val = parser.parse_to_json(json);
- //get nodes list
- JsonVal nodes = json_val.map[JsonVal("nodes")];
//loop over nodes
- for(int i=0; i<nodes.list.size(); i++) {
- JsonVal node = nodes.list[i];
- JsonVal op = node.map[JsonVal("op")];
+ for(int i=0; i<graph->size(); i++) {
+ const mxnet::ext::Node *node = graph->getNode(i);
//get shape/type if available
std::string shape;
int dtype = -1;
- if(node.map.find(JsonVal("attrs")) != node.map.end()) {
- JsonVal attrs = node.map[JsonVal("attrs")];
- if(attrs.map.find(JsonVal("shape")) != attrs.map.end())
- shape = attrs.map[JsonVal("shape")].str;
- if(attrs.map.find(JsonVal("dtype")) != attrs.map.end())
- dtype = std::stoi(attrs.map[JsonVal("dtype")].str);
- }
+ if(node->attrs.count("shape") > 0)
+ shape = node->attrs.at("shape");
+ if(node->attrs.count("dtype") > 0)
+ dtype = std::stoi(node->attrs.at("dtype"));
//check if op dtype is float, and if option was specified to require float types
if((dtype == kFloat32 && options.count("reqFloat") > 0) || options.count("reqFloat") == 0) {
- //check if op is in whitelist
- if(std::find(op_names.begin(),op_names.end(),op.str.c_str()) != op_names.end()) {
- // found op in whitelist, set value to -1 to include op in any subgraph
+ //check if op is in allowlist
+ if(std::find(op_names.begin(),op_names.end(),node->op.c_str()) != op_names.end()) {
+ // found op in allowlist, set value to -1 to include op in any subgraph
ids->at(i) = -1;
}
}
}
return MX_SUCCESS;
}
-MXReturnValue myReviewSubgraph(const std::string& json, int subgraph_id, bool* accept,
- const std::unordered_map<std::string, std::string>& options,
- std::unordered_map<std::string, std::string>* attrs,
- const std::unordered_map<std::string, MXTensor>& args,
- const std::unordered_map<std::string, MXTensor>& aux) {
+MXReturnValue myReviewSubgraph(const mxnet::ext::Graph *subgraph, int subgraph_id, bool* accept,
+ const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- for (auto kv : args) {
- std::cout << "arg: " << kv.first << " ==> (";
- for (auto s : kv.second.shape)
- std::cout << s << ",";
- std::cout << ") [";
- for (int i=0; i<kv.second.size(); i++)
- std::cout << kv.second.data<float>()[i] << ", ";
- std::cout << "]" << std::endl;
- }
-
- // check if option `reqArgs` was specified, and if so check if args were provided
Review comment:
Plus, now the args are not available in a separate map, they are embedded within the Node objects in the graph (ie. Node->tensor). So theres not an easy way to check for args anymore. Instead, as you need the tensor for a particular param (by calling node.tensor) you can check and see if the tensor is `nullptr` or not.
----------------------------------------------------------------
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 #18894: [1.x] Backporting #18779 to v1.x
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #18894:
URL: https://github.com/apache/incubator-mxnet/pull/18894#discussion_r472511650
##########
File path: example/extensions/lib_subgraph/subgraph_lib.cc
##########
@@ -176,70 +174,42 @@ REGISTER_OP(_custom_subgraph_op)
const std::vector<std::string> op_names({"exp","log"});
-MXReturnValue mySupportedOps(const std::string& json,
+MXReturnValue mySupportedOps(const mxnet::ext::Graph* graph,
std::vector<int>* ids,
const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- //convert json string to json object
- JsonParser parser;
- JsonVal json_val = parser.parse_to_json(json);
- //get nodes list
- JsonVal nodes = json_val.map[JsonVal("nodes")];
//loop over nodes
- for(int i=0; i<nodes.list.size(); i++) {
- JsonVal node = nodes.list[i];
- JsonVal op = node.map[JsonVal("op")];
+ for(int i=0; i<graph->size(); i++) {
+ const mxnet::ext::Node *node = graph->getNode(i);
//get shape/type if available
std::string shape;
int dtype = -1;
- if(node.map.find(JsonVal("attrs")) != node.map.end()) {
- JsonVal attrs = node.map[JsonVal("attrs")];
- if(attrs.map.find(JsonVal("shape")) != attrs.map.end())
- shape = attrs.map[JsonVal("shape")].str;
- if(attrs.map.find(JsonVal("dtype")) != attrs.map.end())
- dtype = std::stoi(attrs.map[JsonVal("dtype")].str);
- }
+ if(node->attrs.count("shape") > 0)
+ shape = node->attrs.at("shape");
+ if(node->attrs.count("dtype") > 0)
+ dtype = std::stoi(node->attrs.at("dtype"));
//check if op dtype is float, and if option was specified to require float types
if((dtype == kFloat32 && options.count("reqFloat") > 0) || options.count("reqFloat") == 0) {
- //check if op is in whitelist
- if(std::find(op_names.begin(),op_names.end(),op.str.c_str()) != op_names.end()) {
- // found op in whitelist, set value to -1 to include op in any subgraph
+ //check if op is in allowlist
+ if(std::find(op_names.begin(),op_names.end(),node->op.c_str()) != op_names.end()) {
+ // found op in allowlist, set value to -1 to include op in any subgraph
ids->at(i) = -1;
}
}
}
return MX_SUCCESS;
}
-MXReturnValue myReviewSubgraph(const std::string& json, int subgraph_id, bool* accept,
- const std::unordered_map<std::string, std::string>& options,
- std::unordered_map<std::string, std::string>* attrs,
- const std::unordered_map<std::string, MXTensor>& args,
- const std::unordered_map<std::string, MXTensor>& aux) {
+MXReturnValue myReviewSubgraph(const mxnet::ext::Graph *subgraph, int subgraph_id, bool* accept,
+ const std::unordered_map<std::string, std::string>& options) {
for (auto kv : options) {
std::cout << "option: " << kv.first << " ==> " << kv.second << std::endl;
}
- for (auto kv : args) {
- std::cout << "arg: " << kv.first << " ==> (";
- for (auto s : kv.second.shape)
- std::cout << s << ",";
- std::cout << ") [";
- for (int i=0; i<kv.second.size(); i++)
- std::cout << kv.second.data<float>()[i] << ", ";
- std::cout << "]" << std::endl;
- }
-
- // check if option `reqArgs` was specified, and if so check if args were provided
Review comment:
This wasnt required in 2.0 since we were deprecating Symbol/Module APIs. In Gluon args are always available so there was no calling `optimize_for` with/without args, only with.
----------------------------------------------------------------
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