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

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

Merged
merged 11 commits into from
Jun 16, 2019
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1ca7d65
fix horovod build when mxnet is built from source
apeforest Jun 11, 2019
3d2b76f
Merge remote-tracking branch 'upstream/master' into bugfix/horovod_build
apeforest Jun 11, 2019
bf9a00f
Merge remote-tracking branch 'upstream/master' into bugfix/horovod_build
apeforest Jun 13, 2019
96919ab
copy header file in mkldnn
apeforest Jun 14, 2019
4a0492b
Merge remote-tracking branch 'upstream/master' into bugfix/horovod_build
apeforest Jun 14, 2019
58cd271
copy mkldnn_version header file to include
apeforest Jun 14, 2019
d805ad7
remove unnecessary line
apeforest Jun 14, 2019
f19d05d
fix the untracked file
apeforest Jun 14, 2019
a8a12ec
whitelist mkldnn headerfile
apeforest Jun 14, 2019
cfd1d9c
exlude lint path
apeforest Jun 14, 2019
b8da91a
Merge remote-tracking branch 'upstream/master' into bugfix/horovod_build
apeforest Jun 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,4 +161,7 @@ tests/mxnet_unit_tests
coverage.xml

# Local CMake build config
cmake_options.yml
cmake_options.yml

# header file generated at compile time
include/mkldnn/mkldnn_version.h
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ lint: cpplint rcpplint jnilint pylint

cpplint:
3rdparty/dmlc-core/scripts/lint.py mxnet cpp include src plugin cpp-package tests \
--exclude_path src/operator/contrib/ctc_include
--exclude_path src/operator/contrib/ctc_include include/mkldnn

pylint:
python3 -m pylint --rcfile=$(ROOTDIR)/ci/other/pylintrc --ignore-patterns=".*\.so$$,.*\.dll$$,.*\.dylib$$" python/mxnet tools/caffe_converter/*.py
Expand Down
1 change: 0 additions & 1 deletion include/mkldnn
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions include/mkldnn/mkldnn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
../../3rdparty/mkldnn/include/mkldnn.h
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@apeforest apeforest Jun 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Great. Please retrigger the CI.

Copy link
Member

Choose a reason for hiding this comment

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

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

1 change: 1 addition & 0 deletions include/mkldnn/mkldnn_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
../../3rdparty/mkldnn/include/mkldnn_types.h
3 changes: 2 additions & 1 deletion mkldnn.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ ifeq ($(USE_MKLDNN), 1)
MKLDNN_SUBMODDIR = $(ROOTDIR)/3rdparty/mkldnn
MKLDNN_BUILDDIR = $(MKLDNN_SUBMODDIR)/build
MXNET_LIBDIR = $(ROOTDIR)/lib
MXNET_INCLDIR = $(ROOTDIR)/include
ifeq ($(UNAME_S), Darwin)
OMP_LIBFILE = $(MKLDNNROOT)/lib/libiomp5.dylib
MKLML_LIBFILE = $(MKLDNNROOT)/lib/libmklml.dylib
Expand Down Expand Up @@ -49,7 +50,7 @@ $(MKLDNN_LIBFILE):
cp $(OMP_LIBFILE) $(MXNET_LIBDIR)
cp $(MKLML_LIBFILE) $(MXNET_LIBDIR)
cp $(MKLDNN_LIBFILE) $(MXNET_LIBDIR)

cp $(MKLDNN_BUILDDIR)/include/mkldnn_version.h $(MXNET_INCLDIR)/mkldnn/.
mkldnn_clean:
$(RM) -r 3rdparty/mkldnn/build
$(RM) -r $(MKLDNNROOT)
Expand Down
3 changes: 3 additions & 0 deletions tools/license_header.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@
# Git submodules under different licenses
'3rdparty',

# 3rdparty headerfiles under different licenses
'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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

license check would fail if this is not whitelisted.


# Code shared with project by author - see file for details
'src/operator/special_functions-inl.h',

Expand Down