[MRG] BUILD: warn instead of raise when openmp unsupported #15174

jeremiedbb · 2019-10-10T14:15:20Z

Changed the behavior to warn during the build that OpenMP is not supported, continuing the build without OpenMP. It should ease the sprints a little bit :)

(Edited)
after some discussions the behavior is now the following:

check that a test program not involving OpenMP can be compiled. Allows fast fail (cythonizing takes time) and allows to distinguish from openmp related build errors (see Building scikit-learn with gcc 9.2 in a conda env fails #15129)
then check that a test program involving OpenMP can be compiled.
- if OpenMP is supported, build continues with OpenMP features enabled
- otherwise, a warning is raised and the build continues with OpenMP features disabled (and no warning in the init at runtime)
a new test is added which asserts that sklearn has been built with OpenMP enabled. It can be skipped with an env var.
a new entry in show version is added: built with OpenMP: True/False

This PR also add a new CI job to test sklearn build without openmp on macOS.

…ing openmp error message when irrelevant

jeremiedbb · 2019-10-10T14:45:14Z

based on the discussion in #14624 @rth @ogrisel

ogrisel

First pass:

sklearn/__init__.py

sklearn/_build_utils/__init__.py

jeremiedbb · 2019-10-10T15:09:48Z

@thomasjpfan or @NicolasHug could you test on your mac that the warning is printed as expected when you build on an environment which does not support OpenMP (without setting SKLEARN_NO_OPENMP).

jeremiedbb · 2019-10-10T16:07:17Z

The failing coverage is because we don't test that the warning is printed. It would require a special CI job on macOS with no OpenMP support. Not sure it's worth it.

NicolasHug · 2019-10-10T16:34:39Z

@NicolasHug could you test on your mac

I'm triggered

(kidding, I'm on linux though :p)

jeremiedbb · 2019-10-10T17:03:44Z

I had this image in my head that the US team was on the macos side of the force. I hope you'll find the strength to forgive me :)

thomasjpfan · 2019-10-10T17:09:52Z

Without setting SKLEARN_NO_OPENMP I get:

It seems that scikit-learn cannot be built with OpenMP support.

- Make sure you have followed the installation instructions:

    https://scikit-learn.org/dev/developers/advanced_installation.html

- If your compiler supports OpenMP but the build still fails, please
  submit a bug report at:

    https://github.com/scikit-learn/scikit-learn/issues

- If you want to build scikit-learn without OpenMP support, you can set
  the environment variable SKLEARN_NO_OPENMP and rerun the build
  command. Note however that some estimators will run in sequential
  mode and their `n_jobs` parameter will have no effect anymore.

                    ***

  warnings.warn(message)
/Users/thomasfan/anaconda3/envs/sk-master-2/lib/python3.7/site-packages/Cython/Distutils/old_build_ext.py:41: UserWarning: Cython.Distutils.old_build_ext does not properly handle dependencies and is deprecated.
  "Cython.Distutils.old_build_ext does not properly handle dependencies "
Traceback (most recent call last):
  File "setup.py", line 290, in <module>
    setup_package()
  File "setup.py", line 286, in setup_package
    setup(**metadata)
  File "/Users/thomasfan/anaconda3/envs/sk-master-2/lib/python3.7/site-packages/numpy/distutils/core.py", line 137, in setup
    config = configuration()
  File "setup.py", line 174, in configuration
    config.add_subpackage('sklearn')
  File "/Users/thomasfan/anaconda3/envs/sk-master-2/lib/python3.7/site-packages/numpy/distutils/misc_util.py", line 1036, in add_subpackage
    caller_level = 2)
  File "/Users/thomasfan/anaconda3/envs/sk-master-2/lib/python3.7/site-packages/numpy/distutils/misc_util.py", line 1005, in get_subpackage
    caller_level = caller_level + 1)
  File "/Users/thomasfan/anaconda3/envs/sk-master-2/lib/python3.7/site-packages/numpy/distutils/misc_util.py", line 942, in _get_configuration_from_setup_py
    config = setup_module.configuration(*args)
  File "sklearn/setup.py", line 81, in configuration
    )
  File "/Users/thomasfan/Documents/scikit-learn-2/sklearn/_build_utils/__init__.py", line 92, in maybe_cythonize_extensions
    compiler_directives={'language_level': 3})
  File "/Users/thomasfan/anaconda3/envs/sk-master-2/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 966, in cythonize
    aliases=aliases)
  File "/Users/thomasfan/anaconda3/envs/sk-master-2/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 810, in create_extension_list
    for file in nonempty(sorted(extended_iglob(filepattern)), "'%s' doesn't match any files" % filepattern):
  File "/Users/thomasfan/anaconda3/envs/sk-master-2/lib/python3.7/site-packages/Cython/Build/Dependencies.py", line 109, in nonempty
    raise ValueError(error_msg)
ValueError: 'sklearn/__check_build/_check_build.pyx' doesn't match any files

jeremiedbb · 2019-10-10T20:03:52Z

Sorry Thomas my request wasn't precise enough. I added a warning in the init of sklearn when sklearn has been built without openmp unintentionally. I meant could you test if this warning is printed ? And if you have time, if it's not printed when you build with SKLEARN_NO_OPENMP ?
Thanks a lot.

sklearn/_build_utils/openmp_helpers.py

jnothman

This looks pretty good from the code side. I'm not sure the logic would be easy for a reader to understand readily, but I think it's okay. I've not tested

sklearn/__init__.py

sklearn/_build_utils/openmp_helpers.py

thomasjpfan · 2019-10-10T23:05:10Z

@jeremiedbb To clarify, is this PR suppose to build when SKLEARN_NO_OPENMP is unset and openmp is not available?

jnothman · 2019-10-11T00:14:28Z

is this PR suppose to build when SKLEARN_NO_OPENMP is unset and openmp is

not available AFAIK yes, that's the primary point: make it easy to set up a development environment.

thomasjpfan · 2019-10-11T00:56:57Z

I am not able to build with SKLEARN_NO_OPENMP unset and openmp not configured properly with the error here: #15174 (comment)

NicolasHug

Looks good, thanks @jeremiedbb . Do we still need the new CI?

NicolasHug · 2019-11-05T13:49:46Z

sklearn/utils/tests/test_openmp_helpers.py

+
+    msg = ("This test fails because scikit-learn has been built without OpenMP"
+           " support. You can skip this test by setting the environment "
+           "variable SKLEARN_SKIP_OPENMP_TEST")


This is however not recommended since...

NicolasHug · 2019-11-05T13:50:01Z

sklearn/utils/tests/test_openmp_helpers.py

+from sklearn.utils._openmp_helpers import _openmp_supported
+
+
+def test_openmp_supported():


Maybe this should be in sklearn/tests directly?

I thought so at first but didn't found where. What do you have in mind ? in test_common ? in a new file ?

New file is good for me

NicolasHug · 2019-11-05T13:51:36Z

sklearn/utils/tests/test_openmp_helpers.py

+    # Check that sklearn is built with OpenMP supported.
+    # This test can be disabled by setting the environment variable
+    # ``SKLEARN_SKIP_OPENMP_TEST``.
+    if os.getenv("SKLEARN_SKIP_OPENMP_TEST"):


Should this just be the old SKLEARN_NO_OPENMP variable? Maybe we can just re-use it

besides the fact that I find it more explicit, I think it might add some confusion since SKLEARN_NO_OPENMP existed at some point but was used at build time whereas this one if used at runtime.

jeremiedbb · 2019-11-06T10:46:22Z

Do we still need the new CI?

I think it's useful see this comment

jeremiedbb · 2019-11-06T10:53:56Z

sklearn/utils/_show_versions.py

 def show_versions():
-    "Print useful debugging information"
+    """Print useful debugging information"""

    sys_info = _get_sys_info()
    deps_info = _get_deps_info()

-    print('\nSystem:')
+    print('\nSystem')
+    print('------')
    for k, stat in sys_info.items():
        print("{k:>10}: {stat}".format(k=k, stat=stat))

-    print('\nPython deps:')
+    print('\nPython deps')
+    print('-----------')
    for k, stat in deps_info.items():
        print("{k:>10}: {stat}".format(k=k, stat=stat))
+
+    print("\n{k:>10}: {stat}".format(k="Built with OpenMP",
+                                     stat=_openmp_supported()))


as discussed with @ogrisel irl, I added an entry in show_versions to show OpenMP status.
It will displayed as Built with OpenMP: True/False.
Later it could be improved to display the OpenMP runtime linked (requires threadpoolctl)

I also allowed myself a little cosmetic change in this function. replaced

Python deps: pip: 18.1 setuptools: 41.4.0 sklearn: 0.22.dev0 ...

by

Python deps ----------- pip: 18.1 setuptools: 41.4.0 sklearn: 0.22.dev0 ...

which I find nicer.

Python deps => Python dependencies

NicolasHug · 2019-11-06T14:48:08Z

I think it's useful see this comment

But wasn't this tested already since we allowed the SKLEARN_NO_OPENMP variable?

jeremiedbb · 2019-11-06T14:51:11Z

there's no SKLEARN_NO_OPENMP with this PR

sklearn/_build_utils/__init__.py

sklearn/utils/_show_versions.py

NicolasHug · 2019-11-06T14:56:18Z

There is in master

jeremiedbb · 2019-11-06T14:58:27Z

yes but this PR removes it
(in master the build fails if no openmp and this var makes it succeed. Since we decided to make the build always succeed this var is not necessary anymore)

NicolasHug · 2019-11-06T15:13:10Z

I understand why we don't need the var, that's not my point.

My question is about the new CI. You claim it's still necessary because:

besides testing the warning, we also want to make sure that each piece of sklearn, which would normally use openmp, works correctly when not using openmp.

we'll agree the first point isn't relevant anymore since we chose not to warn in init now
Regarding "we also want to make sure that each piece of sklearn, which would normally use openmp, works correctly when not using openmp": I might be missing something but shouldn't this already be tested in master? Since master currently allows to build without openmp.

jeremiedbb · 2019-11-06T15:21:51Z

agreed
currently we indeed test in master that tests pass without openmp. But this PR removes SKLEARN_NO_OPENMP so the 2 current jobs in master using it won't use it anymore and will build sklearn with openmp. So we need a new job to test without openmp otherwise we won't test it at all.

NicolasHug · 2019-11-06T15:27:37Z

Wny don't we just repurpose those 2 jobs??

jeremiedbb · 2019-11-06T15:33:38Z

Both are linux jobs (note that they were not made to test no openmp, they existed before). On linux we can't build without openmp now. So we need a macOS job for that.

NicolasHug · 2019-11-06T15:47:47Z

Thanks, that's the part I was missing

NicolasHug

LGTM, thanks @jeremiedbb

(I think @ogrisel's approval is about the previous version so this would still need another +1)

NicolasHug · 2019-11-06T15:49:39Z

sklearn/tests/test_build.py

+    if os.getenv("SKLEARN_SKIP_OPENMP_TEST"):
+        pytest.skip("test explicitly disabled (SKLEARN_SKIP_OPENMP_TEST)")


So concretely we have a whole new CI job only for these 2 lines.
Kinda sad but OK.

not really because we want to test that all functions pass the tests when building without openmp (hgbt, sparse_manhattan, t-sne, ...)

ogrisel

I am overall +1 with this PR but I find the phrasing "openmp support" very confusing. scikit-learn does not "support OpenMP". It's the compilers that support OpenMP or not based on their configuration.

I would rather rephrase variables such as SKLEARN_OPENMP_SUPPORTED to SKLEARN_OPENMP_PARALLELISM_ENABLED and so on.

Here are a few nitpicks (and others along those lines):

sklearn/_build_utils/openmp_helpers.py

sklearn/tests/test_build.py

sklearn/utils/_openmp_helpers.pyx

sklearn/_build_utils/__init__.py

jeremiedbb · 2019-11-12T17:35:13Z

I would rather rephrase variables such as SKLEARN_OPENMP_SUPPORTED to SKLEARN_OPENMP_PARALLELISM_ENABLED and so on.

Done.

ogrisel · 2019-11-13T09:03:27Z

Merged! Thanks @jeremiedbb!

…arn#15174)

jeremiedbb added 9 commits October 10, 2019 14:18

first compile a first test programm not using openmp to avoid display…

8e65e19

…ing openmp error message when irrelevant

cln

f37ca84

debug

28c512e

debug

e1d50a7

fix

8842f53

cln

a950dad

change raise to warn when open unsupported + add a runtime warning

9b67fa6

cln

71ce4bc

fix var name

099d8b9

jeremiedbb added the Build / CI label Oct 10, 2019

fix var name

4c07fee

jeremiedbb mentioned this pull request Oct 10, 2019

Scikit-learn can not be built with OpenMP support #14624

Closed

jeremiedbb changed the title ~~[WIP] BUILD: warn instead of raise when openmp unsupported~~ [MRG] BUILD: warn instead of raise when openmp unsupported Oct 10, 2019

jeremiedbb added the Waiting for Reviewer label Oct 10, 2019

ogrisel reviewed Oct 10, 2019

View reviewed changes

sklearn/__init__.py Outdated Show resolved Hide resolved

sklearn/__init__.py Outdated Show resolved Hide resolved

sklearn/__init__.py Outdated Show resolved Hide resolved

sklearn/_build_utils/__init__.py Outdated Show resolved Hide resolved

jeremiedbb added 2 commits October 10, 2019 17:19

adress comments

96ca9ae

don't track coverage on setups

1b8dbcf

merge master

d0d0375

thomasjpfan reviewed Oct 10, 2019

View reviewed changes

sklearn/_build_utils/openmp_helpers.py Outdated Show resolved Hide resolved

jnothman reviewed Oct 10, 2019

View reviewed changes

sklearn/__init__.py Outdated Show resolved Hide resolved

sklearn/_build_utils/openmp_helpers.py Outdated Show resolved Hide resolved

NicolasHug reviewed Nov 5, 2019

View reviewed changes

jeremiedbb added 3 commits November 6, 2019 11:24

add openmp info in show version

2250b56

improve test error message

2da7585

cln

2e6392c

jeremiedbb commented Nov 6, 2019

View reviewed changes

cln

3d50dbb

NicolasHug reviewed Nov 6, 2019

View reviewed changes

sklearn/_build_utils/__init__.py Outdated Show resolved Hide resolved

sklearn/utils/_show_versions.py Outdated Show resolved Hide resolved

jeremiedbb added 2 commits November 6, 2019 15:59

revert show version titles

f1fa1ef

update comment

c53404b

NicolasHug approved these changes Nov 6, 2019

View reviewed changes

jeremiedbb added this to the 0.22 milestone Nov 12, 2019

merge master

a179b7e

ogrisel approved these changes Nov 12, 2019

View reviewed changes

reword openmp support -> enabled

93ef87c

oops

d09db8a

ogrisel merged commit 9876f74 into scikit-learn:master Nov 13, 2019

panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020

[MRG] BUILD: warn instead of raise when openmp unsupported (scikit-le…

247e3e6

…arn#15174)

thomasjpfan mentioned this pull request Oct 25, 2022

CI "no OpenMP" build environment actually has OpenMP #24694

Closed

		from sklearn.utils._openmp_helpers import _openmp_supported


		def test_openmp_supported():

		if os.getenv("SKLEARN_SKIP_OPENMP_TEST"):
		pytest.skip("test explicitly disabled (SKLEARN_SKIP_OPENMP_TEST)")

[MRG] BUILD: warn instead of raise when openmp unsupported #15174

[MRG] BUILD: warn instead of raise when openmp unsupported #15174

Conversation

jeremiedbb commented Oct 10, 2019 • edited Loading

jeremiedbb commented Oct 10, 2019

ogrisel left a comment

Choose a reason for hiding this comment

jeremiedbb commented Oct 10, 2019

jeremiedbb commented Oct 10, 2019

NicolasHug commented Oct 10, 2019

jeremiedbb commented Oct 10, 2019

thomasjpfan commented Oct 10, 2019

jeremiedbb commented Oct 10, 2019

jnothman left a comment

Choose a reason for hiding this comment

thomasjpfan commented Oct 10, 2019

jnothman commented Oct 11, 2019 via email

thomasjpfan commented Oct 11, 2019

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremiedbb commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug commented Nov 6, 2019

jeremiedbb commented Nov 6, 2019

NicolasHug commented Nov 6, 2019

jeremiedbb commented Nov 6, 2019 • edited Loading

NicolasHug commented Nov 6, 2019

jeremiedbb commented Nov 6, 2019

NicolasHug commented Nov 6, 2019

jeremiedbb commented Nov 6, 2019

NicolasHug commented Nov 6, 2019

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremiedbb Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

ogrisel left a comment • edited Loading

Choose a reason for hiding this comment

jeremiedbb commented Nov 12, 2019

ogrisel commented Nov 13, 2019

jeremiedbb commented Oct 10, 2019 •

edited

Loading

jeremiedbb commented Nov 6, 2019 •

edited

Loading

jeremiedbb Nov 6, 2019 •

edited

Loading

ogrisel left a comment •

edited

Loading