-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
@ZhennanQin for review the compatible :) |
What's the difference between |
b6b5147
to
478c60a
Compare
11944af
to
3860dee
Compare
@ZhennanQin Here in this PR we're trying to do the following things:
In this PR we want to enable this functionality for users to more easily use partitioning. In a later PR we want to enable subgraph properties to be dynamically loaded from libraries. |
6fb556b
to
e1067da
Compare
@samskalicky Thanks for the explanation, it helps a lot to understand this PR. Basically, I think |
@ZhennanQin Thanks for your comments, we'll try and clarify our thought process. Apologies for the lack of clarity in this PR. You are correct in the current status of the code that @mseth10 has committed so far today. The PR is still WIP and more changes are coming, lets not make decisions on what is there today. Instead lets focus on the items i mentioned before (they will get into the PR description before we attempt to merge, I promise! :-D). We will add more arguments to the optimizeFor API in the coming commits. We need to add arguments to enable us to to do shape/type propagation prior to partitioning (so we can use shape/type info to select ops in subgraph properties), and we want to accept arbitrary options that we pass to the subgraph property for further configuration (ie. blacklisted ops). These new API changes will not be compatible with the current get_backend_symbol API. Since MXNet maintains semantic versioning for minor releases we cannot change the API yet. So instead we'll create another API (optimizeFor) along side get_backend_symbol for now. |
@mseth10 we also need to add the ability to reject creating a subgraph in the buildsubgraph.cc file: We want to enable the subgraph property to return a null node to say reject creating a subgraph. The partitioning pass has a decycle feature that may removed nodes that were selected when calling the select function on the subgraph property. So the subgraph may be smaller than anticipated, and we want the ability to not create subgraphs based on some criteria in the subgraph property (ie. subgraph size too small). |
dfeeacf
to
6082f41
Compare
@samskalicky @mseth10 As this PR is still WIP, please ping me when this PR is ready to review. I agree with you that we need a more powerful API to put dtype, shape and ctx into consideration to make partition more accurate. |
@ZhennanQin can you please approve? @mseth10 has added the "Next steps" in the PR description to save the outcome of our discussions from the PR comments. |
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. Thanks for contributing!
@mxnet-label-bot remove [pr-awaiting-review] |
@mxnet-label-bot add [pr-awaiting-merge] |
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.
Thanks for great works on this :)
Merging now.
Description
This PR introduces enhanced Python and C APIs to trigger graph partitioning for a given backend. These APIs provide an option to the user to infer shapes, dtypes and stypes before partitioning. They add the ability to pass optional arguments to subgraph property.
This PR makes the following modifications to subgraph property:
We reuse the existing graph partitioning algorithm and control using subgraph property classes. Since we’re modifying a symbol object, we add this API to the Symbol class. Users will call
optimize_for
on an existing symbol object to create a new partitioned symbol.Here is an example showing partitioning without args, which means infer shape/type will NOT be called before partitioning. It is similar to
GetBackendSymbol
.Here is an example showing partitioning with args, which means infer shape/type is called before partitioning.
Here is an example showing partitioning with kwargs to set a list of ops to be excluded from subgraphs.
Next steps:
excluded_ops
passed as arguments tooptimize_for
as part of the PR on support for custom subgraph propertiesoptimize_for
just like we have it insimple_bind
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.