Register fake grad to subgraph and quantized operators by xinyu-intel · Pull Request #14275 · apache/mxnet

xinyu-intel · 2019-02-28T02:24:45Z

Description

Motivation:
Register fake grad to subgraph and quantized operators to support loading back JSON files which contain inference_only operators as symbolblock to run gluon inference.

@pengzhao-intel @TaoLv @ZhennanQin @reminisce

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

pengzhao-intel · 2019-02-28T04:08:43Z

MI, this is a temp solution to enable GluonCV INT8 flow and we will revert it after the improvement of CachedOP is done.

reminisce

LGTM. One suggestion, please add TODO(owner's name): some statement in the code as a reminder for future maintenance.

TaoLv · 2019-02-28T06:47:40Z

Is it possible to merge this PR into #14276 ? @xinyu-intel @ZhennanQin @pengzhao-intel

ZhennanQin · 2019-02-28T06:59:10Z

@TaoLv I suggest not merge them together because this PR is a workaround while #14276 isn't. Then we can simply revert this PR when cached_op refactoring is done.

TaoLv · 2019-02-28T07:09:22Z

@ZhennanQin I'm afraid this PR has side effect if it's merged before #14276 . Reverting should not be a big deal as it only changes 9 lines. We always need a PR to revert changes.

ZhennanQin · 2019-02-28T07:16:54Z

@TaoLv Yes, there's side-effect if this is merged before #14276. So the correct merge order is to merge #14276 first. If you think it's not a big deal on reverting, I'm fine to make them into same PR, although I don't see any benefit from it.

TaoLv · 2019-02-28T07:34:56Z

Avoiding side effect and keeping master branch healthy is the benefit.

ZhennanQin · 2019-02-28T07:37:58Z

Merge with correct order can have same benefit:)

ZhennanQin · 2019-02-28T07:39:19Z

@xinyu-intel Please collect #14276 into this PR as @TaoLv suggests.

anirudhacharya · 2019-03-01T02:35:36Z

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

pengzhao-intel · 2019-03-03T02:54:15Z

@TaoLv please help review again :)

TaoLv

LGTM. Just minor comment.

TaoLv · 2019-03-03T03:37:51Z

src/operator/subgraph/mkldnn/mkldnn_conv_post_quantize_property.cc

  static SubgraphPropertyPtr Create() {
-    return std::make_shared<SgMKLDNNConvPostQuantizeProperty>();
+    auto property = std::make_shared<SgMKLDNNConvPostQuantizeProperty>();
+    property->SetAttr<std::string>("prop_name",


Nit: can we use "property_name" or just "name" here? Because "prop" also stands for "propagation" somewhere.

TaoLv · 2019-03-03T03:39:43Z

@reminisce Can you take a look at the changes for subgraph?

pengzhao-intel

LGTM

@reminisce please help confirm the subgraph related changes :)

anirudh2290 · 2019-03-04T23:55:37Z

src/operator/quantization/quantize.cc

 .set_attr<nnvm::FInferType>("FInferType", QuantizeType)
 .set_attr<FInferStorageType>("FInferStorageType", QuantizeStorageType)
+// TODO(Xinyu): a temp solution to enable GluonCV INT8 flow,
+// will be reverted after the improvement of CachedOP is done.


is this currently WIP ? can you open an issue for this

#14331 added:)

anirudh2290 · 2019-03-05T00:29:17Z

src/executor/graph_executor.cc

+      LOG(INFO) << "Skip subgraph " << full_name << " as it requires `grad_req=null`.";
+      return src;
+    }
+  }


can we add a test for this case: module bound with for_training=True, and the symbol reused to invoke graph partitioning with MKLDNN. The returned symbol shouldn't contain mkldnn subgraph ops.

The problem is, we can't get the symbol after module bound. So we can't check if MKLDNN graph partitioning happens or not.

Why can't we use mod._sym and call get_backend_symbol("MKLDNN") on it?

Because there's no python API can return the partitioned symbol after bind, even mod._sym holds the original symbol. And get_backend_symbol("MKLDNN") will apply partition and won't skip inference_only pass. inference_only only works for bind.

makes sense.

* add fake grad * Skip inference only subgraph pass when gradient is needed. * add fake grad to quantizev2 * add TODO * modify prop_name to property_name * add test case

xinyu-intel added 2 commits February 25, 2019 16:29

add fake grad

8a3c106

Merge remote-tracking branch 'upstream/master' into fake_grad

3da11fb

Merge remote-tracking branch 'upstream/master' into fake_grad

c9e7116

reminisce reviewed Feb 28, 2019

View reviewed changes

ZhennanQin mentioned this pull request Feb 28, 2019

Skip inference only subgraph pass when gradient is needed. #14276

Closed

7 tasks

TaoLv approved these changes Feb 28, 2019

View reviewed changes

xinyu-intel added 3 commits February 28, 2019 16:04

Skip inference only subgraph pass when gradient is needed.

237376d

add fake grad to quantizev2

4d16e29

add TODO

6f06baa

marcoabreu added the pr-awaiting-review PR is waiting for code review label Mar 1, 2019

TaoLv approved these changes Mar 3, 2019

View reviewed changes

pengzhao-intel approved these changes Mar 4, 2019

View reviewed changes

xinyu-intel added 3 commits March 4, 2019 10:37

modify prop_name to property_name

a1a31c0

Merge remote-tracking branch 'upstream/master' into fake_grad

869b601

add test case

61d4d16

anirudh2290 reviewed Mar 5, 2019

View reviewed changes

xinyu-intel mentioned this pull request Mar 5, 2019

A temp solution to enable GluonCV INT8 flow #14331

Open

reminisce approved these changes Mar 6, 2019

View reviewed changes

reminisce merged commit b486594 into apache:master Mar 6, 2019

Conversation

xinyu-intel commented Feb 28, 2019

Description

Checklist

Essentials

Changes

Comments

Uh oh!

pengzhao-intel commented Feb 28, 2019

Uh oh!

reminisce left a comment

Choose a reason for hiding this comment

Uh oh!

TaoLv commented Feb 28, 2019

Uh oh!

ZhennanQin commented Feb 28, 2019

Uh oh!

TaoLv commented Feb 28, 2019

Uh oh!

ZhennanQin commented Feb 28, 2019

Uh oh!

TaoLv commented Feb 28, 2019

Uh oh!

ZhennanQin commented Feb 28, 2019

Uh oh!

ZhennanQin commented Feb 28, 2019

Uh oh!

anirudhacharya commented Mar 1, 2019

Uh oh!

pengzhao-intel commented Mar 3, 2019

Uh oh!

TaoLv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TaoLv commented Mar 3, 2019

Uh oh!

pengzhao-intel left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants