Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Enable MKL-DNN in pip packages #16899

Merged
merged 18 commits into from Feb 15, 2020
Merged

Enable MKL-DNN in pip packages #16899

merged 18 commits into from Feb 15, 2020

Conversation

TaoLv
Copy link
Member

@TaoLv TaoLv commented Nov 25, 2019

Description

Discussion: https://lists.apache.org/thread.html/1a22dbd79098adab6d02d16e8d607bae2acc908c0bb1b085d28a51ba@%3Cdev.mxnet.apache.org%3E

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 https://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

@TaoLv TaoLv requested a review from szha as a code owner November 25, 2019 07:53
@pengzhao-intel pengzhao-intel added this to In progress in CPU Performance and Quantization via automation Nov 25, 2019
@TaoLv
Copy link
Member Author

TaoLv commented Nov 25, 2019

@lanking520 @perdasilva

Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the changes, it doesn't quite make sense to keep the *mkl variants anymore, since that's the only difference.

@TaoLv
Copy link
Member Author

TaoLv commented Nov 27, 2019

I'm not sure whether the backward compatibility matters for convenient binary releases or not. What about if downstream projects explicitly call pip install mxnet-cu90mkl in their scripts [1]?
@pengzhao-intel suggested to keep these *mkl variants till 2.0 major release [2].

[1]https://github.com/dmlc/gluon-cv/blob/master/tests/py3.yml#L19
[2]https://lists.apache.org/thread.html/87282a60cd73257d3dd4b57401ffe2eabd293d77456d9966b1f91336@%3Cdev.mxnet.apache.org%3E

@pengzhao-intel
Copy link
Contributor

@szha does this make sense to keep *mkl variants for a short time? Or we need to remove from now.
@marcoabreu @samskalicky @larroy any comments?

I am going to merging the PR in the early of next week in case there is no objection.

@szha
Copy link
Member

szha commented Dec 1, 2019

Not really. Given that the content would be the same, having both would just waste space.

@wkcn
Copy link
Member

wkcn commented Dec 1, 2019

I think keeping *mkl is better, since users may use *mkl to install MXNet.
To save space, we can add a dummy package, whose dependency is MXNet package, and raise the deprecated warning.

@pengzhao-intel
Copy link
Contributor

Not really. Given that the content would be the same, having both would just waste space.

@szha @wkcn Agree. We will try if dummy package works now.

@TaoLv
Copy link
Member Author

TaoLv commented Dec 4, 2019

For these .mk files, I git rm those non-mkl variants and then git mv those mkl variants to non-mkl.

…to release-mkldnn

Conflicts:
	make/pip/pip_linux_mkl.mk
	make/pip/pip_linux_native.mk
	make/staticbuild/darwin_mkl.mk
	make/staticbuild/linux_cu100mkl.mk
	make/staticbuild/linux_cu101mkl.mk
	make/staticbuild/linux_cu75mkl.mk
	make/staticbuild/linux_cu80mkl.mk
	make/staticbuild/linux_cu90mkl.mk
	make/staticbuild/linux_cu91mkl.mk
	make/staticbuild/linux_cu92mkl.mk
	make/staticbuild/linux_mkl.mk
@TaoLv
Copy link
Member Author

TaoLv commented Dec 26, 2019

@szha @perdasilva Could you please review? Is it possible to verify these changes in dev CD? Thanks.

@TaoLv TaoLv changed the title [WIP] Enable MKL-DNN in pip packages Enable MKL-DNN in pip packages Dec 26, 2019
@@ -29,6 +29,12 @@

#include <Rcpp.h>
#include <string>
#include <opencv2/core/version.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related or is your branch out of date?

@larroy
Copy link
Contributor

larroy commented Jan 4, 2020

Can you rebase?

@TaoLv
Copy link
Member Author

TaoLv commented Jan 4, 2020

Can you rebase?

Done.

CPU Performance and Quantization automation moved this from In progress to Reviewer approved Jan 26, 2020
…to release-mkldnn

Conflicts:
	tools/staticbuild/README.md
@TaoLv
Copy link
Member Author

TaoLv commented Feb 11, 2020

Thank you @samskalicky. I over looked these files when merging the master branch. Will update them.

@@ -45,6 +44,7 @@ CMAKE_STATICBUILD=1 tools/staticbuild/build.sh cpu

For the CMake build, you need to install `patchelf` first, for example via `apt
install patchelf` on Ubuntu systems.
>>>>>>> ae145cdb1de95e8275dd087585f4aab69bc94005
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be a leftover from resolving merge conflict

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It's fixed.

Copy link
Contributor

@samskalicky samskalicky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I restarted your windows GPU run since the cmake.exe error is still flaky. But im not sure about the website error:

[2020-02-11T02:14:04.984Z] Extension error:
[2020-02-11T02:14:04.984Z] Could not import extension autodocsumm (exception: cannot import name 'Signature' from 'sphinx.ext.autodoc' (unknown location))
[2020-02-11T02:14:05.247Z] make[1]: *** [html] Error 2
[2020-02-11T02:14:05.247Z] Makefile:95: recipe for target 'html' failed
[2020-02-11T02:14:05.247Z] make[1]: Leaving directory '/work/mxnet/docs/python_docs/python/build'

@aaronmarkham any idea?

@TaoLv
Copy link
Member Author

TaoLv commented Feb 11, 2020

Thank you @samskalicky . See #17560 for the website faliure.

@TaoLv
Copy link
Member Author

TaoLv commented Feb 14, 2020

@szha @leezu @samskalicky Is it good to go? Let me know if anything need be adjusted. Thanks.

@leezu
Copy link
Contributor

leezu commented Feb 14, 2020

@TaoLv Sheng just merged fixes for the CD pipeline. Let's wait 24 hours to verify CD works on master branch. Then we can merge this PR.

@leezu leezu merged commit 2f6cdd3 into apache:master Feb 15, 2020
CPU Performance and Quantization automation moved this from Reviewer approved to Done Feb 15, 2020
@szha
Copy link
Member

szha commented Feb 16, 2020

Looks like the update has taken effect, though the jenkins jobs are still running for *mkl variants. http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/restricted-mxnet-cd%2Fmxnet-cd-release-job/detail/mxnet-cd-release-job/724/pipeline

@TaoLv
Copy link
Member Author

TaoLv commented Feb 17, 2020

@szha What can I do for this situation? It seems we need change the value of MXNET_VARIANTS in the build environment.

zheyuye pushed a commit to zheyuye/incubator-mxnet that referenced this pull request Feb 19, 2020
* enable mkldnn by default in pip packages

* add make file for native build

* rm *mkl build scripts

* remove *mkl variants

* Change readme/cd

* clear native build configurations

* fix static build test in ci

* git mv linux_cu102mkl.mk linux_cu102.mk

* fix merge conflict
@leezu leezu mentioned this pull request Mar 5, 2020
leezu added a commit that referenced this pull request Mar 5, 2020
* Fix cd/mxnet_lib/dynamic/Jenkins_pipeline.groovy

Fixes a regression in CD introduced by
#17645

* Fix whitespace

* Add NATIVE_ADDITIONAL.md

Fixes a regression in CD introduced by
#16899

* Update other $TYPE_ADDITIONAL.MD

* Fix cd/python/docker

Fixes regression introduced by
#15990
@leezu leezu mentioned this pull request Mar 6, 2020
MoisesHer pushed a commit to MoisesHer/incubator-mxnet that referenced this pull request Apr 10, 2020
* Fix cd/mxnet_lib/dynamic/Jenkins_pipeline.groovy

Fixes a regression in CD introduced by
apache#17645

* Fix whitespace

* Add NATIVE_ADDITIONAL.md

Fixes a regression in CD introduced by
apache#16899

* Update other $TYPE_ADDITIONAL.MD

* Fix cd/python/docker

Fixes regression introduced by
apache#15990
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request May 29, 2020
* enable mkldnn by default in pip packages

* add make file for native build

* rm *mkl build scripts

* remove *mkl variants

* Change readme/cd

* clear native build configurations

* fix static build test in ci

* git mv linux_cu102mkl.mk linux_cu102.mk

* fix merge conflict
anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request May 29, 2020
* Fix cd/mxnet_lib/dynamic/Jenkins_pipeline.groovy

Fixes a regression in CD introduced by
apache#17645

* Fix whitespace

* Add NATIVE_ADDITIONAL.md

Fixes a regression in CD introduced by
apache#16899

* Update other $TYPE_ADDITIONAL.MD

* Fix cd/python/docker

Fixes regression introduced by
apache#15990
mseth10 pushed a commit to mseth10/incubator-mxnet that referenced this pull request Jun 2, 2020
* Fix cd/mxnet_lib/dynamic/Jenkins_pipeline.groovy

Fixes a regression in CD introduced by
apache#17645

* Fix whitespace

* Add NATIVE_ADDITIONAL.md

Fixes a regression in CD introduced by
apache#16899

* Update other $TYPE_ADDITIONAL.MD

* Fix cd/python/docker

Fixes regression introduced by
apache#15990
mseth10 pushed a commit to mseth10/incubator-mxnet that referenced this pull request Jun 2, 2020
* Fix cd/mxnet_lib/dynamic/Jenkins_pipeline.groovy

Fixes a regression in CD introduced by
apache#17645

* Fix whitespace

* Add NATIVE_ADDITIONAL.md

Fixes a regression in CD introduced by
apache#16899

* Update other $TYPE_ADDITIONAL.MD

* Fix cd/python/docker

Fixes regression introduced by
apache#15990
leezu added a commit that referenced this pull request Jun 9, 2020
* Fix CD (#17776)

* Fix cd/mxnet_lib/dynamic/Jenkins_pipeline.groovy

Fixes a regression in CD introduced by
#17645

* Fix whitespace

* Add NATIVE_ADDITIONAL.md

Fixes a regression in CD introduced by
#16899

* Update other $TYPE_ADDITIONAL.MD

* Fix cd/python/docker

Fixes regression introduced by
#15990

* [CD] update pypi description, setup.py (#17681)

* update pypi description, setup.py, use manylinux2010, use unified dist link for nightly

* Use manylinux2014

Co-authored-by: Leonard Lausen <leonard@lausen.nl>

* reverting .so path as per MAKE flow

Co-authored-by: Leonard Lausen <lausen@amazon.com>
Co-authored-by: Sheng Zha <szha@users.noreply.github.com>
Co-authored-by: Leonard Lausen <leonard@lausen.nl>
leezu added a commit that referenced this pull request Jun 15, 2020
* Fix CD (#17776)

* Fix cd/mxnet_lib/dynamic/Jenkins_pipeline.groovy

Fixes a regression in CD introduced by
#17645

* Fix whitespace

* Add NATIVE_ADDITIONAL.md

Fixes a regression in CD introduced by
#16899

* Update other $TYPE_ADDITIONAL.MD

* Fix cd/python/docker

Fixes regression introduced by
#15990

* [CD] update pypi description, setup.py (#17681)

* update pypi description, setup.py, use manylinux2010, use unified dist link for nightly

* Use manylinux2014

Co-authored-by: Leonard Lausen <leonard@lausen.nl>

* reverting .so path as per MAKE flow

Co-authored-by: Leonard Lausen <lausen@amazon.com>
Co-authored-by: Sheng Zha <szha@users.noreply.github.com>
Co-authored-by: Leonard Lausen <leonard@lausen.nl>
ChaiBapchya pushed a commit to ChaiBapchya/mxnet that referenced this pull request Aug 15, 2020
* Fix CD (apache#17776)

* Fix cd/mxnet_lib/dynamic/Jenkins_pipeline.groovy

Fixes a regression in CD introduced by
apache#17645

* Fix whitespace

* Add NATIVE_ADDITIONAL.md

Fixes a regression in CD introduced by
apache#16899

* Update other $TYPE_ADDITIONAL.MD

* Fix cd/python/docker

Fixes regression introduced by
apache#15990

* [CD] update pypi description, setup.py (apache#17681)

* update pypi description, setup.py, use manylinux2010, use unified dist link for nightly

* Use manylinux2014

Co-authored-by: Leonard Lausen <leonard@lausen.nl>

* reverting .so path as per MAKE flow

Co-authored-by: Leonard Lausen <lausen@amazon.com>
Co-authored-by: Sheng Zha <szha@users.noreply.github.com>
Co-authored-by: Leonard Lausen <leonard@lausen.nl>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants