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.

Sign up for GitHub

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

Jump to bottom

[MRG] BUILD: check OpenMP support and add a switch to build without it #13543

Merged

jnothman merged 28 commits into scikit-learn:master from jeremiedbb:build-no-openmp

Apr 16, 2019

Member

jeremiedbb commented Mar 28, 2019 •

edited

Loading

Fixes #13390

This PR adds a check for OpenMP support at the beginning of the build. If the check fails, the build crashes with an explicit error message.

It also add the possibility to build without OpenMP support by explicitly setting the environment variable SKLEARN_NO_OPENMP.

jeremiedbb changed the title ~~[MRG] BUILD: check OpenMP support and add a switch to build without it~~ [WIP] BUILD: check OpenMP support and add a switch to build without it

jeremiedbb force-pushed the build-no-openmp branch from 38d9a1e to 56f1786 Compare

March 29, 2019 09:30

Member Author

jeremiedbb commented Mar 29, 2019

I think I came up with something working. There's still one issue. The common test test_configure tests all the setup.py in sklearn. Thus it tries to check if openmp is supported. But if sklearn required specific environment variables at build time (e.g. on macos with default apple clang), it could fail because they are not set anymore.

I'm not sure how we want to deal with that.

jeremiedbb changed the title ~~[WIP] BUILD: check OpenMP support and add a switch to build without it~~ [MRG] BUILD: check OpenMP support and add a switch to build without it

rth reviewed

View reviewed changes

Member

rth left a comment

I agree that it's better if scikit-learn can still be built without OpenMP support if needed. Setting that flag via an env variable sounds reasonable.

sklearn/_build_utils/openmp_helpers.py Outdated

+                      "  and rerun the build command. Notice however that it will hurt the "
+                      "performances of some estimators, which will run in sequential mode "
+                      "and for which the `n_jobs` parameter will have no effect anymore."
+                      "\n\n                    ***\n")

Member

rth Mar 29, 2019

To improve readability better to use a multi-line string wrapped in textwrap.dedent

sklearn/_build_utils/openmp_helpers.py Outdated

+                      openmp_supported = False
+                  finally:
+                      os.chdir(start_dir)

Member

rth Mar 29, 2019 •

edited

Loading

Maybe clean up the temp folder here?

sklearn/_build_utils/openmp_helpers.py Outdated

+                      printf("nthreads=%d\\n", omp_get_num_threads());
+                      return 0;
+                      }
+                      """

Member

rth Mar 29, 2019

also maybe wrap it in textwrap.dendent to keep correct indentation.

sklearn/_build_utils/openmp_helpers.py Outdated

+                      "- Make sure you have followed the installation instructions:\n\n"
+                      "     "
+                      "https://scikit-learn.org/dev/developers/advanced_installation.html"
+                      "\n\n- If your compiler should support OpenMP but the build still "

Member

rth Mar 29, 2019

"should support" -> "supports"

sklearn/_build_utils/openmp_helpers.py Outdated

+                      "     "
+                      "https://scikit-learn.org/dev/developers/advanced_installation.html"
+                      "\n\n- If your compiler should support OpenMP but the build still "
+                      "fails, please submit a bug report here:\n\n"

Member

rth Mar 29, 2019

"bug report at"

sklearn/_build_utils/openmp_helpers.py Outdated

+                      "fails, please submit a bug report here:\n\n"
+                      "     "
+                      "https://github.com/scikit-learn/scikit-learn/issues\n\n"
+                      "- If you explicitly want to build scikit-learn without OpenMP "

Member

rth Mar 29, 2019

remove "explicitly"

sklearn/_build_utils/openmp_helpers.py Outdated

+                      "support, you can set the following environment variable:\n\n"
+                      "     SKLEARN_NO_OPENMP=TRUE\n\n"
+                      "  and rerun the build command. Notice however that it will hurt the "
+                      "performances of some estimators, which will run in sequential mode "

Member

rth Mar 29, 2019

I would remove "hurt the performance", people needing to build without OpenMP support should know what OpenMP does.

sklearn/_build_utils/openmp_helpers.py Outdated



		def get_openmp_flag(compiler):
		if os.getenv('SKLEARN_NO_OPENMP') == 'TRUE':

Member

rth Mar 29, 2019

I think, for binary env variables, their existence should interpreted as True, i.e. if os.getenv('SKLEARN_NO_OPENMP'). That removes the need to think about how to define boolean True (or "True" string) in environment variable in BASH and Windows CMD: e.g. whether it's true, "TRUE", True or maybe 'True'...

Member

NicolasHug commented Mar 29, 2019 •

edited

Loading

if sklearn required specific environment variables at build time [...] it could fail because they are not set anymore.

I think it's fair to fail if the variable isn't there anymore. It makes sense to force SKLEARN_NO_OPENMP to be always set (build time and run time) if you don't want openMP.

So we are certain that even openmp-specific code like e.g. openmp.omp_get_max_threads() can
i) compile
ii) doesn't crash at run time

even if openMP isn't installed on the machine? It's one thing to compile without the openMP flags, but it's another thing to run without openMP being installed.

Also, do we want a CI that doesn't have openMP to make sure everything works?

Member Author

jeremiedbb commented Mar 29, 2019 •

edited

Loading

I think it's fair to fail if the variable isn't there anymore. It makes sense to force SKLEARN_NO_OPENMP to be always set (build time and run time) if you don't want openMP.

It's not only related to SKLEARN_NO_OPENMP, but to CPPFLAGS, CFLAGS, LDFLAGS, ... on macos with default apple clang.

even if openMP isn't installed on the machine? It's one thing to compile without the openMP flags, but it's another thing to run without openMP being installed.

Needs to be tested indeed, but I don't see how it would be possible that the program have access to the openmp symbols if it's not linked to openmp. So my guess is that it will work.

Also, do we want a CI that doesn't have openMP to make sure everything works?

I'm +0 on that.

Member

NicolasHug commented Mar 29, 2019

I don't see how it would be possible that the program have access to the openmp symbols if it's not linked to openmp.

Sorry I'm don't I understand what you mean.

Using omp_get_max_threads in a .pyx file compiles to this .c:

#ifdef _OPENMP                                                                 
#include <omp.h>                                                               
#endif /* _OPENMP */

/* ... */

__pyx_v_n_threads = omp_get_max_threads();

How does this compile if _OPENMP is not defined?

Member Author

jeremiedbb commented Mar 29, 2019 •

edited

Loading

How does this compile if _OPENMP is not defined?

Honestly I don't know, but if _OPENMP is not defined, openmp is not included, so it shouldn't matter whether openmp exists somewhere on the system or not. That's why I think cython must do something in that case. Anyway; I agree that it needs to be tested.

Member

NicolasHug commented Mar 29, 2019

Oh, actually I just noticed that the .c file actually is:

#include <omp.h>  

/* ... */

#ifdef _OPENMP                                                                 
#include <omp.h>                                                               
#endif /* _OPENMP */

/* ... */

__pyx_v_n_threads = omp_get_max_threads();

Now I'm just really confused.

Member Author

jeremiedbb commented Mar 29, 2019

I'm testing on the macos job where I removed the installation of libomp.

Member Author

jeremiedbb commented Mar 29, 2019

fatal error: 'omp.h' file not found
#include <omp.h>
         ^~~~~~~

You were right :)
Somehow, gcc must have linked openmp without the -fopenmp flag when I tested locally.

But then there's something I don't understand. Why is this flag necessary ? is _OPENMP not defined in that case, meaning no pragmas ? I'm really confused...

Anyway, this is a problem, and I guess we can't switch to no OpenMP build mode.

Member

NicolasHug commented Mar 29, 2019 •

edited

Loading

Thanks for the test!

I think I have a vague idea of what's going on:

If your code uses from openmp cimport omp_get_max_threads, Cython is going to add

#include <omp.h>

If you only use e.g. prange which does not need from openmp..., it will protect the import with

#ifdef _OPENMP                                                                 
#include <omp.h>                                                               
#endif /* _OPENMP */

If you use both, it will add both.

So if you don't import anything with from openmp cimport ..., everything is protected with the ifdef _OPENMP. If you don't use the -openmp flag it still works fine since the only openmp code is the #pragma stuff above the for loops, e.g.:

#ifdef _OPENMP
#pragma omp for firstprivate(__pyx_v_thread_idx) lastprivate(__pyx_v_thread_idx)
#endif /* _OPENMP */
for (__pyx_t_11 = 0; __pyx_t_11 < __pyx_t_3; __pyx_t_11++){

You must have tested without using from openmp cimport something

Member

NicolasHug commented Mar 29, 2019

Anyway, this is a problem, and I guess we can't switch to no OpenMP build mode.

Agreed :/

jeremiedbb changed the title ~~[MRG] BUILD: check OpenMP support and add a switch to build without it~~ [WIP] BUILD: check OpenMP support and add a switch to build without it

NicolasHug mentioned this pull request

Build flag to force enable/disable OpenMP #13390

Closed

Member

jnothman commented Mar 30, 2019 via email

Is there any way to guard the cimport with the ifdef explicitly?

Member

NicolasHug commented Mar 30, 2019 •

edited

Loading

Is there any way to guard the cimport with the ifdef explicitly?

(we can't ifdef a cimport since one is C code and the other is Cython code. My answer below assumes you meant to protect the include <omp.h> with #ifdef)

Not without changing Cython since it's Cython that generates the .c code.

The only solution I can think of is the one I mentioned before:
#13390 (comment)

Basically, for every single function that you can cimport from openmp, we would need to write a fallback to use in case SKLEARN_NO_OPENMP is on.

That sounds like a bad idea to me

Member Author

jeremiedbb commented Apr 1, 2019 •

edited

Loading

Actually there's a compile time conditional statement in cython. The syntax is DEF and IF, see the doc.

So one possibility would be to protect every direct call to openmp, cimport openmp, openmp.omp_get_num_threads(), etc... like this:

IF SKLEARN_OPENMP:
    num_threads = openmp.omp_get_num_threads()
ELSE:
    num_threads = 1

~~and add a helper in the setup, before any cythonization, to write DEF SKLEARN_OPENMP = True(False) at the beginning of any pyx file.~~ Cleaner: cython has a compile_time_env parameter.

Do you think it's worth ?

Member

agramfort commented Apr 1, 2019 via email

If we support this we need a CI testing it My 2c

Member

NicolasHug commented Apr 1, 2019

Do you think it's worth ?

No, see #13390 (comment)

(We're having the same discussion here and on #13390, might be better to stick to the issue)

jeremiedbb added 9 commits

April 2, 2019 17:16


          add check for openmp support and switch to build without it

09ec62a

tst

f300574

tst

18f31e3

tst

35403f6

tst

7d5bb56


          cln code. add compile_time_env

315c27d

cln

d0374e8


          don't check openmp in tests

a8b3a1b


          restore

f591d19

Member

NicolasHug commented Apr 14, 2019

Yes I think it'd be clearer. Maybe SKLEARN_OPENMP_SUPPORTED to avoid potential conflicts, just in case

jeremiedbb added 5 commits

April 15, 2019 10:50


          merge master

18863c8


          merge master

9efb3d5


          sklearn_openmp -> sklearn_openmp_supported

a96035b


          doc sklearn_no_openmp

bfd79b8


          doc protecting openmp

8d7b678

Member Author

jeremiedbb commented Apr 15, 2019

I added some documentation. Let me know what you think.

jnothman reviewed

View reviewed changes

doc/developers/advanced_installation.rst Outdated

+                 It is possible to build scikit-learn without OpenMP support by setting the
+                 ``SKLEARN_NO_OPENMP`` environment variable. This is not recommanded since it
+                 will force some estimators to run in sequential mode and their ``n_jobs``
+                 parameter will be ignored.

Member

jnothman Apr 15, 2019

Note that this needs to happen before cythonizing

NicolasHug reviewed

View reviewed changes

doc/developers/advanced_installation.rst Outdated Show resolved Hide resolved

doc/developers/performance.rst Outdated

@@ @@ -68,7 +68,7 @@ following: @@
                    with the gold standard, easy to debug Python version.
 . Once the code is optimized (not simple bottleneck spottable by
-                   profiling), check whether it is possible to have **coarse grained
+                   profiling), check whether it is possible to have **coarse graineds

Member

NicolasHug Apr 15, 2019

revert?

Member Author

jeremiedbb Apr 15, 2019

how did that end up here ?

sklearn/_build_utils/__init__.py Show resolved Hide resolved

sklearn/tests/test_common.py Show resolved Hide resolved

NicolasHug and others added 4 commits

April 15, 2019 17:31


          Update doc/developers/advanced_installation.rst

ed2bee8

Co-Authored-By: jeremiedbb <34657725+jeremiedbb@users.noreply.github.com>


          revert

3c8f13f


          Merge branch 'build-no-openmp' of github.com:jeremiedbb/scikit-learn …

4ba5e74

…into build-no-openmp


          update doc

15e6236

NicolasHug reviewed

View reviewed changes

sklearn/_build_utils/check_openmp.pyx Outdated

		@@ -0,0 +1,9 @@
		# the build will fail if OpenMP support is not handled correctly.

Member

NicolasHug Apr 15, 2019

Can you please explicitly describe the cases where this file fails?

Member Author

jeremiedbb Apr 15, 2019

Actually I don't know exactly. I know one case of failure: building with a compiler which does not support openmp.

But with the added functionalities, either the build crashes before, with an explicit error message or one has set SKLEARN_NO_OPENMP and the build passes.

It's more like a unit test for the build. I'm not sure we should keep it. But there might be other reasons of failure that we don't know yet (there always are :/).

What explanation do you have in mind ?

Member

NicolasHug Apr 15, 2019

What explanation do you have in mind ?

Well, I asked because I can't think of any reason to keep it either. It seems redundant with check_openmp_support().

there might be other reasons of failure that we don't know yet (there always are :/).

Definitely agree. However, the fact that it's here makes me think there is a reason I don't understand, and it's making everything confusing ^^

I would suggest to remove it then, along with the changes to test_configure().

Member

jnothman Apr 15, 2019

The function check_openmp is never called? Yes, I think if can be removed

Member Author

jeremiedbb Apr 16, 2019

I would suggest to remove it then, along with the changes to test_configure().

I removed it. However the changes to test_configure() are unrelated to that. Without those, the test would fail on osx with default apple clang compiler. We don't want to have to set all the env variables to enable openmp on default apple clang to run the test suite. One can use sklearn on a system which could not build scikit learn.

Member

NicolasHug Apr 16, 2019

Without those, the test would fail on osx with default apple clang compiler.

Which tests would fail?

Member Author

jeremiedbb Apr 16, 2019

test_configure, the one we are talking about :)

Member

NicolasHug Apr 16, 2019

So if I understand it correctly, the only use-case for this are mac users that:

want to run the tests
don't need to build scikit-learn (else they would have set the env variables already and the checks would work)

?

Member Author

jeremiedbb Apr 16, 2019

not exactly because they would have set the env variable when building but it won't be set anymore when testing

Member

NicolasHug Apr 16, 2019

I see, thanks

jnothman added the Blocker label


          remove openmp cython check

717952a

NicolasHug reviewed

View reviewed changes

sklearn/tests/test_common.py Outdated

+                      # OpenMP checks are build checks done before cythonization to allow
+                      # fast fail. Since sklearn can be built and tested on 2 different
+                      # machines, we disable it for a configure check.

Member

NicolasHug Apr 16, 2019 •

edited

Loading

Since this addresses of very specific use-case, I think we need to be very clear on the explanation here. This has been confusing me for a while (I might still be missing something but feel free to correct me). Here's my attempt:

This test will run every setup.py and eventually call check_openmp_support(), which tries to compile a C file that uses openmp, unless SKLEARN_NO_OPENMP is set. Some users might want to run the tests without having build-support for openmp. In particular, mac users need to set some env variables to build with openmp support, and these might not be set anymore at test time. We thus temporarily set SKLEARN_NO_OPENMP, so that this test runs smoothly.


          update test comment

b6efd63

NicolasHug reviewed

View reviewed changes

sklearn/tests/test_common.py

+                      # users need to set some environment variables to build with openmp
+                      # support, and these might not be set anymore at test time. We thus
+                      # temporarily set SKLEARN_NO_OPENMP, so that this test runs smoothly.
+                      old_env = os.getenv('SKLEARN_NO_OPENMP')

Member

NicolasHug Apr 16, 2019

call this old_sklearn_no_openmp?

NicolasHug reviewed

View reviewed changes

Member

NicolasHug left a comment

Nitpick but LGTM

Thanks a lot @jeremiedbb

NicolasHug approved these changes

View reviewed changes

Member

jnothman commented Apr 16, 2019

Let's just do this and see how it goes

jnothman merged commit 0fb7cc7 into scikit-learn:master

Member

jnothman commented Apr 16, 2019

Thanks to you both!

jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request


          BLD: check OpenMP support and add a switch to build without it (sciki…

b957edd

…t-learn#13543)

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request


          BLD: check OpenMP support and add a switch to build without it (sciki…

bc1e414

…t-learn#13543)

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request


          Revert "BLD: check OpenMP support and add a switch to build without it (

4430d72

scikit-learn#13543)"

This reverts commit bc1e414.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request


          Revert "BLD: check OpenMP support and add a switch to build without it (

1dc1806

scikit-learn#13543)"

This reverts commit bc1e414.

jorisvandenbossche mentioned this pull request

Add parallel processing where it makes sense pyproj4/pyproj#364

Closed

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request


          BLD: check OpenMP support and add a switch to build without it (sciki…

987a5f0

…t-learn#13543)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment