[DO NOT REVIEW] Subgraph API by reminisce · Pull Request #12104 · apache/mxnet

reminisce · 2018-08-09T06:12:45Z

Description

Incremental change to the dev branch. Not ready for review.

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 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 http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best 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

zheng-da · 2018-08-09T20:53:54Z

src/executor/graph_executor.cc

+                                   const std::string& prop_name,
+                                   const nnvm::ShapeVector& arg_shapes,
+                                   const nnvm::DTypeVector& arg_dtypes,
+                                   const StorageTypeVector arg_stypes,


use reference for arg_stypes.

zheng-da · 2018-08-09T20:58:27Z

src/operator/subgraph/partition_graph.cc

    return indexed_graph.node_id(node1) < indexed_graph.node_id(node2);
  };
  size_t subgraph_id = 0;
+  auto subgraph_selector = subg_prop.CreateSubgraphSelector();


a selector should be created for each node. It's stateful.

zheng-da · 2018-08-09T21:04:20Z

src/executor/graph_executor.cc

+  g.outputs = ret.outputs;
+  g = InferForwardAttrs(g, arg_shapes, arg_dtypes, arg_stypes, default_ctx,
+                        ctx_map, in_arg_ctxes, aux_state_ctxes);
+  subgraph_prop->SetAttr("graph", g);


This interface doesn't allow customizing g. I think it's better to provide an interface such as set_graph and allow the child class to override this virtual method.

I think this can be done in CreateSubgraphSelector()?

Subgraph selector is created by each node. I feel it's more intuitive to customize g somewhere subgraph property is created.

ZhennanQin · 2018-08-10T02:46:52Z

Can this change cover hybridize gluon model? I don't see the change in cached_op.

ZhennanQin · 2018-08-10T02:43:41Z

src/executor/graph_executor.cc

                               Executor* shared_exec) {
  auto exec = new exec::GraphExecutor();
+  if (!exec->subgraph_property().empty()) {
+    symbol = exec::PartitionGraph(symbol, exec->subgraph_property(), arg_shape_map, arg_dtype_map,


Many subgraph pass can only be used on inference, eg. default_subgraph_op doesn't have FGradient attribute and can't support backward computation. Maybe default_subgraph_op will support backward in future, but some mkldnn fusion pass is designed to be used in inference only. Shall we allow subgraph_property to describe if it's inference only and apply it as it requires?

Is it necessary to indicate whether a subgraph property is for inference only or not?

Yes, that's also my question. We indeed have some fusion passes only for inference, we need a way to disable it on training. If we can't describe this in subgraph property, then we need to find another way to indicate that.

ZhennanQin · 2018-08-10T03:06:34Z

Another question: what's the sequence should we execute below 2 kinds of subgraph pass:

the pass to cover all operators that backends support.(just like default_subgraph_op)
Fusion pass to combine several ops into a new ops.

I'm asking this is because, if we want to apply 2 after 1, then we need to have the ability to traverse into subgraph node to do fusion. Do we support this?

reminisce · 2018-08-10T05:45:43Z

@ZhennanQin CachedOp will be submitted through a another PR. We want to make this PR as small as possible.

I think what you described about fusion can be done in graph partitioning as well. In CreateSubgraphNode(), you can replace the subgraph symbol with a new subgraph symbol consisting of fused operators. Does this make sense?

ZhennanQin · 2018-08-10T05:54:58Z

@reminisce Then we need to call PartitionGraph() inside CreateSubgraphNode() to traverse nodes, otherwise we have to implement a separate traverse method. If that meet your expectation, then I'm OK with this solution.

reminisce · 2018-08-10T22:00:45Z

@ZhennanQin Yes, I fine with that.

zheng-da · 2018-08-13T23:19:51Z

src/operator/subgraph/subgraph_property.h

+
+#define MXNET_REGISTER_SUBGRAPH_PROPERTY(Name, SubgraphPropertyType) \
+  static DMLC_ATTRIBUTE_UNUSED auto __make_ ## SubgraphPropertyType ## _ ## Name ## __ = \
+    SubgraphPropertyRegistry::Get()->__REGISTER__(#Name, &SubgraphPropertyType::Create);


remove ";" here.

zheng-da · 2018-08-13T23:20:10Z

src/operator/subgraph/default_subgraph_property.h

+  }
+};
+
+MXNET_REGISTER_SUBGRAPH_PROPERTY(default, DefaultSubgraphProperty)


add a ";" at the end.

Graph partitioner and subgraph op Fix duplicate entry bugs (apache#11767) Make subgraph var node name unique (apache#11876) [DO NOT REVIEW] Fix bug of eliminating cycles (apache#11907) * Fix cycle bug * Fix decycle bug * Fix comment [DO NOT REVIEW] Subgraph API (apache#12104) * Initial commit * Add unit tests * Fix lint * Fix lint * Clean up * Add graph partitiong to Bind * Add property name to graph partitioning c api * Fix unit test gpu context * Address cr * Move subgraph to attrs.subgraphs and fix the example * Fix lint * Add var version unit test * Address cr * Enable unit test that was flaky

* Graph partitioner and subgraph op (#11251) Graph partitioner and subgraph op Fix duplicate entry bugs (#11767) Make subgraph var node name unique (#11876) [DO NOT REVIEW] Fix bug of eliminating cycles (#11907) * Fix cycle bug * Fix decycle bug * Fix comment [DO NOT REVIEW] Subgraph API (#12104) * Initial commit * Add unit tests * Fix lint * Fix lint * Clean up * Add graph partitiong to Bind * Add property name to graph partitioning c api * Fix unit test gpu context * Address cr * Move subgraph to attrs.subgraphs and fix the example * Fix lint * Add var version unit test * Address cr * Enable unit test that was flaky * Clean up * Clean up * Clean up * Change version return type in NDArray * Clean up * Add register or get for subgraph prop registry * Address cr * Remove unnecessary code * Handle var version issue in naive engine * Delete example * Remove registration of resource request for default subgraph op * Add doc string * Improve doc string

* Graph partitioner and subgraph op (apache#11251) Graph partitioner and subgraph op Fix duplicate entry bugs (apache#11767) Make subgraph var node name unique (apache#11876) [DO NOT REVIEW] Fix bug of eliminating cycles (apache#11907) * Fix cycle bug * Fix decycle bug * Fix comment [DO NOT REVIEW] Subgraph API (apache#12104) * Initial commit * Add unit tests * Fix lint * Fix lint * Clean up * Add graph partitiong to Bind * Add property name to graph partitioning c api * Fix unit test gpu context * Address cr * Move subgraph to attrs.subgraphs and fix the example * Fix lint * Add var version unit test * Address cr * Enable unit test that was flaky * Clean up * Clean up * Clean up * Change version return type in NDArray * Clean up * Add register or get for subgraph prop registry * Address cr * Remove unnecessary code * Handle var version issue in naive engine * Delete example * Remove registration of resource request for default subgraph op * Add doc string * Improve doc string

reminisce added 4 commits August 8, 2018 15:54

Initial commit

acc47ac

Add unit tests

cf56959

Fix lint

478e328

Fix lint

96cc7d2

reminisce requested a review from anirudh2290 as a code owner August 9, 2018 06:12

reminisce added 3 commits August 8, 2018 23:14

Clean up

62d7e61

Add graph partitiong to Bind

ed229db

Add property name to graph partitioning c api

554e93c

zheng-da reviewed Aug 9, 2018

View reviewed changes

Fix unit test gpu context

08d9d7c

ZhennanQin reviewed Aug 10, 2018

View reviewed changes

Address cr

98e951e

Move subgraph to attrs.subgraphs and fix the example

08e7838

reminisce requested a review from szha as a code owner August 10, 2018 22:48

reminisce added 2 commits August 10, 2018 15:51

Fix lint

b5a1207

Add var version unit test

6352cab

szha removed their request for review August 11, 2018 18:46

zheng-da reviewed Aug 13, 2018

View reviewed changes

reminisce added 2 commits August 13, 2018 17:27

Address cr

153e987

Enable unit test that was flaky

998c341

reminisce merged commit 4c1933e into apache:subgraph Aug 14, 2018

Conversation

reminisce commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhennanQin commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhennanQin commented Aug 10, 2018

Uh oh!

reminisce commented Aug 10, 2018

Uh oh!

ZhennanQin commented Aug 10, 2018

Uh oh!

reminisce commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

reminisce commented Aug 9, 2018 •

edited

Loading