Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Update setup and travis to support OpenMP #13053

Merged
merged 10 commits into from Feb 4, 2019

Conversation

Projects
None yet
7 participants
@jeremiedbb
Copy link
Contributor

commented Jan 27, 2019

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.

@glemaitre

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

In scikit-image, they run the compiler with the given flag:
https://github.com/scikit-image/scikit-image/blob/master/setup.py#L69
I don't know if it could be a good idea to do the same.

By using OpenMP, we will need to ensure which minimum version we need.

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

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 ?

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

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.

@glemaitre

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

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.

I agree that your approach is better. I was actually only speaking about compiling a sample and remove the flag if it fails.

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 ?

This is actually one of my question. I don't know what is best.

@glemaitre

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

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 even on the main ppa.

I was meaning to only provide the minimum version in the documentation.

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

I guess OpenMP 3.0 is safe. Previous specs are considered legacy on the openmp specs page

@NicolasHug

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Just wanted to thank you @jeremiedbb because it saved me so much time for #12807

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

Glad it's useful for someone else cause it cost me so much time :D

@jnothman

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

Glad it's useful for someone else cause it cost me so much time :D

Blog it somewhere?

@glemaitre

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

Blog it somewhere?

Indeed, it might be a great thing. I can offer a hand on my free time for that.

@ogrisel
Copy link
Member

left a comment

Small suggestion. Let me know what you think.

Show resolved Hide resolved setup.py
@amueller

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

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.
Ideally we get people to try the wheels a bunch before we attempt a release.

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

Ideally we get people to try the wheels a bunch before we attempt a release.

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 :)

jeremiedbb added some commits Jan 29, 2019

Show resolved Hide resolved build_tools/travis/install.sh Outdated

jeremiedbb added some commits Jan 31, 2019


Then you need to set the following environment variables::

export CC=clang

This comment has been minimized.

Copy link
@thomasjpfan

thomasjpfan Jan 31, 2019

Member

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.

This comment has been minimized.

Copy link
@ogrisel

ogrisel Feb 7, 2019

Member

@jeremiedbb this comment was not addressed prior to merging the PR.

This comment has been minimized.

Copy link
@jeremiedbb

jeremiedbb Feb 7, 2019

Author Contributor

Yes I thought it was for the travis build, and not for the install doc, but it's been added in #13093

@ogrisel

ogrisel approved these changes Feb 1, 2019

Copy link
Member

left a comment

LGTM.

@ogrisel ogrisel added the Needs Review label Feb 1, 2019

Show resolved Hide resolved setup.py Outdated
@jnothman
Copy link
Member

left a comment

Let's give it a whirl?

@jnothman jnothman merged commit 7307b56 into scikit-learn:master Feb 4, 2019

11 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No alert changes
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 3df0c19...5411a46
Details
codecov/project 92.42% (-0.01%) compared to 3df0c19
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 6, 2019

thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 7, 2019

MLopez-Ibanez pushed a commit to MLopez-Ibanez/scikit-learn that referenced this pull request Feb 9, 2019

MLopez-Ibanez added a commit to MLopez-Ibanez/scikit-learn that referenced this pull request Feb 9, 2019

xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019

xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019

xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.