Fix horovod build failure when mxnet is built from source #15213

apeforest · 2019-06-11T08:46:40Z

Description

The problem was due to the missing mkldnn_version.h file under include/mkldnn.

The mkldnn_version.h is generated at compile time by MKLDNN library. It is linked to 3rdparty/mkldnn/build/include when mxnet is built using make, the default build system currently supported in MXNet.

An earlier PR #14899 fixed the issue when mxnet is installed by pip. This PR is to fix the build error when mxnet is built from source.

Thanks @TaoLv for rootcausing the problem and his suggestions.

apeforest · 2019-06-11T08:47:59Z

@PatricZhao @yuxihu @TaoLv Please help to review.

TaoLv · 2019-06-11T09:42:52Z

Seems cmake is not happy with the change :(

apeforest · 2019-06-11T16:44:22Z

Seems cmake is not happy with the change :(

Unfortunately this is a hotfix that can unblock user from building horovod with mxnet from source. I agree we need a better solution in the long run to solve the dual building system issue and exporting header files. For 1.5 release, we can ask users to build using make, which I think is still the de facto build system of mxnet in 1.5 release.

yuxihu

Thanks for the fix. Please change mxnet_version.h to mkldnn_version.h in the PR description.

yuxihu · 2019-06-11T17:15:54Z

include/mkldnn/mkldnn.h

@@ -0,0 +1 @@
+../../3rdparty/mkldnn/include/mkldnn.h


why do we need to link this file? We only miss mkldnn_version.h, right?

Earlier it was linked to the entire 3rdparty/mkldnn/include folder. However it does not contain mkldnn_version.h there. In this change, I only link to required files instead of linking to the entire folder.

Did you verify if it works? I remember mkldnn.h may depend on other header files in the original folder.

Yes, I could verify it works fine in my local branch.

make -j$(nproc) export LD_LIBRARY_PATH=/usr/local/cuda/lib:$LD_LIBRARY_PATH pip install --no-cache-dir horovod

Here is the files in my include:

(mxnet) ubuntu@ip-172-31-7-88:~/src/mxnet$ ls include/mkldnn/ mkldnn.h mkldnn_version.h

Great. Please retrigger the CI.

I see the including chain is: ndarray.h -> mkldnn.hpp -> mkldnn.h -> mkldnn_types.h and mkldnn_version.h.

roywei · 2019-06-12T04:16:29Z

I don't think re-trigger CI is going to pass, is there any way to fix CMake errors in CI?

apeforest · 2019-06-12T17:55:26Z

@TaoLv This is not working because there are two issues in CI:
(1) ASAN is using Cmake and it will break
(2) CI lint complains about mxnet_version.h. Since it's a generated file at compile time, I cannot fix it.

After some more thoughts, I think a more elegant fix may be modifying the make script in MKLDNN module such that it copies mkldnn_version.h to the include folder after compilation is complete? This way it solves problem for both cmake and make, and it does not cause any CI issue in MXNet.

This problem is not limited to horovod. Any downstream project that needs to build mxnet from source will fail due to this. I think this is currently a blocker for MXNet release.

@PatricZhao Your advice is highly appreciated. Thanks!

marcoabreu · 2019-06-13T01:49:32Z

My two cents are that we must not break make as well as cmake. Both are supported and should be kept.

TaoLv · 2019-06-13T03:04:13Z

@apeforest I think we need add an install target to the Makefile and copy libraries and including files to the install destination. Downstream projects like horovod should find libraries and headers there, not in the source code of mxnet or mkldnn. @szha @marcoabreu

install:
       mkdir -p $(INSTALL_PREFIX)/lib/mxnet
       mkdir -p $(INSTALL_PREFIX)/include/mxnet
       cp -rf lib/* $(INSTALL_PREFIX)/lib/mxnet/
       cp -Lrf include/* $(INSTALL_PREFIX)/include/mxnet/

       if [ -e "lib/libmkldnn.so.0" ]; then \
               mkdir -p $(INSTALL_PREFIX)/include/mxnet/mkldnn; \
               cp -rf 3rdparty/mkldnn/build/install/include/* $(INSTALL_PREFIX)/include/mxnet/mkldnn/; \
       fi

apeforest · 2019-06-13T21:49:28Z

@TaoLv There is an issue with this approach:
When Horovod builds with mxnet, it actually uses mxnet.libinfo.find_include_path() to look for the header file location. With your suggested change, how can mxnet know the include path at runtime?

TaoLv · 2019-06-14T05:40:50Z

My two cents:

If you really want to copy files in source code folder, you can do that in the mkldnn.mk file. It will help to leave MXNet Makefile clean.
The change will generate new folder and file under include/mxnet folder which will be shown as untracked files when call git status and the new folder and file cannot be removed by make clean.

apeforest · 2019-06-14T18:46:10Z

@TaoLv Thanks for your suggestion.

I updated mkldnn.mk to leave Makefile clean
I updated .gitignore file so that you would not see untracked files in include/mkldnn
It can be cleaned by make clean.

Please help to review again.

vandanavk · 2019-06-14T23:09:49Z

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

roywei · 2019-06-15T07:26:37Z

Thanks for the fix!! I have verified this works. Please trigger CI again.
mxnet:

sudo make -j$(nproc) USE_CUDA=1 USE_CUDNN=1 USE_CUDA_PATH=/usr/local/cuda/ USE_NCCL=1

horovod:

 HOROVOD_GPU_ALLREDUCE=NCCL  pip install horovod --user --no-cache-dir -U

result:

Collecting horovod
  Downloading https://files.pythonhosted.org/packages/42/f8/0a2fedf45122d8a1b2dbd573e737ccb32cd0776aa4c4b157d3f18b9ff0ca/horovod-0.16.4.tar.gz (2.6MB)
    100% |████████████████████████████████| 2.6MB 87.4MB/s 
Requirement already satisfied, skipping upgrade: cloudpickle in /usr/local/lib/python2.7/dist-packages (from horovod) (1.0.0)
Requirement already satisfied, skipping upgrade: psutil in /home/ubuntu/.local/lib/python2.7/site-packages (from horovod) (5.6.3)
Requirement already satisfied, skipping upgrade: six in /usr/local/lib/python2.7/dist-packages (from horovod) (1.10.0)
Requirement already satisfied, skipping upgrade: cffi>=1.4.0 in /home/ubuntu/.local/lib/python2.7/site-packages (from horovod) (1.12.3)
Requirement already satisfied, skipping upgrade: pycparser in /home/ubuntu/.local/lib/python2.7/site-packages (from cffi>=1.4.0->horovod) (2.19)
Installing collected packages: horovod
  Running setup.py install for horovod ... done
Successfully installed horovod-0.16.4

git status

git status
On branch bugfix/horovod_build
Your branch is up-to-date with 'origin/bugfix/horovod_build'.
nothing to commit, working directory clean```

`sudo make clean` also removes it

pengzhao-intel

It's great that the horovod issue is fixed :)

@TaoLv please review the PR again

TaoLv · 2019-06-15T14:28:33Z

tools/license_header.py

@@ -68,6 +68,9 @@
               # Git submodules under different licenses
               '3rdparty',

+               # 3rdparty headerfiles under different licenses
+               'include/mkldnn',


Can you explain? MKL-DNN is also under Apache-2.0.

I imagine mkldnn would be licensed to intel instead of ASF even though the license is apache 2.0

license check would fail if this is not whitelisted.

TaoLv

Approve now. But just for the record, I don't really like this solution but take it as a hot fix to make horovod happy and to unblock the 1.5.0 release. We need consider a better way to expose headers to downstream projects. Looking for headers from source code is definitely not a good choice and is not guaranteed - that's why horovod's build was broken when MKL-DNN is updated.

szha · 2019-06-15T19:27:49Z

include/mkldnn

@@ -1 +0,0 @@
-../3rdparty/mkldnn/include


why not just change this to the install path?

Because we do not need to export all the header files under the install path.

I think it's simpler that way. Otherwise next time if there's any change mkldnn header we need to go through this all over again.

roywei · 2019-06-16T14:34:40Z

I have created feature request to better address this #15255

Can we merge this PR if it all looks good?

* fix horovod build when mxnet is built from source * copy header file in mkldnn * copy mkldnn_version header file to include * remove unnecessary line * fix the untracked file * whitelist mkldnn headerfile * exlude lint path

apeforest added 2 commits June 11, 2019 08:31

fix horovod build when mxnet is built from source

1ca7d65

Merge remote-tracking branch 'upstream/master' into bugfix/horovod_build

3d2b76f

apeforest requested review from anirudh2290 and eric-haibin-lin as code owners June 11, 2019 08:46

yuxihu reviewed Jun 11, 2019

View reviewed changes

yuxihu approved these changes Jun 11, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into bugfix/horovod_build

bf9a00f

copy header file in mkldnn

96919ab

apeforest requested a review from szha as a code owner June 14, 2019 00:08

Merge remote-tracking branch 'upstream/master' into bugfix/horovod_build

4a0492b

apeforest added 3 commits June 14, 2019 17:57

copy mkldnn_version header file to include

58cd271

remove unnecessary line

d805ad7

fix the untracked file

f19d05d

apeforest added 2 commits June 14, 2019 18:59

whitelist mkldnn headerfile

a8a12ec

exlude lint path

cfd1d9c

marcoabreu added Distributed pr-awaiting-review PR is waiting for code review labels Jun 14, 2019

roywei approved these changes Jun 15, 2019

View reviewed changes

pengzhao-intel approved these changes Jun 15, 2019

View reviewed changes

TaoLv reviewed Jun 15, 2019

View reviewed changes

TaoLv approved these changes Jun 15, 2019

View reviewed changes

szha reviewed Jun 15, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into bugfix/horovod_build

b8da91a

roywei mentioned this pull request Jun 16, 2019

[Feature request] better way to expose header files to downstream projects. #15255

Open

apeforest added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Jun 16, 2019

szha merged commit 85aaa3a into apache:master Jun 16, 2019

apeforest deleted the bugfix/horovod_build branch August 23, 2019 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix horovod build failure when mxnet is built from source #15213

Fix horovod build failure when mxnet is built from source #15213

apeforest commented Jun 11, 2019 •

edited

Loading

apeforest commented Jun 11, 2019

TaoLv commented Jun 11, 2019

apeforest commented Jun 11, 2019 •

edited

Loading

yuxihu left a comment

yuxihu Jun 11, 2019

apeforest Jun 11, 2019

yuxihu Jun 11, 2019

apeforest Jun 11, 2019 •

edited

Loading

yuxihu Jun 11, 2019

TaoLv Jun 12, 2019

roywei commented Jun 12, 2019

apeforest commented Jun 12, 2019 •

edited

Loading

marcoabreu commented Jun 13, 2019

TaoLv commented Jun 13, 2019

apeforest commented Jun 13, 2019

TaoLv commented Jun 14, 2019

apeforest commented Jun 14, 2019

vandanavk commented Jun 14, 2019

roywei commented Jun 15, 2019 •

edited

Loading

pengzhao-intel left a comment

TaoLv Jun 15, 2019

szha Jun 15, 2019

apeforest Jun 16, 2019

TaoLv left a comment

szha Jun 15, 2019

apeforest Jun 16, 2019

szha Jun 16, 2019

roywei commented Jun 16, 2019

		@@ -0,0 +1 @@
		../../3rdparty/mkldnn/include/mkldnn.h

		@@ -1 +0,0 @@
		../3rdparty/mkldnn/include

Fix horovod build failure when mxnet is built from source #15213

Fix horovod build failure when mxnet is built from source #15213

Conversation

apeforest commented Jun 11, 2019 • edited Loading

Description

apeforest commented Jun 11, 2019

TaoLv commented Jun 11, 2019

apeforest commented Jun 11, 2019 • edited Loading

yuxihu left a 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

apeforest Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roywei commented Jun 12, 2019

apeforest commented Jun 12, 2019 • edited Loading

marcoabreu commented Jun 13, 2019

TaoLv commented Jun 13, 2019

apeforest commented Jun 13, 2019

TaoLv commented Jun 14, 2019

apeforest commented Jun 14, 2019

vandanavk commented Jun 14, 2019

roywei commented Jun 15, 2019 • edited Loading

pengzhao-intel left a 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

TaoLv left a 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

roywei commented Jun 16, 2019

apeforest commented Jun 11, 2019 •

edited

Loading

apeforest commented Jun 11, 2019 •

edited

Loading

apeforest Jun 11, 2019 •

edited

Loading

apeforest commented Jun 12, 2019 •

edited

Loading

roywei commented Jun 15, 2019 •

edited

Loading