Update MKL-DNN to v0.18 release (was: fix the Dense layer issue) #13668

TaoLv · 2018-12-18T04:42:08Z

Description

Issue: #13451

Since the case provided in the issue needs huge memory, I'm not sure if it's appropriate to put it into unit test.

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

Roshrini · 2018-12-19T00:27:15Z

@TaoLv Can you please check the failing test? @mseth10 Can you help review this PR?
@mxnet-label-bot Add [pr-awaiting-review]

…to fix-inner-product

TaoLv · 2019-01-04T14:52:20Z

@zheng-da Please try this update to see if it can fix your issue. Thank you.

stu1130 · 2019-01-16T21:22:30Z

@zheng-da ping for the update! Thanks

sandeep-krishnamurthy · 2019-01-28T17:48:52Z

@azai91 - Can you please help review this PR?

pengzhao-intel · 2019-02-03T02:16:04Z

We will update the latest MKLDNN CI before the merge.

pengzhao-intel · 2019-02-03T02:16:59Z

@zheng-da please help verify the fix.

ankkhedia · 2019-02-14T23:15:59Z

@zheng-da Ping for review !

TaoLv · 2019-02-17T03:25:35Z

MKL-DNN 0.18 is coming soon. We plan to upgrade MKL-DNN dependency to 0.18 and full validation is WIP. Please hold on to the code review and merge. Thank you.

pengzhao-intel · 2019-02-17T03:41:41Z

@TaoLv Is there the test case to catch this issue?

TaoLv · 2019-02-17T04:38:12Z

There is one in the original issue: #13451. But since it needs a big NDArray (~20GB memory) so I'm not sure if we can put it into CI unit test. I'm considering to add it into nightly: /~https://github.com/apache/incubator-mxnet/blob/master/tests/nightly/test_large_array.py

pengzhao-intel · 2019-02-17T04:46:00Z

yes, it's a big concern for CI. @zheng-da
@apeforest do you have any plan to test this kind of functionality in your proposal?

…to fix-inner-product Conflicts: 3rdparty/mkldnn

cmake/DownloadMKLML.cmake

pengzhao-intel · 2019-03-04T02:22:36Z

#14052

@TaoLv the 0.18 will fix this issue, please double check :)

…to fix-inner-product

This reverts commit a9b3886.

TaoLv · 2019-03-13T05:07:25Z

CI is finally passed. This PR is ready for review now. @pengzhao-intel @szha

pengzhao-intel · 2019-03-13T05:13:22Z

/~https://github.com/apache/incubator-mxnet/blob/6aa8c27a69cafc65eb97c4e077cb399462b617e2/tests/python/unittest/test_operator.py#L1634

Please add this test back, suppose this can be fixed.

TaoLv · 2019-03-13T14:39:46Z

incubator-mxnet/tests/python/unittest/test_operator.py

Line 1634 in 6aa8c27

@unittest.skip("Flaky test #14052")
Please add this test back, suppose this can be fixed.

I tried the test case and it passed on my local machine with this PR. But since it's flaky, I will submit another PR to re-enable it after this PR is merged. It deserves several times of CI run to make sure it's really fixed.

TaoLv · 2019-03-13T14:43:45Z

CMakeLists.txt

@@ -255,6 +255,7 @@ if(USE_MKLDNN)
  add_subdirectory(3rdparty/mkldnn)

  include_directories(3rdparty/mkldnn/include)
+  include_directories(${PROJECT_BINARY_DIR}/3rdparty/mkldnn/include)


In this folder, there is a new header file of MKL-DNN which is automatically generated during compilation. I doubt it would be used by Horovod and should be distributed with pip package. @szha @yuxihu

TaoLv · 2019-03-15T02:18:32Z

Ping for review, @szha , @zheng-da , @yuxihu , @pengzhao-intel .

pengzhao-intel

LGTM, please switch to release version before merging.

wkcn

Thanks for your contribution! LGTM

mkldnn.mk

…to fix-inner-product

pengzhao-intel · 2019-03-16T11:11:15Z

It's great that all CI passed.
@wkcn @szha please help confirm all concerns are resolved.

We will merge the PR in 24 hours in case there's no further concern.

wkcn

Thanks for Your contribution! LGTM

pengzhao-intel · 2019-03-17T09:14:15Z

Merging now :) Thanks all of the efforts from Tao and reviewers.

…che#13668) * update MKL-DNN to fix Dense layer issue(apache#13451) * fix format numbers * update mkldnn * update mkldnn to v0.18 release * update mklml version & fix mkldnn build * fix typo * fix cmake * fix cmake * skip test_quantized_conv * fix quantized_conv and cmake * Revert "fix cmake" This reverts commit a9b3886. * update mkldnn submodule * update mkldnn head to 0.18.1 patch release * empty commit to retrigger ci * retrigger ci

) * update MKL-DNN to fix Dense layer issue(#13451) * fix format numbers * update mkldnn * update mkldnn to v0.18 release * update mklml version & fix mkldnn build * fix typo * fix cmake * fix cmake * skip test_quantized_conv * fix quantized_conv and cmake * Revert "fix cmake" This reverts commit a9b3886. * update mkldnn submodule * update mkldnn head to 0.18.1 patch release * empty commit to retrigger ci * retrigger ci

…che#13668) * update MKL-DNN to fix Dense layer issue(apache#13451) * fix format numbers * update mkldnn * update mkldnn to v0.18 release * update mklml version & fix mkldnn build * fix typo * fix cmake * fix cmake * skip test_quantized_conv * fix quantized_conv and cmake * Revert "fix cmake" This reverts commit a9b3886. * update mkldnn submodule * update mkldnn head to 0.18.1 patch release * empty commit to retrigger ci * retrigger ci

TaoLv added 2 commits December 18, 2018 12:33

update MKL-DNN to fix Dense layer issue(apache#13451)

d29019c

fix format numbers

c86c10d

marcoabreu added the pr-awaiting-review PR is waiting for code review label Dec 19, 2018

TaoLv added 2 commits December 20, 2018 10:21

Merge branch 'master' of /~https://github.com/apache/incubator-mxnet in…

290f035

…to fix-inner-product

update mkldnn

e75a0ef

TaoLv requested a review from anirudh2290 as a code owner December 20, 2018 13:29

TaoLv added 2 commits December 24, 2018 09:41

Merge branch 'master' of /~https://github.com/apache/incubator-mxnet in…

c5de5c3

…to fix-inner-product

Merge branch 'master' of /~https://github.com/apache/incubator-mxnet in…

d612a02

…to fix-inner-product

sandeep-krishnamurthy added Operator MKLDNN labels Jan 28, 2019

TaoLv changed the title ~~Update MKL-DNN to fix the Dense layer issue~~ [WIP] Update MKL-DNN to fix the Dense layer issue Feb 17, 2019

TaoLv added 3 commits March 2, 2019 11:30

Merge branch 'master' of /~https://github.com/apache/incubator-mxnet in…

512794c

…to fix-inner-product Conflicts: 3rdparty/mkldnn

update mkldnn to v0.18 release

c2392a1

update mklml version & fix mkldnn build

0551ef3

TaoLv requested a review from szha as a code owner March 2, 2019 04:02

TaoLv changed the title ~~[WIP] Update MKL-DNN to fix the Dense layer issue~~ [WIP] Update MKL-DNN to v0.18 release (was: fix the Dense layer issue) Mar 2, 2019

szha reviewed Mar 2, 2019

View reviewed changes

cmake/DownloadMKLML.cmake Outdated Show resolved Hide resolved

fix typo

85b22f9

TaoLv added 8 commits March 4, 2019 23:06

Merge branch 'master' of /~https://github.com/apache/incubator-mxnet in…

849841d

…to fix-inner-product

fix cmake

a9b3886

Merge branch 'master' of /~https://github.com/apache/incubator-mxnet in…

99a8d10

…to fix-inner-product

skip test_quantized_conv

ac72fba

Merge branch 'master' of /~https://github.com/apache/incubator-mxnet in…

259f147

…to fix-inner-product

fix quantized_conv and cmake

38efc7e

Revert "fix cmake"

ff9be44

This reverts commit a9b3886.

update mkldnn submodule

b0e0add

TaoLv changed the title ~~[WIP] Update MKL-DNN to v0.18 release (was: fix the Dense layer issue)~~ Update MKL-DNN to v0.18 release (was: fix the Dense layer issue) Mar 13, 2019

TaoLv commented Mar 13, 2019

View reviewed changes

pengzhao-intel approved these changes Mar 15, 2019

View reviewed changes

wkcn approved these changes Mar 15, 2019

View reviewed changes

mkldnn.mk Show resolved Hide resolved

TaoLv added 4 commits March 16, 2019 10:09

update mkldnn head to 0.18.1 patch release

93cb7b8

Merge branch 'master' of /~https://github.com/apache/incubator-mxnet in…

a77c7a3

…to fix-inner-product

empty commit to retrigger ci

7500ed0

retrigger ci

eb12d84

wkcn approved these changes Mar 16, 2019

View reviewed changes

pengzhao-intel merged commit c2f939f into apache:master Mar 17, 2019

TaoLv mentioned this pull request Mar 18, 2019

Gluon dense layer gets segfault on large arrays when MKLDNN is built in MXNet. #13451

Closed

pengzhao-intel mentioned this pull request Mar 18, 2019

Update imagenet quantization script for MKL-DNN #14407

Closed

6 tasks

yuxihu mentioned this pull request May 6, 2019

Add mkldnn_version.h to pip package #14899

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MKL-DNN to v0.18 release (was: fix the Dense layer issue) #13668

Update MKL-DNN to v0.18 release (was: fix the Dense layer issue) #13668

TaoLv commented Dec 18, 2018

Roshrini commented Dec 19, 2018

TaoLv commented Jan 4, 2019

stu1130 commented Jan 16, 2019

sandeep-krishnamurthy commented Jan 28, 2019

pengzhao-intel commented Feb 3, 2019

pengzhao-intel commented Feb 3, 2019

ankkhedia commented Feb 14, 2019

TaoLv commented Feb 17, 2019 •

edited

Loading

pengzhao-intel commented Feb 17, 2019

TaoLv commented Feb 17, 2019

pengzhao-intel commented Feb 17, 2019

pengzhao-intel commented Mar 4, 2019

TaoLv commented Mar 13, 2019

pengzhao-intel commented Mar 13, 2019

TaoLv commented Mar 13, 2019

TaoLv Mar 13, 2019

TaoLv commented Mar 15, 2019

pengzhao-intel left a comment

wkcn left a comment

pengzhao-intel commented Mar 16, 2019

wkcn left a comment

pengzhao-intel commented Mar 17, 2019

Update MKL-DNN to v0.18 release (was: fix the Dense layer issue) #13668

Update MKL-DNN to v0.18 release (was: fix the Dense layer issue) #13668

Conversation

TaoLv commented Dec 18, 2018

Description

Checklist

Essentials

Changes

Comments

Roshrini commented Dec 19, 2018

TaoLv commented Jan 4, 2019

stu1130 commented Jan 16, 2019

sandeep-krishnamurthy commented Jan 28, 2019

pengzhao-intel commented Feb 3, 2019

pengzhao-intel commented Feb 3, 2019

ankkhedia commented Feb 14, 2019

TaoLv commented Feb 17, 2019 • edited Loading

pengzhao-intel commented Feb 17, 2019

TaoLv commented Feb 17, 2019

pengzhao-intel commented Feb 17, 2019

pengzhao-intel commented Mar 4, 2019

TaoLv commented Mar 13, 2019

pengzhao-intel commented Mar 13, 2019

TaoLv commented Mar 13, 2019

TaoLv Mar 13, 2019

Choose a reason for hiding this comment

TaoLv commented Mar 15, 2019

pengzhao-intel left a comment

Choose a reason for hiding this comment

wkcn left a comment

Choose a reason for hiding this comment

pengzhao-intel commented Mar 16, 2019

wkcn left a comment

Choose a reason for hiding this comment

pengzhao-intel commented Mar 17, 2019

TaoLv commented Feb 17, 2019 •

edited

Loading