[MRG] Update setup and travis to support OpenMP #13053
Conversation
|
In scikit-image, they run the compiler with the given flag: By using OpenMP, we will need to ensure which minimum version we need. |
|
They only have the '-fopenmp' flag, which will only work for gcc and clang. Building with icc or msvc requires different flags. Actually this flag will be silently ignored with these compilers, hence you think you link against openmp but then the program isn't multi-threaded. I tried to cover a wide range of platforms and compilers. Another thing they do is checking that openmp can be linked by trying to compile a bit of code which includes openmp. If it can't, then the library is built not multi-threaded. Do we want that ? or do we prefer the build to fail if it can't be multi-threaded with openmp ? |
|
There's no straightforward way to check the openmp version. It works at least from OpenMP 3.0 which is the implementation shipped with gcc-4.4. Older gcc are not on the main ppa. |
I agree that your approach is better. I was actually only speaking about compiling a sample and remove the flag if it fails.
This is actually one of my question. I don't know what is best. |
I was meaning to only provide the minimum version in the documentation. |
|
I guess OpenMP 3.0 is safe. Previous specs are considered legacy on the openmp specs page |
|
Just wanted to thank you @jeremiedbb because it saved me so much time for #12807 |
|
Glad it's useful for someone else cause it cost me so much time :D |
Blog it somewhere? |
Indeed, it might be a great thing. I can offer a hand on my free time for that. |
|
Btw I think in this context providing nightly wheels would be great: #9485 I mentioned that to @thomasjpfan and @NicolasHug, not sure if either has time to work on it. |
Actually I made a PR on MacPython/scikit-learn-wheels to test building the wheels with openmp support and tested it on my fork on my kmeans PR and all CIs were green. So it's encouraging :) |
|
|
||
| Then you need to set the following environment variables:: | ||
|
|
||
| export CC=clang |
There was a problem hiding this comment.
Should we specify clang explicitly?
export CC=/usr/bin/clang
export CXX=/usr/bin/clang++This is to make sure the system clang is used and not clang from something like anaconda.
There was a problem hiding this comment.
@jeremiedbb this comment was not addressed prior to merging the PR.
There was a problem hiding this comment.
Yes I thought it was for the travis build, and not for the install doc, but it's been added in #13093
Two PRs introduce the use of OpenMP is sklearn: #11950 and #12807.
Using OpenMP requires special compile flags depending on platform and compiler. This PR update the setup to add the correct flag by introspecting the compiler used at build time. There's also an update of travis for osx job because the default apple clang compiler does not support OpenMP.