You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2021/02/03 21:58:58 UTC
[GitHub] [incubator-mxnet] akarbown opened a new pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
akarbown opened a new pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832
## Description ##
Fix building when USE_BLAS option is not provided in the compilation command line.
## Comments ##
Overlooked in #19766 .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570260651
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Yes, it'll fail. I need to rethink it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570097578
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
It's set to 'Open' in the l.19 ('set(BLAS "Open" CACHE STRING "Selected BLAS library")'). Or I miss sth?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-775767176
@mxnet-bot run ci [unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570317383
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
I think that now it's difficult to recreate the previous logic and what's more the USE_MKL_IF_AVAILABLE variable seems to be unnecessary. What do you think about removing the USE_MKL_IF_AVAILABLE for the whole project and as a consequence of that rely only on the USE_BLAS command line option?
Thus, when USE_BLAS will be set to 'mkl' and there will be no MKL libraries - the build should fail.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570366101
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
> Line 18 above initializes set(BLAS "Open") and only if MKL can be found, it's overwritten to BLAS=MKL.
> Do you agree? If so, I think the current PR isn't needed.
>
If there is no 'MKL' the following function: find_package(MKL) is looking for the MKL through FindMKL.cmake file and produce the following warning:
```
CMake Warning at cmake/ChooseBlas.cmake:26 (find_package):
By not providing "FindMKL.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "MKL", but
CMake did not find one.
Could not find a package configuration file provided by "MKL" with any of
the following names:
MKLConfig.cmake
mkl-config.cmake
Add the installation prefix of "MKL" to CMAKE_PREFIX_PATH or set "MKL_DIR"
to a directory containing one of the above files. If "MKL" provides a
separate development package or SDK, be sure it has been installed.
Call Stack (most recent call first):
CMakeLists.txt:312 (include)
```
I wanted to clear it out with that PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19832: [BUGFIX] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-779487367
Jenkins CI successfully triggered : [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570359131
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Line 18 above initializes set(BLAS "Open") and only if MKL can be found, it's overwritten to BLAS=MKL.
Do you agree? If so, I think the current PR isn't needed.
With respect to USE_MKL_IF_AVAILABLE, it's fine with me to remove the variable and just check for MKL by default. As long as user doesn't specify which BLAS library they want, I think it's reasonable for MXNet to check for BLAS libraries in MXNet specified order of priority (ie. 1st prio is MKL, if not available try OpenBlas and otherwise use whatever is available..)
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Thanks for clarifying. One option is to move the code from `elseif(BLAS STREQUAL "MKL" OR BLAS STREQUAL "mkl")` case up here to the beginning of the file and return from file immediately if MKL is found, or proceed to the other options if MKL isn't found. Another option as you mention is to remove USE_MKL_IF_AVAILABLE. Either is fine with me
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570097578
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
It's set to 'Open' in the l.19 ('set(BLAS "Open" CACHE STRING "Selected BLAS library")'). Or I miss sth?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r569826183
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Wouldn't this fail if MKL is not available?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-777291016
@mxnet-bot run ci [unix-cpu, windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570366101
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
> Line 18 above initializes set(BLAS "Open") and only if MKL can be found, it's overwritten to BLAS=MKL.
> Do you agree? If so, I think the current PR isn't needed.
>
If there is no 'MKL' the following function: find_package(MKL) is looking for the MKL through of FindMKL.cmake file and produce the following warning:
```
CMake Warning at cmake/ChooseBlas.cmake:26 (find_package):
By not providing "FindMKL.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "MKL", but
CMake did not find one.
Could not find a package configuration file provided by "MKL" with any of
the following names:
MKLConfig.cmake
mkl-config.cmake
Add the installation prefix of "MKL" to CMAKE_PREFIX_PATH or set "MKL_DIR"
to a directory containing one of the above files. If "MKL" provides a
separate development package or SDK, be sure it has been installed.
Call Stack (most recent call first):
CMakeLists.txt:312 (include)
```
I wanted to clear it out with that PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570329400
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Or maybe I missed something?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu merged pull request #19832: [BUGFIX] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #19832: [BUGFIX] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-779487343
@mxnet-bot run ci [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-775945066
Jenkins CI successfully triggered : [unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570097578
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
It's set to 'Open' in the l.19 ('set(BLAS "Open" CACHE STRING "Selected BLAS library")'). Or I miss sth?
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
It's set to 'Open' in the l.19 ('set(BLAS "Open" CACHE STRING "Selected BLAS library")'). Or I miss sth?
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Yes, it'll fail. I need to rethink it.
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
I think that now it's difficult to recreate a previous logic and what's more the USE_MKL_IF_AVAILABLE variable seems to be unnecessary. What do you think about removing the USE_MKL_IF_AVAILABLE for the whole project and as a consequence of that rely only on the USE_BLAS command line option?
Thus, when USE_BLAS will be set to 'mkl' and there will be no MKL libraries - the build should fail.
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
I think that now it's difficult to recreate the previous logic and what's more the USE_MKL_IF_AVAILABLE variable seems to be unnecessary. What do you think about removing the USE_MKL_IF_AVAILABLE for the whole project and as a consequence of that rely only on the USE_BLAS command line option?
Thus, when USE_BLAS will be set to 'mkl' and there will be no MKL libraries - the build should fail.
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Or maybe I missed something?
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
> Line 18 above initializes set(BLAS "Open") and only if MKL can be found, it's overwritten to BLAS=MKL.
> Do you agree? If so, I think the current PR isn't needed.
>
If there is no 'MKL' the following function: find_package(MKL) is looking for the MKL through of FindMKL.cmake file and produce the following warning:
```
CMake Warning at cmake/ChooseBlas.cmake:26 (find_package):
By not providing "FindMKL.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "MKL", but
CMake did not find one.
Could not find a package configuration file provided by "MKL" with any of
the following names:
MKLConfig.cmake
mkl-config.cmake
Add the installation prefix of "MKL" to CMAKE_PREFIX_PATH or set "MKL_DIR"
to a directory containing one of the above files. If "MKL" provides a
separate development package or SDK, be sure it has been installed.
Call Stack (most recent call first):
CMakeLists.txt:312 (include)
```
I wanted to clear it out with that PR.
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
> Line 18 above initializes set(BLAS "Open") and only if MKL can be found, it's overwritten to BLAS=MKL.
> Do you agree? If so, I think the current PR isn't needed.
>
If there is no 'MKL' the following function: find_package(MKL) is looking for the MKL through FindMKL.cmake file and produce the following warning:
```
CMake Warning at cmake/ChooseBlas.cmake:26 (find_package):
By not providing "FindMKL.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "MKL", but
CMake did not find one.
Could not find a package configuration file provided by "MKL" with any of
the following names:
MKLConfig.cmake
mkl-config.cmake
Add the installation prefix of "MKL" to CMAKE_PREFIX_PATH or set "MKL_DIR"
to a directory containing one of the above files. If "MKL" provides a
separate development package or SDK, be sure it has been installed.
Call Stack (most recent call first):
CMakeLists.txt:312 (include)
```
I wanted to clear it out with that PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-772852319
Hey @akarbown , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [unix-gpu, centos-gpu, edge, clang, windows-gpu, miscellaneous, unix-cpu, website, sanity, centos-cpu, windows-cpu]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-777291057
Jenkins CI successfully triggered : [unix-cpu, windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-775944968
@mxnet-bot run ci [unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570359131
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Line 18 above initializes set(BLAS "Open") and only if MKL can be found, it's overwritten to BLAS=MKL.
Do you agree? If so, I think the current PR isn't needed.
With respect to USE_MKL_IF_AVAILABLE, it's fine with me to remove the variable and just check for MKL by default. As long as user doesn't specify which BLAS library they want, I think it's reasonable for MXNet to check for BLAS libraries in MXNet specified order of priority (ie. 1st prio is MKL, if not available try OpenBlas and otherwise use whatever is available..)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#issuecomment-775767236
Jenkins CI successfully triggered : [unix-cpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] akarbown commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
akarbown commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570317383
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
I think that now it's difficult to recreate a previous logic and what's more the USE_MKL_IF_AVAILABLE variable seems to be unnecessary. What do you think about removing the USE_MKL_IF_AVAILABLE for the whole project and as a consequence of that rely only on the USE_BLAS command line option?
Thus, when USE_BLAS will be set to 'mkl' and there will be no MKL libraries - the build should fail.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #19832: [WIP] Fix building when USE_BLAS option is not provided
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19832:
URL: https://github.com/apache/incubator-mxnet/pull/19832#discussion_r570375523
##########
File path: cmake/ChooseBlas.cmake
##########
@@ -22,12 +22,7 @@ if(DEFINED USE_BLAS)
set(BLAS "${USE_BLAS}")
else()
if(USE_MKL_IF_AVAILABLE)
- if(NOT MKL_FOUND)
- find_package(MKL)
- endif()
- if(MKL_FOUND)
- set(BLAS "MKL")
- endif()
+ set(BLAS "MKL")
Review comment:
Thanks for clarifying. One option is to move the code from `elseif(BLAS STREQUAL "MKL" OR BLAS STREQUAL "mkl")` case up here to the beginning of the file and return from file immediately if MKL is found, or proceed to the other options if MKL isn't found. Another option as you mention is to remove USE_MKL_IF_AVAILABLE. Either is fine with me
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org