[MXNET-679] Improved CMakeLists.txt by lebeg · Pull Request #13576 · apache/mxnet

lebeg · 2018-12-07T13:00:20Z

Description

Part of #11148

Checklist

Essentials

The PR title start starts with [MXNET-679]
Changes are complete
Code is well-documented
To the my best knowledge, examples are not affected by this change

Changes

Renamed cmake/ChooseBlas.cmake to cmake/ChooseBLAS.cmake
Renamed USE_MKLML_MKL to USE_MKLML
Formatted CMakeLists.txt
Added documentation for BLAS options
Cleaned up CUDA language detection

marcoabreu · 2018-12-07T18:57:07Z

 USE_LAPACK: "ON" # Build with lapack support
 USE_MKL_IF_AVAILABLE: "ON" # Use MKL if found
-USE_MKLML_MKL: "ON" # Use MKLDNN variant of MKL (if MKL found) IF USE_MKL_IF_AVAILABLE AND (NOT APPLE)
+USE_MKLML: "ON" # Use MKLDNN variant of MKL (if MKL found) IF USE_MKL_IF_AVAILABLE AND (NOT APPLE)


Not sure about that one, especially considering it breaks compatibility and hides the initial intention to only use the mkl version
@TaoLv

There are no guarantees currently about CMake options compatibility.

hides the initial intention to only use the mkl version

I'm not sure I understand, what intention do you mean?

The comment states that this option is to specifically use the mkldnn variant of mkl and not the standalone mkldnn.

You are right that we do not guarantee compatibility, but I don't see a good reason here why we should break it intentionally. Could you elaborate?

Agree that compatibility is more important, although USE_MKLML_MKL is really redundant for me. Is it possible to give a warning message if USE_MKLML_MKL is provided and set its value to USE_MKLML? And remove this warning message and USE_MKLML_MKL totally in a major release?

You are right that we do not guarantee compatibility, but I don't see a good reason here why we should break it intentionally. Could you elaborate?

Because the name does not make sense and leads to confusion.

The comment states that this option is to specifically use the mkldnn variant of mkl and not the standalone mkldnn.

This is an example of the confusion. There is no such thing as mkldnn variant of mkl. There is MKLDNN - an independent addition to MKL. And there is a packed (and easier to download) version of MKL - MKLML.

What is and was wrong though is the comment about this variable, since the option is not related to MKLDNN.

Is it possible to give a warning message if USE_MKLML_MKL is provided and set its value to USE_MKLML?

I have added a warning.

Roshrini · 2018-12-21T21:43:48Z

@lebeg Can you check failing builds?

Roshrini · 2019-01-02T19:29:49Z

@mxnet-label-bot Update [pr-awaiting-response]

pengzhao-intel · 2019-01-09T14:35:54Z

@lebeg any progress in this PR?

someone reported the issue of cmake with MKLDNN as below.
Dose the fix include in this PR?

Thanks in advance.

https://discuss.gluon.ai/t/topic/5622/47

`cmake -DUSE_MKLDNN=1 -DBLAS=mkl -DMKL_ROOT=/opt/intel/mkl .
– The C compiler identification is GNU 5.4.0
– The CXX compiler identification is GNU 5.4.0
– Check for working C compiler: /usr/bin/cc
– Check for working C compiler: /usr/bin/cc – works
– Detecting C compiler ABI info
– Detecting C compiler ABI info - done
– Detecting C compile features
– Detecting C compile features - done
– Check for working CXX compiler: /usr/bin/c++
– Check for working CXX compiler: /usr/bin/c++ – works
– Detecting CXX compiler ABI info
– Detecting CXX compiler ABI info - done
– Detecting CXX compile features
– Detecting CXX compile features - done
– CMAKE_SYSTEM_NAME Linux
– CMake version ‘3.12.1’ using generator ‘Unix Makefiles’
– The CUDA compiler identification is NVIDIA 10.0.130
– Check for working CUDA compiler: /usr/local/cuda/bin/nvcc
– Check for working CUDA compiler: /usr/local/cuda/bin/nvcc – works
– Detecting CUDA compiler ABI info
– Detecting CUDA compiler ABI info - done
– Performing Test SUPPORT_CXX11
– Performing Test SUPPORT_CXX11 - Success
– Performing Test SUPPORT_CXX0X
– Performing Test SUPPORT_CXX0X - Success
– Performing Test SUPPORT_MSSE2
– Performing Test SUPPORT_MSSE2 - Success
– Determining F16C support
– Performing Test COMPILER_SUPPORT_MF16C
– Performing Test COMPILER_SUPPORT_MF16C - Success
– Downloading MKLML…
– Setting MKLROOT path to /home/usrname/Install/mxnet/mklml/mklml_lnx_2019.0.1.20180928
– CMAKE_BUILD_TYPE is unset, defaulting to Release
– Detecting Intel® MKL: trying mklml_intel
– Intel® MKL: include /home/usrname/Install/mxnet/mklml/mklml_lnx_2019.0.1.20180928/include
– Intel® MKL: lib /home/usrname/Install/mxnet/mklml/mklml_lnx_2019.0.1.20180928/lib/libmklml_intel.so
– Found OpenMP_C: -fopenmp (found version “4.0”)
– Found OpenMP_CXX: -fopenmp (found version “4.0”)
– Found OpenMP: TRUE (found version “4.0”)
– OpenMP lib: /home/usrname/Install/mxnet/mklml/mklml_lnx_2019.0.1.20180928/lib/libiomp5.so
– Found Doxygen: /usr/bin/doxygen (found version “1.8.11”) found components: doxygen dot
– VTune profiling environment is unset
– Looking for pthread.h
– Looking for pthread.h - found
– Looking for pthread_create
– Looking for pthread_create - not found
– Looking for pthread_create in pthreads
– Looking for pthread_create in pthreads - not found
– Looking for pthread_create in pthread
– Looking for pthread_create in pthread - found
– Found Threads: TRUE
– Found CUDA: /usr/local/cuda (found version “10.0”)
CMake Warning (dev) at cmake/ChooseBlas.cmake:23 (find_package):
Policy CMP0074 is not set: find_package uses _ROOT variables.
Run “cmake --help-policy CMP0074” for policy details. Use the cmake_policy
command to set the policy and suppress this warning.
CMake variable MKL_ROOT is set to:
/opt/intel/mkl

For compatibility, CMake is ignoring the variable.
Call Stack (most recent call first):
CMakeLists.txt:283 (include)
This warning is for project developers. Use -Wno-dev to suppress it.
– Could NOT find MKL (missing: MKLML_GNU_LIBRARY MKLDNN_LIBRARY)
CMake Error at /usr/local/share/cmake-3.12/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
Could NOT find MKL (missing: MKLML_GNU_LIBRARY MKLDNN_LIBRARY)
Call Stack (most recent call first):
/usr/local/share/cmake-3.12/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
cmake/Modules/FindMKL.cmake:199 (find_package_handle_standard_args)
cmake/ChooseBlas.cmake:47 (find_package)
CMakeLists.txt:283 (include)
– Configuring incomplete, errors occurred!`

marcoabreu · 2019-01-09T16:31:20Z

@pengzhao-intel could you post an alternative link? It looks quite suspicious and I have defused it for now.

pengzhao-intel · 2019-01-10T02:44:49Z

@marcoabreu Edited. It's Chinese forum and you can try google translation on the web :)

I heard it's a known issue for linking MKL BLAS in CMAKE for a while.
Currently, we have the Intel MKL test on CI. So I proposed to add the CI for CMAKE + MKL.

I think we need to increase the priority to fix it. Any idea?

@lupesko @azai91 @mseth10

lebeg · 2019-01-10T13:56:20Z

@pengzhao-intel

Does the fix include in this PR?

Yes, probably. I have tested it with full MKL and MKLML lib.

From what I can see in the cmake log above it seems that the downloaded MKLML is used in one case and MKL from MKL_ROOT (/opt/intel/mkl) in other. Maybe resetting the MKL_ROOT environment variable might help.

TaoLv · 2019-01-10T14:24:20Z

@lebeg Could you rebase and address the conflicts then we can move forward?

lebeg · 2019-01-10T14:51:16Z

@pengzhao-intel

Does the fix include in this PR?

Actually, I was wrong. This PR doesn't touch FindMKL.cmake. That was fixed in #11148 but never went through.

pengzhao-intel · 2019-01-11T01:42:23Z

@lebeg really thanks for the confirm. I see #11148 is closed.

I think the proposed change is very important and suggest to continue this PR and fix the known issue in CMake and merge earlier.

Feel free to let me and @TaoLv know if anything we can help.

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

@lebeg - This is super useful PR. Can you please help take this forward?

mseth10 · 2019-01-28T21:13:57Z

        -DUSE_CUDNN=1                           \
-        -DUSE_MKLML_MKL=1                       \
+        -DUSE_MKLML=1                           \
+        -DUSE_MKLDNN=1                          \


USE_MKLDNN=1 is default here. No need to turn it explicitly on.

vandanavk · 2019-02-06T00:22:16Z

@lebeg could you re-trigger CI?

anirudhacharya · 2019-03-01T22:34:22Z

@lebeg please update the PR as per the comments.

karan6181 · 2019-03-19T00:28:44Z

@lebeg Could you please re-trigger the CI again since some tests are failing and can you please address the comments? Thank you!

piyushghai · 2019-04-09T00:28:12Z

@lebeg Can you rebase with the current master branch ? There seems to be a conflict.
Is this PR good to go after that ?

lebeg · 2019-04-09T13:13:06Z

It seems that there are more problems. I'm closing this for now as I don't have time for it right now. It would be really great to see somebody picking it up from here.

lebeg requested a review from szha as a code owner December 7, 2018 13:00

lebeg force-pushed the cmake branch from 7720306 to 27d5667 Compare December 7, 2018 17:00

marcoabreu reviewed Dec 7, 2018

View reviewed changes

nswamy added pr-awaiting-review PR is waiting for code review and removed pr-awaiting-review PR is waiting for code review labels Dec 8, 2018

lebeg force-pushed the cmake branch 2 times, most recently from bc40150 to 1df10d3 Compare December 9, 2018 08:05

marcoabreu added pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed pr-awaiting-review PR is waiting for code review labels Jan 2, 2019

lebeg force-pushed the cmake branch from f28f512 to 9c7dc33 Compare January 10, 2019 14:49

pengzhao-intel mentioned this pull request Jan 15, 2019

Cmake : could not find mkl (missing MKLDNN_LIBRARY) #13881

Closed

mseth10 reviewed Jan 28, 2019

View reviewed changes

lebeg force-pushed the cmake branch 2 times, most recently from ff5fa60 to 313e6f4 Compare January 30, 2019 09:37

lebeg force-pushed the cmake branch 2 times, most recently from a5dc272 to 8acc247 Compare February 15, 2019 09:42

lebeg force-pushed the cmake branch from 8acc247 to 7465a25 Compare March 26, 2019 09:53

lebeg added 2 commits April 9, 2019 11:46

Improved CMakeLists.txt

4b2c909

Added USE_MKLML_MKL usage warning

d8cabdc

lebeg force-pushed the cmake branch from 7465a25 to d8cabdc Compare April 9, 2019 09:47

lebeg closed this Apr 9, 2019

Conversation

lebeg commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Uh oh!

marcoabreu Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

lebeg Dec 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoabreu Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

TaoLv Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

lebeg Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lebeg Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

Roshrini commented Dec 21, 2018

Uh oh!

Roshrini commented Jan 2, 2019

Uh oh!

pengzhao-intel commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcoabreu commented Jan 9, 2019

Uh oh!

pengzhao-intel commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lebeg commented Jan 10, 2019

Uh oh!

TaoLv commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lebeg commented Jan 10, 2019

Uh oh!

pengzhao-intel commented Jan 11, 2019

Uh oh!

sandeep-krishnamurthy commented Jan 28, 2019

Uh oh!

mseth10 Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

vandanavk commented Feb 6, 2019

Uh oh!

anirudhacharya commented Mar 1, 2019

Uh oh!

karan6181 commented Mar 19, 2019

Uh oh!

piyushghai commented Apr 9, 2019

Uh oh!

lebeg commented Apr 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

lebeg commented Dec 7, 2018 •

edited

Loading

lebeg Dec 8, 2018 •

edited

Loading

lebeg Dec 9, 2018 •

edited

Loading

pengzhao-intel commented Jan 9, 2019 •

edited

Loading

pengzhao-intel commented Jan 10, 2019 •

edited

Loading

TaoLv commented Jan 10, 2019 •

edited

Loading