[MXNET-33] SSD example not working with mkl-dnn #10021

ashokei · 2018-03-07T05:45:39Z

Description

SSD VGG16-reduced example is not working if pooling convention is "full" with Max pool. This PR resolves the this issue by defaulting to CPU implementation when pooling convention is "full".

Checklist

Essentials

[ ✅ ] Passed code style checking (make lint)
[ ✅ ] Changes are complete (i.e. I finished coding on this PR)

Changes

disable mkl-dnn if pooling convention is "full"

Comments

This PR is to resolve the crash seen. A new PR may be needed to implement "full" pooling convention with mkl-dnn.

ashokei · 2018-03-07T05:54:59Z

@zheng-da @TaoLv please review. I notice we do not have proper "full" pooling convention support, this PR disables mkl-dnn pooling for "full" convention;

We will probably need a new PR to implement the "full" convention feature with mkl-dnn for both Max and Avg Pooling cases.

TaoLv · 2018-03-07T06:13:57Z

There is a test case for max pooling with full convention: https://github.com/apache/incubator-mxnet/blob/master/tests/python/gpu/test_operator_gpu.py#L841
But it isn't observed that this case failed before. If the test case doesn't cover the corner case you mentioned, maybe we need add new test case for it.

piiswrong · 2018-03-07T06:24:46Z

src/operator/nn/mkldnn/mkldnn_pooling-inl.h

@@ -92,6 +92,8 @@ inline bool SupportMKLDNNPooling(const PoolingParam &param,

  if (param.pooling_convention == pool_enum::kValid)
    return true;
+  else
+    return false;


this makes the later code unreachable. Are you sure this is what you want?

@piiswrong the later code is being disabled for now until we support all "full" pooling convention cases. As @TaoLv mentioned, i will add a unit-test for this failure.

maybe use "#if 0" to comment the checks below?

marcoabreu

Please add a test that produces the crash

ashokei · 2018-03-08T21:57:56Z

@marcoabreu @TaoLv @zheng-da Unit-tests currently do not seem to test backward/training mode for pooling very well. Below is the test for reproducing this crash.

ln -s $(pwd)/example/ssd/symbol/vgg16_reduced.py $(pwd)/example/image-classification/symbols/vgg16_reduced.py
python example/image-classification/train_cifar10.py --network vgg16_reduced

This PR will fix the above crash.

marcoabreu · 2018-03-08T23:50:07Z

Feel free to develop and add an appropriate unit test yourself. Please don't hesitate to reach out to us in case you need assistance.

ashokei · 2018-03-14T00:11:21Z

@marcoabreu this issue only happens inside a particular model (SSD VGG16-reduced), i clarified in title/description. I'm not sure a special unittest just for this specific model is appropriate. I am not able to reproduce this issue on separately. please let me know if any other suggestions/feedback, thanks!.

marcoabreu · 2018-03-14T00:25:48Z

That's not an issue, we're already testing a variety of models and the bigger diversity, the better. We can re-visit this decision later on and reduce the test size if required. For now, it would be good to get a test since we would like to re-enable pooling for MKL-DNN at a later point in time - this test would give us a tool to verify that it's actually working again.

marcoabreu · 2018-03-16T13:01:25Z

tests/python/cpu/test_mkldnn.py

+    """
+
+    import mxnet as mx
+    model = os.path.join(os.path.dirname(os.path.realpath(__file__)), "test_model.json")


could you move this file to a subdirectory like tests/python/cpu/data/test_mkldnn_ test_mkldnn_model_model1.json? (format is %FILENAME%_%TESTNAME%_model%NUMBER%.json)?

marcoabreu · 2018-03-16T13:02:13Z

tests/python/cpu/test_mkldnn.py

+            "variable settings"
+
+
+def test_mkldnn_model():


Please explain that this test has no asserts because it is only supposed to run successfully without failure.

@marcoabreu i added assert for sanity check, and renamed file as you mentioned. any other suggestions , please let me know.

marcoabreu · 2018-03-16T17:40:32Z

src/operator/nn/mkldnn/mkldnn_pooling-inl.h


+// need to support pooling convention full 


Please add a link to the issue as part of the comment

you mean , link to this PR ? or JIRA ?

marcoabreu · 2018-03-16T17:43:09Z

tests/python/cpu/test_mkldnn.py

+            "variable settings"
+
+
+def test_mkldnn_model():


pengzhao-intel · 2018-03-23T14:02:41Z

#10092
@marcoabreu could you help review again and merge?

zheng-da · 2018-03-24T04:43:57Z

@ashokei @pengzhao-intel do you know when the Intel MKLDNN team will fix the bug in MKLDNN?

marcoabreu · 2018-03-26T10:57:59Z

tests/python/cpu/test_mkldnn.py

@@ -0,0 +1,94 @@
+# Licensed to the Apache Software Foundation (ASF) under one


tests in tests/python/cpu are currently not run in CI. Please add it to the MKLDNN jobs.

@marcoabreu This PR only adds to existing test, no new test file is created. Can you be specific which CI jobs you are referring to ? If there is a gap in CI, a new JIRA item can be created to address it.

According to https://github.com/apache/incubator-mxnet/blob/master/Jenkinsfile#L439, which in the end points to https://github.com/apache/incubator-mxnet/blob/master/ci/docker/runtime_functions.sh#L387, we're not running the tests located in tests/python/cpu. Please rename tests/python/cpu to tests/python/mkl and add another function in the same style as https://github.com/apache/incubator-mxnet/blob/master/Jenkinsfile#L458.

ashokei · 2018-03-26T16:14:28Z

@zheng-da This is not a bug in MKL-DNN, it is an issue with current MKL-DNN mxnet integration. This PR will use CPU fallback.

zheng-da · 2018-03-27T02:45:00Z

what i mean is that mkldnn doesn't support "full" pooling convention. will mkldnn eventually support it? if it does, when will it support it?

ashokei · 2018-03-27T03:21:56Z

@zheng-da sorry for the confusion, mkl-dnn has more general pooling support, it allows us to implement any arbitrary convention we like. So it is not bug in mkl-dnn, it is just that we havent integrated this specific use-case in our mxnet mkldnn pooling implementation yet.

zheng-da · 2018-03-27T06:09:07Z

oh, i see. do you mind implementing it? fallback is fine, but it'll be better if it can invoke the one in MKLDNN.

ashokei · 2018-03-27T06:20:20Z

yes, we plan to add new features for pooling implementation with mkl-dnn in future PR.
this PR just fixes the crash in SSD, and this specific case (maxpool with fullconvention) is only used in 1 layer, and not common.

ashokei · 2018-03-27T17:09:43Z

@marcoabreu does this look ok, thanks.

ashokei · 2018-03-27T23:33:22Z

@marcoabreu fixed now

marcoabreu

Good job, thanks for fixing the example :)

anirudh2290 · 2018-04-09T22:12:03Z

@ashokei can you please check if the CI failure is related to your change and retrigger it if not ?

ashokei · 2018-04-10T03:48:39Z

@anirudh2290 done...thanks.

anirudh2290 · 2018-04-10T05:20:01Z

@piiswrong is this good to merge ?

zheng-da · 2018-04-18T00:15:52Z

@piiswrong this PR is to fix a bug in the MKLDNN integration. MKLDNN pooling can't handle full pooling convention correctly.
do you have any comments for this PR? if not, can you merge it?

ashokei · 2018-04-23T20:38:30Z

@piiswrong can you please merge this, this PR also has mkldnn jenkins/unittest as requested by @marcoabreu

* use mkl-dnn for 'valid' pooling_convention only * pooling convention full not supported by current mkl-dnn impl * disable unreachable code * add sample model test for mkldnn * fix review feedback * add jira link to comment * fix lint issue * rename python test for mkl * enable python tests for mkldnn in CI * use vgg16 with convention full * fix unittest

ashokei requested a review from cjolivier01 as a code owner March 7, 2018 05:45

piiswrong reviewed Mar 7, 2018

View reviewed changes

ashokei changed the title ~~[MXNET-33] Mkldnn pooling convention crash~~ [WIP] [MXNET-33] Mkldnn pooling convention crash Mar 7, 2018

marcoabreu suggested changes Mar 7, 2018

View reviewed changes

ashokei changed the title ~~[WIP] [MXNET-33] Mkldnn pooling convention crash~~ [MXNET-33] Mkldnn pooling convention crash Mar 8, 2018

ashokei changed the title ~~[MXNET-33] Mkldnn pooling convention crash~~ [MXNET-33] SSD example not working with mkl-dnn Mar 14, 2018

marcoabreu mentioned this pull request Mar 14, 2018

No "full" pooling convention support with MKL-DNN #10092

Closed

marcoabreu suggested changes Mar 16, 2018

View reviewed changes

marcoabreu approved these changes Mar 16, 2018

View reviewed changes

ashokei force-pushed the mkldnn_pooling_convention_valid branch from 773a5af to 6ca171a Compare March 16, 2018 22:52

zheng-da mentioned this pull request Mar 19, 2018

Fix mem alignment. #10161

Merged

5 tasks

marcoabreu suggested changes Mar 26, 2018

View reviewed changes

ashokei force-pushed the mkldnn_pooling_convention_valid branch from 904b3fa to 8dd8e35 Compare March 27, 2018 19:57

ashokei requested review from sergeykolychev and szha as code owners March 27, 2018 19:57

ashokei force-pushed the mkldnn_pooling_convention_valid branch from 7e63aa1 to ed0f909 Compare March 27, 2018 23:30

marcoabreu approved these changes Mar 28, 2018

View reviewed changes

ashokei force-pushed the mkldnn_pooling_convention_valid branch from ed0f909 to 8d4171d Compare April 2, 2018 23:36

ashokei force-pushed the mkldnn_pooling_convention_valid branch from 8d4171d to f6cbd76 Compare April 10, 2018 00:18

ashokei force-pushed the mkldnn_pooling_convention_valid branch from f6cbd76 to 079fba2 Compare April 18, 2018 00:19

ashokei added 11 commits April 23, 2018 11:47

use mkl-dnn for 'valid' pooling_convention only

9ffdbc7

pooling convention full not supported by current mkl-dnn impl

82e4c45

disable unreachable code

6e9a9fa

add sample model test for mkldnn

7f690dc

fix review feedback

715fbb7

add jira link to comment

c4973ba

fix lint issue

069c260

rename python test for mkl

83a47f5

enable python tests for mkldnn in CI

0caa2e5

use vgg16 with convention full

413b747

fix unittest

4fbee8d

ashokei force-pushed the mkldnn_pooling_convention_valid branch from 079fba2 to 4fbee8d Compare April 23, 2018 18:48

ashokei mentioned this pull request Apr 24, 2018

enable jenkins for mkldnn unittest #10670

Closed

7 tasks

piiswrong merged commit fd55a2e into apache:master Apr 24, 2018

TaoLv mentioned this pull request May 24, 2018

[MXNET-33] Enhance mkldnn pooling to support full convention #11047

Merged

7 tasks

		@@ -0,0 +1,94 @@
		# Licensed to the Apache Software Foundation (ASF) under one

[MXNET-33] SSD example not working with mkl-dnn #10021

[MXNET-33] SSD example not working with mkl-dnn #10021

Conversation

ashokei commented Mar 7, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

ashokei commented Mar 7, 2018

TaoLv commented Mar 7, 2018

Choose a reason for hiding this comment

ashokei Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoabreu left a comment

Choose a reason for hiding this comment

ashokei commented Mar 8, 2018 • edited Loading

marcoabreu commented Mar 8, 2018

ashokei commented Mar 14, 2018

marcoabreu commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pengzhao-intel commented Mar 23, 2018

zheng-da commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokei commented Mar 26, 2018

zheng-da commented Mar 27, 2018 • edited Loading

ashokei commented Mar 27, 2018

zheng-da commented Mar 27, 2018

ashokei commented Mar 27, 2018

ashokei commented Mar 27, 2018

ashokei commented Mar 27, 2018

marcoabreu left a comment

Choose a reason for hiding this comment

anirudh2290 commented Apr 9, 2018

ashokei commented Apr 10, 2018

anirudh2290 commented Apr 10, 2018

zheng-da commented Apr 18, 2018

ashokei commented Apr 23, 2018

ashokei commented Mar 7, 2018 •

edited

Loading

ashokei Mar 7, 2018 •

edited

Loading

ashokei commented Mar 8, 2018 •

edited

Loading

zheng-da commented Mar 27, 2018 •

edited

Loading