-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-83] Fix CMake build issue with MKL. #10075
Conversation
becec31
to
caaaea1
Compare
Looks like this is building properly in CI and locally for me. @cjolivier01 would you be able to have a look and see if there's any issues with the change? |
Not sure. At this point, I really don't know what's going on with the mkl build, what works with what, does mkldnn use mklml or mkl? can mkldnn operate on its own, what combinations work, etc. |
ci/docker/runtime_functions.sh
Outdated
@@ -278,8 +278,6 @@ build_ubuntu_gpu_cmake() { | |||
cmake \ | |||
-DUSE_CUDA=1 \ | |||
-DUSE_CUDNN=1 \ | |||
-DUSE_MKLML_MKL=0 \ | |||
-DUSE_MKLDNN=0 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one was to explicitely test without MKLDNN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the value in explicitly turning off MKL, but what we want to do here is test the default build. I think CI should reflect how users are likely to actually use a project, and I don't think they're likely to explicitly turn off all the settings they're not using. We can certainly do both if you prefer, but in my mind it's more important to ensure basic builds work.
If you feel strongly these should stay in I'll amend the commit, after this patch it should work in either case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case is handled by build_ubuntu_gpu_cmake_mkldnn. The idea here is to create a variety of environments and different build configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build_ubuntu_gpu_cmake_mkldnn explicitly turns on MKL, which is fine because if you're opting into a feature you know what that feature is. My intent here was to test the default behaviour (what a user is likely to do). I.e. what happens when you don't explicitly change any other option other than CUDA.
@cjolivier01 MKL-DNN can work standalone (you can build it without MKL/MKLML and it will perform great). MKL-DNN uses MKL/MKLML only to accelerate inner product via gemm from MKL/MKLML. MKLML is a small library, which contains part of MKL functionality (ML related). update: information from MKL-DNN team for MKLML.
|
@pengzhao-intel Would also be great if you could review the PR. Does it look ok from your point of view? |
caaaea1
to
b5ac365
Compare
b5ac365
to
3314bec
Compare
@pengzhao-intel thank you for the explanation. Is this information documented somewhere on the Intel or mxnet docs ? |
I am not sure if the mkl flags in cmake still follow those, but I don't think these changes affect that behavior (allowable overlap of mkl libraries). |
@anirudh2290 it's in the MKL-DNN readme by "Intel MKL small libraries" and I am working on improving the explanations. I will update these descriptions into MXNET doc later. @KellenSunderland I am looking the whole logic. And as @cjolivier01 mentioned, the current building logic is not very clear. The CMake and Makefile flow is not consistency. |
@pengzhao-intel great to have a look over the whole system, but what is clear to me given the current setup is: (Assuming MKL flags left at default values) This change won't affect MKLDNN usage, but it makes sure we're only setting MKL_FOUND if the full MKL installation is present. |
@KellenSunderland Agree, this change is good. We can merge it. |
Description
Fixes issue #10072
I'd like to do a test run in CI to make sure this doesn't have unintended side effects.