-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Making MKL-DNN default on MXNet master #13681
Changes from 111 commits
1e17a51
abbc3ad
92b91f8
335748b
ce5336c
d12d2cd
b8a0203
bc6c482
15a41fc
42b3353
5af258a
32ab9ce
e2422d6
372f697
67e4dff
150b324
890cf1d
89b11c6
da8f62c
f29c254
40fd0ac
78c6093
cb095c6
c08f6fa
0302290
bf78666
23fd7d9
d103ec8
46da874
9b60119
4b07dcf
6536cda
de4ff31
0e5a362
6eadfae
c9e8bd2
0d0f407
b300b88
b336ef0
9827a5b
b9be823
fdcee0d
46ee0bd
7772cdd
45e8cd8
ed31e12
746596c
ab2ec74
6a02949
9ba1181
3cc21f3
0b894a0
6b4db54
a754793
4ad0b29
ccf9855
225b446
54231a9
2d2a0f9
884d955
7dfa87e
409acd0
8422287
346a602
20741c4
4e6b2ca
992a2a0
45fd008
7934cf1
b40e996
74e86e6
5a18a8f
c0bd964
2dc1683
4eb65df
ec5421d
75dd532
2af09a8
76d842f
103c9d1
edc3bd1
d510436
16cca19
ab75373
63821d4
13bf7ae
935b1dc
96482f7
59bae57
6d583df
37701b0
83c36d5
c94129a
0220572
2384a90
377db81
365838d
dfaef51
9f7e869
c1a5d2c
3ec81a1
2a3d362
bd9e55b
bd55a1c
c68ccd0
8c82763
670c3ae
130ec16
b7330cc
fe4b20e
38b3d66
b554519
f770f8a
8da6078
28ba12c
10e40a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -268,7 +268,7 @@ def compile_centos7_cpu_mkldnn() { | |
timeout(time: max_time, unit: 'MINUTES') { | ||
utils.init_git() | ||
utils.docker_run('centos7_cpu', 'build_centos7_mkldnn', false) | ||
utils.pack_lib('centos7_mkldnn', mx_lib, true) | ||
utils.pack_lib('centos7_mkldnn', mx_mkldnn_lib, true) | ||
} | ||
} | ||
} | ||
|
@@ -289,6 +289,20 @@ def compile_centos7_gpu() { | |
}] | ||
} | ||
|
||
def compile_centos7_gpu_mkldnn() { | ||
return ['GPU: CentOS 7 MKLDNN': { | ||
node(NODE_LINUX_CPU) { | ||
ws('workspace/build-centos7-gpu-mkldnn') { | ||
timeout(time: max_time, unit: 'MINUTES') { | ||
utils.init_git() | ||
utils.docker_run('centos7_gpu', 'build_centos7_gpu_mkldnn', false) | ||
utils.pack_lib('centos7_gpu_mkldnn', mx_mkldnn_lib, true) | ||
} | ||
} | ||
} | ||
}] | ||
} | ||
|
||
def compile_unix_clang_3_9_cpu() { | ||
return ['CPU: Clang 3.9': { | ||
node(NODE_LINUX_CPU) { | ||
|
@@ -992,6 +1006,25 @@ def test_centos7_python3_cpu() { | |
}] | ||
} | ||
|
||
def test_centos7_python3_cpu_mkldnn() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need new test for MKL-DNN default? I thought There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @marcoabreu Any thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TaoLv MKLDNN is well-tested on CI for Unix Python, but not for CentOS Python. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with Tao. I also don't think that CentOS should make any difference for mkldnn, so there's no need to test that path. Wdyt, @TaoLv There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mseth10 Do you know any problem with CentOS Python which is not exposed in the existing CI? If has, I would like to have another PR for adding it into CI and keep this PR focusing on MKL-DNN build change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TaoLv CentOS Python tests on CI use non MKL-DNN builds. Shouldn't we be using MKL-DNN builds now that MKL-DNN is default on CentOS? There is no other problem exposed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @mseth10 . Although I don't think there are any differences between Ubuntu+MKL-DNN and CentOS+MKL-DNN, I'm happy to see more test suits are enabled for MKL-DNN backend (if we don't mind increasing the execution time of CI). Maybe we can separate them into another PR as CI enhancement? @marcoabreu what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TaoLv Just to note, I checked and the CI execution time is not increased, as the tests run in parallel and Ubuntu test takes longer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is necessary to be honest. Mkldnn should not really interface with the operating system, but I'm not certain. Ideally Tao could consult with some Intel engineers. But to unblock the progress of this PR, we should have that discussion in a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback. For speedy progress of this PR, I will not add tests to CI as part of it. |
||
return ['Python3: CentOS 7 CPU MKLDNN': { | ||
node(NODE_LINUX_CPU) { | ||
ws('workspace/build-centos7-cpu-mkldnn') { | ||
timeout(time: max_time, unit: 'MINUTES') { | ||
try { | ||
utils.unpack_and_init('centos7_mkldnn', mx_mkldnn_lib, true) | ||
utils.docker_run('centos7_cpu', 'unittest_centos7_cpu', false) | ||
utils.publish_test_coverage() | ||
} finally { | ||
utils.collect_test_results_unix('nosetests_unittest.xml', 'nosetests_python3_centos7_cpu_mkldnn_unittest.xml') | ||
utils.collect_test_results_unix('nosetests_train.xml', 'nosetests_python3_centos7_cpu_mkldnn_train.xml') | ||
} | ||
} | ||
} | ||
} | ||
}] | ||
} | ||
|
||
def test_centos7_python3_gpu() { | ||
return ['Python3: CentOS 7 GPU': { | ||
node(NODE_LINUX_GPU) { | ||
|
@@ -1010,6 +1043,24 @@ def test_centos7_python3_gpu() { | |
}] | ||
} | ||
|
||
def test_centos7_python3_gpu_mkldnn() { | ||
return ['Python3: CentOS 7 GPU MKLDNN': { | ||
node(NODE_LINUX_GPU) { | ||
ws('workspace/build-centos7-gpu-mkldnn') { | ||
timeout(time: max_time, unit: 'MINUTES') { | ||
try { | ||
utils.unpack_and_init('centos7_gpu_mkldnn', mx_mkldnn_lib, true) | ||
utils.docker_run('centos7_gpu', 'unittest_centos7_gpu', true) | ||
utils.publish_test_coverage() | ||
} finally { | ||
utils.collect_test_results_unix('nosetests_gpu.xml', 'nosetests_python3_centos7_gpu_mkldnn.xml') | ||
} | ||
} | ||
} | ||
} | ||
}] | ||
} | ||
|
||
def test_centos7_scala_cpu() { | ||
return ['Scala: CentOS CPU': { | ||
node(NODE_LINUX_CPU) { | ||
|
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.
Actually
CMAKE_SYSTEM_PROCESSOR
will not work for cross compilation. You could reuse a variableCMAKE_CROSSCOMPILING
for this as shown here.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.
Do you suggest we check for
SYSTEM_ARCHITECTURE STREQUAL "x86_64" AND NOT CMAKE_CROSSCOMPILING
instead of
CMAKE_SYSTEM_PROCESSOR MATCHES x86_64
?Also, will
CMAKE_HOST_SYSTEM_PROCESSOR MATCHES x86_64
help in case of cross compilation?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.
Yes, I think
SYSTEM_ARCHITECTURE STREQUAL "x86_64" AND NOT CMAKE_CROSSCOMPILING
should work. But you need to add the trick forCMAKE_CROSSCOMPILING
from here as well: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.
@lebeg I checked,
CMAKE_SYSTEM_PROCESSOR
works for cross compilation.CMAKE_CROSSCOMPILING
isTRUE
only for ARM v6, v7, v8, and for all these casesCMAKE_SYSTEM_PROCESSOR
exists andCMAKE_SYSTEM_PROCESSOR MATCHES x86_64
returnsFALSE
. Hence, I don't think any change is needed. Please correct me if I am missing anything.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.
Note the semantics of
mxnet_option
. If the condition is not satisfied, this option will be turned off ,not setting the default value off.