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

[WIP] Add helpers for BLAS and OpenMP libs #13297

Open
wants to merge 26 commits into
base: master
from

Conversation

5 participants
@jeremiedbb
Copy link
Contributor

commented Feb 27, 2019

Currently being developed in loky. Will be move here once done.

This PR adds a new submodule _clibs which provides helpers for the OpenMP and BLAS libs. It's main purpose is to avoid oversubscription in nested parallelism, e.g. BLAS calls inside an OpenMP prange.
It's adapted from code of @tomMoral from Loky.

It also defines an effective_n_jobs_openmp which differs from joblib's effective_n_jobs when n_jobs is None in which case it's set to omp_get_max_threads, as discussed irl with @ogrisel

@agramfort agramfort added this to In progress in Sprint Paris 2019 Feb 27, 2019

@tomMoral
Copy link
Contributor

left a comment

Some comments.

It would be nice to add a test that check openmp actually take this into account.
You can use something like proposed in tomMoral/loky#135 with opemmp.omp_get_num_threads.

Show resolved Hide resolved sklearn/utils/_clibs_helpers.pyx Outdated
Show resolved Hide resolved sklearn/utils/_clibs.py Outdated
@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

requires #13294 for the CI to pass

jeremiedbb added some commits Feb 27, 2019

@ogrisel ogrisel moved this from In progress to Needs review in Sprint Paris 2019 Feb 27, 2019

Show resolved Hide resolved sklearn/utils/_clibs.py Outdated
Show resolved Hide resolved sklearn/utils/_clibs_helpers.pyx Outdated
@ogrisel
Copy link
Member

left a comment

Here is a first batch of comments:

Show resolved Hide resolved sklearn/utils/_clibs_helpers.pyx Outdated
Show resolved Hide resolved sklearn/utils/_clibs_helpers.pyx Outdated
Show resolved Hide resolved sklearn/utils/_clibs_helpers.pyx Outdated
Show resolved Hide resolved sklearn/utils/_clibs_helpers.pyx Outdated
Show resolved Hide resolved sklearn/utils/_clibs_helpers.pyx Outdated
Show resolved Hide resolved sklearn/utils/_clibs.py Outdated

tomMoral and others added some commits Feb 28, 2019

@ogrisel

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

The coverage report seems to indicate that we don't get the clib tests run on windows:

https://codecov.io/gh/scikit-learn/scikit-learn/compare/ffd27c8594fa58b8b0559ae0cf461116bf09e381...7bdea1b3b6fd960622c096e355c71c50b64b74bd/diff

Checking the azure pipelines output seems to indicate that the codecov upload step was skipped:

https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=501

Let me try to enable it in: #13332.

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Yes I added coverage to appveyor but we dropped appveyor :/

jeremiedbb added some commits Feb 28, 2019

tomMoral and others added some commits Mar 1, 2019

Show resolved Hide resolved sklearn/utils/setup.py Outdated
@ogrisel

ogrisel approved these changes Mar 5, 2019

Copy link
Member

left a comment

LGTM.

@ogrisel

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

The failure on circle ci is caused by an old version of scikit-image and is unrelated to this PR.

    from numpy.lib.arraypad import _validate_lengths
ImportError: cannot import name '_validate_lengths
@rth

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Once tomMoral/loky#135 is merged can we use most of this PR from loky then, or do we still need some cython files from this PR?

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

We'll still need a helper to get the number of threads for openmp (similar to effective_n_jobs) since it's been decided to not include it in loky, and a test for that base on a prange.

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Besides, we'll need to vendor it in sklearn, because even if it's released, we use loky from joblib so we won't have it if we want to support several versions of joblib.

@NicolasHug
Copy link
Contributor

left a comment

Some comments

Thanks a lot @jeremiedbb for your PRs on openmp support, that's making my life easier :)

@@ -83,6 +83,11 @@ def configuration(parent_package='', top_path=None):
include_dirs=[numpy.get_include()],
libraries=libraries)

config.add_extension("_threaded_libs_helpers",
sources=["_threaded_libs_helpers.pyx"],
include_dirs=[numpy.get_include()],

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Mar 25, 2019

Contributor

nitpick: you shouhldn't need to include numpy lib right?

# Check that an OpenMP library is loaded
limits = get_thread_limits()

assert not all([lib is None for lib in [limits['openmp_llvm'],

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Mar 25, 2019

Contributor

use any(lib for lib...)?

This comment has been minimized.

Copy link
@tomMoral

tomMoral Mar 25, 2019

Contributor

This would create a generator.

In [1]: any(True for i in range(2))                                             
Out[1]: <generator object <genexpr> at 0x7fb5eff82308>

In [2]: any([True for i in range(2)])                                           
Out[2]: True

if clib == "openblas" and SKIP_OPENBLAS:
raise SkipTest("Possible bug in getting maximum number of threads with"
" OpenBLAS < 0.2.16 and OpenBLAS does not expose it's "

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Mar 25, 2019

Contributor

its (without the ')

This comment has been minimized.

Copy link
@tomMoral

tomMoral Mar 25, 2019

Contributor

thanks for the typo

return n_threads


def check_num_threads(int n=100):

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Mar 25, 2019

Contributor

This is only useful for the tests?

If that's the case then it should maybe be private, and that should be explained in the docstring so that developers don't think they might use it.

Also if I understand correctly, I would suggest the following docstring (since it's not obvious what default number of threads means here):

Run a short parallel section, and return the number of threads that where effectively used by openmp.

If None, does not do anything.
subset : string or None, optional (default="all")

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Mar 25, 2019

Contributor

Isn't this redundant with the limits parameter as a dict?
It's not clear to me why one would prefer one over the other.

Also the accepted keys of the dict for limits should be documented

This comment has been minimized.

Copy link
@tomMoral

tomMoral Mar 25, 2019

Contributor

This is not redundant as the dict for limit permits to select specific implementation (for instance you can have two different openblas for numpy and scipy) while the subset (rename user_api in the loky version) permit to set globally, for all library implementing the BLAS or openmp protocol some limits.

-------
version : string or None
None means OpenBLAS is not loaded or version < 0.3.4, since OpenBLAS
did not expose it's verion before that.

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Mar 25, 2019

Contributor

its
version

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Thanks for the review @NicolasHug but most of the code here has changed. It's being developed in loky (tomMoral/loky#135) and the plan is to vendor it in sklearn once it's done. Not sure if your comments apply to the current version of the code if you to take a look, go ahead :)

@NicolasHug

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

OK, then maybe indicate this in the header to avoid unnecessary reviews

@jeremiedbb jeremiedbb changed the title [MRG] Add helpers for BLAS and OpenMP libs [WIP] Add helpers for BLAS and OpenMP libs Mar 25, 2019

@jeremiedbb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

yep, sorry about that

@tomMoral

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

OK, then maybe indicate this in the header to avoid unnecessary reviews

Thanks for the review @NicolasHug , I took your comments into accounts in the loky PR. :)

@ogrisel

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

For the record, the helpers to protect against over subscription between Cython prange and 3rd party threadpools is being better tested and improved in this repo: https://github.com/joblib/threadpoolctl/

Once ready, the plan is to contribute it to numpy and vendor a backport in scikit-learn in the mean time.

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.