[MXNET-1324] Add NaiveRunGraph to imperative utils #14192

junrushao · 2019-02-18T19:17:31Z

Description

This PR introduces another way of executing a graph, NaiveRunGraph, which runs the operators one by another. This is part of effort of enabling dynamic shapes inside a CahcedOp. Since the output shape of operators coundn't be determined ahead of time, we have to run these operators one at a time.

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

Add a NaiveForward pass.

Comments

Since it is only used in dynamic shape, this won't cause performance regression.

junrushao · 2019-02-18T21:59:50Z

@szha @zheng-da Could you help review? Thanks a lot!

roywei · 2019-02-19T00:17:21Z

@mxnet-label-bot add[Backend, pr-awaiting-review]

junrushao · 2019-02-28T21:59:25Z

@szha Hey I added a unit test for this PR :-)

src/imperative/cached_op.cc

vandanavk · 2019-03-04T23:05:13Z

@zheng-da is this PR good to go?

junrushao · 2019-03-04T23:22:05Z

Rebased to the master because of the dramatic change to TShape.

junrushao · 2019-03-05T05:31:31Z

@szha @zheng-da Hey could you do another round of review? It has passed CI.

src/imperative/cached_op.cc

junrushao · 2019-03-06T08:49:30Z

@szha @zheng-da hey could you guys do another round of review?

* Add NaiveRunGraph to imperative utils * update * update * Update * Update * Add unittest * Rebase to master * Add something to be consistent with master branch * Retrigger CI * Address comments from Da * Address comments from Sheng * Address * Refactor * Fix lint * Retrigger CI * Retrigger CI

larroy · 2019-06-13T17:24:24Z

src/imperative/imperative_utils.cc

+  return ndinputs;
+}
+
+inline std::vector<NDArray*> NodeOutputs(const nnvm::IndexedGraph& idx,


Why inline?

larroy · 2019-06-13T17:24:33Z

src/imperative/imperative_utils.cc

+  return req;
+}
+
+inline void InvokeOperator(const nnvm::IndexedGraph& idx,


Why inline?

Also this function should be static or in the anonymous namespace as it's not used outside this unit.

junrushao · 2019-06-13T18:13:00Z

@larroy thanks for asking! So could you elaborate what’s the problem of inline?

larroy · 2019-06-13T21:34:19Z

You are inlining a big function, this will cause code bloat at no benefit. Let the compiler do the inline for you in the majority of the cases. I removed the inline anyway so it's no longer in master.

See also: #15182

larroy · 2019-06-13T21:43:51Z

Since this commit might cause a regression, I would kindly request if you can elaborate a bit about the differences between running operators one at a time vs the previous behaviour.

larroy · 2019-06-13T21:45:28Z

src/imperative/cached_op.cc

+  }
+  // We leverage the shape inference pass to detect whether dynamic shape exists.
+  // If so, the pass will fail with `contain_dynamic_shape = true`,
+  // This method is only called once, so the overhead is negligible.


Were you able to verify this? I'm afraid this could cause slowdown.

If infer shape fails, it means there is probably an op of dynamic shape (np.unique, boolean mask).

MXNet didn't support this kind of op before, because of the limitation of our system. Because of these lines, we now can support it in Gluon blocks.

I would suggest to clearly mention the behavior in our docs that if infer shape fails, the code will go to the slow path (naive run graph, etc).

junrushao · 2019-06-14T01:44:13Z

Since this commit might cause a regression, I would kindly request if you can elaborate a bit about the differences between running operators one at a time vs the previous behaviour.

I agree. This is part of this proposal by @zheng-da, but I believe it will be super helpful if

the changes are reflected or elaborated in our docs so that the behavior is clearly understood.
suppress the newly added naive run graph pass if other flags (like static_alloc) are on.

* Add NaiveRunGraph to imperative utils * update * update * Update * Update * Add unittest * Rebase to master * Add something to be consistent with master branch * Retrigger CI * Address comments from Da * Address comments from Sheng * Address * Refactor * Fix lint * Retrigger CI * Retrigger CI

marcoabreu added Backend Issues related to the backend of MXNet pr-awaiting-review PR is waiting for code review labels Feb 19, 2019

junrushao force-pushed the dynamic-cached-op-pr-2 branch from edd748f to 697d772 Compare February 28, 2019 19:53

zheng-da reviewed Feb 28, 2019

View reviewed changes

src/imperative/cached_op.cc Show resolved Hide resolved

junrushao added 7 commits March 4, 2019 15:16

Add NaiveRunGraph to imperative utils

2784f3d

update

693eba7

update

ee26390

Update

bb85090

Update

a23d554

Add unittest

319ee39

Rebase to master

82e4c37

junrushao force-pushed the dynamic-cached-op-pr-2 branch from db50798 to 82e4c37 Compare March 4, 2019 23:17

Add something to be consistent with master branch

7f3090f

junrushao added 2 commits March 4, 2019 20:15

Retrigger CI

65f1741

Address comments from Da

efe7e61

szha reviewed Mar 5, 2019

View reviewed changes

src/imperative/cached_op.cc Outdated Show resolved Hide resolved

junrushao added 6 commits March 5, 2019 11:47

Address comments from Sheng

fc29cce

Address

062cdc6

Refactor

e6b5b00

Fix lint

b04a2be

Retrigger CI

94e15f2

Retrigger CI

ff5b6ad

szha merged commit 83d2c2d into apache:master Mar 6, 2019

This was referenced Mar 29, 2019

Masking Layer doesn't work after adding a NaiveRunGraph feature in MXNet awslabs/keras-apache-mxnet#228

Open

Ignoring masking layer test for RNN with MXNet backend awslabs/keras-apache-mxnet#229

Merged

larroy reviewed Jun 13, 2019

View reviewed changes

junrushao mentioned this pull request Jun 18, 2019

[MXNET-1417][Performance] Caching Dynamic Shape Checking Result #15262

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-1324] Add NaiveRunGraph to imperative utils #14192

[MXNET-1324] Add NaiveRunGraph to imperative utils #14192

junrushao commented Feb 18, 2019

junrushao commented Feb 18, 2019

roywei commented Feb 19, 2019

junrushao commented Feb 28, 2019

vandanavk commented Mar 4, 2019

junrushao commented Mar 4, 2019

junrushao commented Mar 5, 2019 •

edited

Loading

junrushao commented Mar 6, 2019

larroy Jun 13, 2019

larroy Jun 13, 2019

larroy Jun 13, 2019

junrushao commented Jun 13, 2019

larroy commented Jun 13, 2019

larroy commented Jun 13, 2019

larroy Jun 13, 2019

junrushao Jun 14, 2019

junrushao commented Jun 14, 2019 •

edited

Loading

[MXNET-1324] Add NaiveRunGraph to imperative utils #14192

[MXNET-1324] Add NaiveRunGraph to imperative utils #14192

Conversation

junrushao commented Feb 18, 2019

Description

Checklist

Essentials

Changes

Comments

junrushao commented Feb 18, 2019

roywei commented Feb 19, 2019

junrushao commented Feb 28, 2019

vandanavk commented Mar 4, 2019

junrushao commented Mar 4, 2019

junrushao commented Mar 5, 2019 • edited Loading

junrushao commented Mar 6, 2019

larroy Jun 13, 2019

Choose a reason for hiding this comment

larroy Jun 13, 2019

Choose a reason for hiding this comment

larroy Jun 13, 2019

Choose a reason for hiding this comment

junrushao commented Jun 13, 2019

larroy commented Jun 13, 2019

larroy commented Jun 13, 2019

larroy Jun 13, 2019

Choose a reason for hiding this comment

junrushao Jun 14, 2019

Choose a reason for hiding this comment

junrushao commented Jun 14, 2019 • edited Loading

junrushao commented Mar 5, 2019 •

edited

Loading

junrushao commented Jun 14, 2019 •

edited

Loading