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

[v1.x] Mkldnn header fix v1x for nightly binaries #18797

Merged
merged 3 commits into from Aug 3, 2020
Merged

[v1.x] Mkldnn header fix v1x for nightly binaries #18797

merged 3 commits into from Aug 3, 2020

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Jul 27, 2020 •
edited

Description

Nightly binaries available at https://dist.mxnet.io/python with the label 1.7.0 are built from 1.x branch.
So to fix the MKLDNN header missing issue in Nightly binaries, we need to cherry-pick it into 1.x branch

Cherrypicks #18608

@MoisesHer @mseth10 @leezu @szha

MoisesHer and others added 2 commits July 27, 2020 10:53
@MoisesHer @leezu @ChaiBapchya
Cherry-pick apache#18310 apache#18355 (apache#18608)
f0cf1de 
* cherry-pick: Fix missing MKLDNN headers (apache#18310)

* Include all mkldnn headers in CD builds (apache#18355)

* Fix cmake mkldnn install target. Previously mkldnn headers are installed to CMAKE_INSTALL_INCLUDEDIR instead of CMAKE_INSTALL_INCLUDEDIR/mkldnn

* Fix pypi_package.sh pip/setup.py for mkldnn builds

* Set CMAKE_CUDA_COMPILER in aarch64-linux-gnu-toolchain.cmake (apache#18713)

CMAKE_CUDA_HOST_COMPILER will be reset if CMAKE_CUDA_COMPILER is not set as of cmake 3.17.3

See https://gitlab.kitware.com/cmake/cmake/-/issues/20826

Co-authored-by: Leonard Lausen <lausen@amazon.com>
@ChaiBapchya
remove linux-gputoolchain
51dc03c
@ChaiBapchya ChaiBapchya requested a review from szha as a code owner July 27, 2020 18:07
@mxnet-bot
Copy link

Hey @ChaiBapchya , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [unix-cpu, edge, windows-gpu, miscellaneous, centos-gpu, windows-cpu, website, sanity, unix-gpu, centos-cpu, clang]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@szha szha requested a review from leezu July 27, 2020 19:30
@lanking520 lanking520 added the pr-awaiting-review PR is waiting for code review label Jul 28, 2020
szha
szha reviewed Jul 30, 2020
View reviewed changes
tools/pip/setup.py
Comment on lines +159 to +161
if variant != 'NATIVE':
shutil.copytree(os.path.join(CURRENT_DIR, 'mxnet-build/3rdparty/mkldnn/include'),
os.path.join(CURRENT_DIR, 'mxnet/include/mkldnn'))
Copy link
Member

Choose a reason for hiding this comment

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

there's a flag in mxnet runtime library on whether mkldnn is enabled. instead of relying on variant it will be more robust to switch to that as condition instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean something like this: mxnet.runtime.Features.is_enabled("MKLDNN") ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh bummer I forgot to replace that if condition with runtime.Features

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#19195

@szha
Copy link
Member

szha commented Jul 30, 2020

I think when enabling the branch protection, we accidentally turned on "Require branches to be up to date before merging". I'm requesting to disable it in https://issues.apache.org/jira/browse/INFRA-20616. Don't worry about updating the branch in this PR for now.

@ChaiBapchya
Copy link
Contributor Author

Sure. Thanks Sheng for jumping in. I'll wait until the branch protection is fixed.

@szha szha merged commit 2aa2702 into apache:v1.x Aug 3, 2020
@ChaiBapchya ChaiBapchya deleted the mkldnn_header_fix_v1x_nightly branch August 4, 2020 04:52
@ChaiBapchya
Copy link
Contributor Author

@MoisesHer since this is merged, nightly binary of 4th Aug should have mkldnn. Can you verify?

@ChaiBapchya
Copy link
Contributor Author

I can confirm mkldnn headers exist

wget https://repo.mxnet.io/dist/python/cpu/mxnet-1.7.0b20200803-py2.py3-none-manylinux2014_x86_64.whl
unzip mxnet-1.7.0b20200803-py2.py3-none-manylinux2014_x86_64.whl -d 17_check_mkl
cd 17_check_mkl
find . -name "*mkl*"

Output

./mxnet/include/mkldnn
./mxnet/include/mkldnn/mkldnn_debug.h
./mxnet/include/mkldnn/mkldnn_types.h
./mxnet/include/mkldnn/mkldnn.h
./mxnet/include/mkldnn/mkldnn_version.h
./mxnet/include/mkldnn/mkldnn_config.h
./mxnet/include/mkldnn/mkldnn_dnnl_mangling.h
./mxnet/include/mkldnn/mkldnn.hpp

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@szha szha szha left review comments

@leezu leezu Awaiting requested review from leezu

Assignees
No one assigned
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ChaiBapchya @mxnet-bot @szha @lanking520 @MoisesHer