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 2019/12/30 16:32:14 UTC
[GitHub] [incubator-mxnet] samskalicky opened a new pull request #17194:
[WIP] enhancements for custom sugraph op
samskalicky opened a new pull request #17194: [WIP] enhancements for custom sugraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194
## Description ##
Enhancements for custom subgraph operators in external libraries.
- Adds API to set boolean flag to denote a subgraph operator in an external library
- Removes the requirement to manually implement infer shape/type/storage/mutate/request
- Uses default functions from subgraph/common.h
## Checklist ##
### Essentials ###
Please feel free to remove inapplicable items for your PR.
- [ ] 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
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] mseth10 commented on a change in pull request
#17194: Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
mseth10 commented on a change in pull request #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#discussion_r362148435
##########
File path: src/c_api/c_api.cc
##########
@@ -646,10 +654,28 @@ int MXLoadLib(const char *path) {
// TODO(samskalicky): enable constant overwriting of registertion multiple times
plevel++;
}
- regOp.set_attr<nnvm::FInferType>("FInferType", infer_type, plevel);
- regOp.set_attr<mxnet::FInferShape>("FInferShape", infer_shape, plevel);
- regOp.set_attr<FInferStorageType>("FInferStorageType", infer_storage_type, plevel);
- regOp.set_attr<FResourceRequest>("FResourceRequest", resc_req, plevel);
+ if (!isSubgraphOp) {
+ regOp.set_attr<nnvm::FInferType>("FInferType", infer_type, plevel);
+ regOp.set_attr<mxnet::FInferShape>("FInferShape", infer_shape, plevel);
+ regOp.set_attr<FInferStorageType>("FInferStorageType", infer_storage_type, plevel);
+ regOp.set_attr<FResourceRequest>("FResourceRequest", resc_req, plevel);
+ // optionally add fmutate inputs if user specified a function
+ if (mutate_fp != nullptr)
+ regOp.set_attr<nnvm::FMutateInputs>("FMutateInputs", mutate_inputs, plevel);
+ } else {
+ using namespace mxnet::op;
+ regOp.set_attr<nnvm::FInferType>("FInferType",
+ DefaultSubgraphOpType, plevel);
+ regOp.set_attr<mxnet::FInferShape>("FInferShape",
+ DefaultSubgraphOpShape, plevel);
+ regOp.set_attr<FInferStorageType>("FInferStorageType",
+ DefaultSubgraphOpStorageType, plevel);
+ regOp.set_attr<FResourceRequest>("FResourceRequest",
+ DefaultSubgraphOpResourceRequest, plevel);
+ regOp.set_attr<nnvm::FMutateInputs>("FMutateInputs",
Review comment:
should we check `if (mutate_fp != nullptr)` 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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] zachgk merged pull request #17194: Enhancements
for custom subgraph op
Posted by GitBox <gi...@apache.org>.
zachgk merged pull request #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky commented on issue #17194:
Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-569983733
Hey @wkcn this PR is finished and ready for review, would you please take a look into this enhancement for custom subgraph ops? Happy New Year!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky edited a comment on issue #17194:
Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-570113206
@wkcn
> Is it available to copy a custom subgraph op test into `tests/python/unitest/test_extensions.py`?
The current subgraph op has an example tests here, but is SUPER hacky:
https://github.com/apache/incubator-mxnet/blob/e65fc4bee5b8602b38ba2e419a4ea2e5ce5b7e9a/example/extensions/lib_custom_op/test_subgraph.py#L47-L56
In this example we're literally doing a string replace to change the op in the symbol json string. This isnt something we want to test in the CI. Typically subgraph ops are inserted when the graph is partitioned.
It will be officially tested in test_extensions.py in the other PR that adds support for custom subgraph properties and actually uses custom subgraph ops in #17034 here are the code changes:
https://github.com/apache/incubator-mxnet/pull/17034/files#diff-460d06cb3af2af996c5ec259b086712fR87-R150
This PR (#17194) modifies the custom op that is used in that test with the changes in this PR:
https://github.com/apache/incubator-mxnet/pull/17194/files#diff-4347bc72a85f439003da183493ba2089R87
Would be great to get your feedback on #17034 too.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request
#17194: Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#discussion_r362144623
##########
File path: include/mxnet/lib_api.h
##########
@@ -571,6 +572,10 @@ class CustomOp {
create_opstate = func;
return *this;
}
+ CustomOp& isSubgraphOp(bool state) {
Review comment:
done! changed it to `setIsSubgraphOp()` and removed argument
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky commented on issue #17194:
Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-569844676
@TaoLv @ZhennanQin would you please review since you're familiar with the subgraph 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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky commented on a change in pull request
#17194: Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#discussion_r362259670
##########
File path: src/c_api/c_api.cc
##########
@@ -646,10 +654,28 @@ int MXLoadLib(const char *path) {
// TODO(samskalicky): enable constant overwriting of registertion multiple times
plevel++;
}
- regOp.set_attr<nnvm::FInferType>("FInferType", infer_type, plevel);
- regOp.set_attr<mxnet::FInferShape>("FInferShape", infer_shape, plevel);
- regOp.set_attr<FInferStorageType>("FInferStorageType", infer_storage_type, plevel);
- regOp.set_attr<FResourceRequest>("FResourceRequest", resc_req, plevel);
+ if (!isSubgraphOp) {
+ regOp.set_attr<nnvm::FInferType>("FInferType", infer_type, plevel);
+ regOp.set_attr<mxnet::FInferShape>("FInferShape", infer_shape, plevel);
+ regOp.set_attr<FInferStorageType>("FInferStorageType", infer_storage_type, plevel);
+ regOp.set_attr<FResourceRequest>("FResourceRequest", resc_req, plevel);
+ // optionally add fmutate inputs if user specified a function
+ if (mutate_fp != nullptr)
+ regOp.set_attr<nnvm::FMutateInputs>("FMutateInputs", mutate_inputs, plevel);
+ } else {
+ using namespace mxnet::op;
+ regOp.set_attr<nnvm::FInferType>("FInferType",
+ DefaultSubgraphOpType, plevel);
+ regOp.set_attr<mxnet::FInferShape>("FInferShape",
+ DefaultSubgraphOpShape, plevel);
+ regOp.set_attr<FInferStorageType>("FInferStorageType",
+ DefaultSubgraphOpStorageType, plevel);
+ regOp.set_attr<FResourceRequest>("FResourceRequest",
+ DefaultSubgraphOpResourceRequest, plevel);
+ regOp.set_attr<nnvm::FMutateInputs>("FMutateInputs",
Review comment:
as discussed offline, we'll only use default functions when the user sets the setIsSubgraphOp flag. Later we can revisit if the user wants to use custom functions mixed with default 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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky edited a comment on issue #17194:
Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-570113206
@wkcn
> Is it available to copy a custom subgraph op test into `tests/python/unitest/test_extensions.py`?
The current subgraph op has an example tests here, but is SUPER hacky:
https://github.com/apache/incubator-mxnet/blob/e65fc4bee5b8602b38ba2e419a4ea2e5ce5b7e9a/example/extensions/lib_custom_op/test_subgraph.py#L47-L56
In this example we're literally doing a string replace to change the op in the symbol json string. This isnt something we want to test in the CI. Typically subgraph ops are inserted when the graph is partitioned using subgraph properties.
It will be officially tested in test_extensions.py in the other PR that adds support for custom subgraph properties and actually uses custom subgraph ops in #17034 here are the code changes:
https://github.com/apache/incubator-mxnet/pull/17034/files#diff-460d06cb3af2af996c5ec259b086712fR87-R150
This PR (#17194) modifies the custom op that is used in that test with the changes in this PR:
https://github.com/apache/incubator-mxnet/pull/17194/files#diff-4347bc72a85f439003da183493ba2089R87
Would be great to get your feedback on #17034 too.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn commented on issue #17194: Enhancements for
custom subgraph op
Posted by GitBox <gi...@apache.org>.
wkcn commented on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-570101629
@samskalicky Happy New Year!
This PR looks good to me : )
Is it available to copy a custom subgraph op test into `python/unitest/test_extensions.py`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] TaoLv commented on a change in pull request
#17194: Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
TaoLv commented on a change in pull request #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#discussion_r362141031
##########
File path: include/mxnet/lib_api.h
##########
@@ -571,6 +572,10 @@ class CustomOp {
create_opstate = func;
return *this;
}
+ CustomOp& isSubgraphOp(bool state) {
Review comment:
Sounds this function should return true or false. How about using `setXXX`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky commented on issue #17194:
Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-570113206
@wkcn
> Is it available to copy a custom subgraph op test into `tests/python/unitest/test_extensions.py`?
The current subgraph op has an example tests here, but is SUPER hacky:
https://github.com/apache/incubator-mxnet/blob/e65fc4bee5b8602b38ba2e419a4ea2e5ce5b7e9a/example/extensions/lib_custom_op/test_subgraph.py#L55-L56
In this example we're literally doing a string replace to change the op in the symbol json string. This isnt something we want to test in the CI. Typically subgraph ops are inserted when the graph is partitioned.
It will be officially tested in test_extensions.py in the other PR that adds support for custom subgraph properties and actually uses custom subgraph ops in #17034 here are the code changes:
https://github.com/apache/incubator-mxnet/pull/17034/files#diff-460d06cb3af2af996c5ec259b086712fR87-R150
This PR (#17194) modifies the custom op that is used in that test with the changes in this PR:
https://github.com/apache/incubator-mxnet/pull/17194/files#diff-4347bc72a85f439003da183493ba2089R87
Would be great to get your feedback on #17034 too.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] wkcn edited a comment on issue #17194:
Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
wkcn edited a comment on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-570101629
@samskalicky Happy New Year!
This PR looks good to me : )
Is it available to copy a custom subgraph op test into `tests/python/unitest/test_extensions.py`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky edited a comment on issue #17194:
Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-570113206
@wkcn
> Is it available to copy a custom subgraph op test into `tests/python/unitest/test_extensions.py`?
The current subgraph op has an example tests here, but is SUPER hacky:
https://github.com/apache/incubator-mxnet/blob/e65fc4bee5b8602b38ba2e419a4ea2e5ce5b7e9a/example/extensions/lib_custom_op/test_subgraph.py#L47-L56
In this example we're literally doing a string replace to change the op in the symbol json string. This isnt something we want to test in the CI. Typically subgraph ops are inserted when the graph is partitioned using subgraph properties.
It will be officially tested in test_extensions.py in the other PR that adds support for custom subgraph properties and actually uses custom subgraph ops in #17034 here are the code changes:
https://github.com/apache/incubator-mxnet/pull/17034/files#diff-460d06cb3af2af996c5ec259b086712fR87-R150
This PR (#17194) modifies the custom op that is used in that test with the changes in this PR:
https://github.com/apache/incubator-mxnet/pull/17194/files#diff-4347bc72a85f439003da183493ba2089R87
Would be great to get your feedback on #17034 too.
If this is acceptable would you please approve/merge 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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] samskalicky commented on issue #17194:
Enhancements for custom subgraph op
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17194: Enhancements for custom subgraph op
URL: https://github.com/apache/incubator-mxnet/pull/17194#issuecomment-569843920
@mxnet-label-bot add [pr-awaiting-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
With regards,
Apache Git Services