-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@mxnet-label-bot add [pr-awaiting-review] |
@TaoLv @ZhennanQin would you please review since you're familiar with the subgraph API? |
include/mxnet/lib_api.h
Outdated
@@ -571,6 +572,10 @@ class CustomOp { | |||
create_opstate = func; | |||
return *this; | |||
} | |||
CustomOp& isSubgraphOp(bool state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds this function should return true or false. How about using setXXX
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! changed it to setIsSubgraphOp()
and removed argument
DefaultSubgraphOpStorageType, plevel); | ||
regOp.set_attr<FResourceRequest>("FResourceRequest", | ||
DefaultSubgraphOpResourceRequest, plevel); | ||
regOp.set_attr<nnvm::FMutateInputs>("FMutateInputs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check if (mutate_fp != nullptr)
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
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! |
@samskalicky Happy New Year! This PR looks good to me : ) |
The current subgraph op has an example tests here, but is SUPER hacky: 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: This PR (#17194) modifies the custom op that is used in that test with the changes in this PR: Would be great to get your feedback on #17034 too. If this is acceptable would you please approve/merge this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
Description
Enhancements for custom subgraph operators in external libraries.
Design
Its difficult to implement the full shape/type propagation in an external library for a whole subgraph. Rather that re-implement these complex pieces, we'll use the default functions in the subgraph API.
Starting from the user in the custom library, the registration is now only:
Here, setting
isSubgraphOp
means we dont have to register the infer shape/type functions. When re-registering the operator in MXNet, ifisSubgraphOp
is true, then we re-use the functions from the subgraph API:Checklist
Essentials
Please feel free to remove inapplicable items for your PR.