Workaround for openblas + openmp threading layer

[jeremiedbb]

No description provided.

[ogrisel]

Indeed, test_threadpool_controller_limit failing as reported in #110:

https://dev.azure.com/joblib/threadpoolctl/_build/results?buildId=1406&view=logs&j=c58f1808-95ea-5220-6c46-471ee848c1df&t=74046923-2fe7-50f5-309f-1dd07d06f3be&l=104

[ogrisel]

I think we need a dedicated API to protect about the sequential_blas_under_openmp oversubscription case that we need in scikit-learn.

• For an OpenBLAS BLAS implementation with an OpenMP threading layer and if there is a single OpenMP runtime loaded, assume that OpenMP will already handle the case natively and do nothing;
• For an OpenBLAS implementation with a threading layer that is either native or OpenMP but there are several active OpenMP runtimes linked to the Python process, set limit=1 on this OpenBLAS;
• For MKL and BLIS with OpenMP threading layers, we need to check.

This should be a new context manager, both for the functional and the pre-loaded object oriented API.

WDYT @jeremiedbb?

[jeremiedbb]

So at first I tried this new context manager idea but I was very unsatisfied with it. I think I came up with a more satisfying solution. When we set the limit to blas only, we check if the internal state of openmp has changed and if so we set it back to the original state (this does not modify the state of the blas, I checked).

I thinks it's better to just have this kind of workaround for a behavior that is unexpected at least and should even be considered a bug imo, than adding a new whole public object just to handle that. What do you think ?

[jeremiedbb]

Hum, actually setting back openmp to its original state is bad when we don't want to set the limits for the nested openmp openblas case... It will run as if no limit was set even if openblas_get_num_threads returns 1.

[jeremiedbb]

Ok so I think I came up with an acceptable alternative: a new helper function that gives the appropriate params to pass to threadpool_limits only for the use case of nested blas inside an openmp parallel section. There's no need to introduce a new context manager.

[ogrisel]

Nice!

Maybe this could also be somehow exposed in the .limit function? For instance

with threadpoolctl.threadpool_limits(limits="sequential_blas_under_openmp"):
    # code protected against over-subscription

WDYT @jeremiedbb?

If you agree, please update the README to document that high level policy.

[jeremiedbb]

Maybe this could also be somehow exposed in the .limit function?

I like this Idea. Actually I think it should be the only public way to do that and I would make the helper private. Ok for you ?

[ogrisel]

I agree. Let's make the getter private. The changelog will need to be updated accordingly to only mention the new limit option.

[ogrisel]

LGTM!

[ogrisel]

This PR is missing a changelog entry though.

[ogrisel]

I find the construction of the first sentence in the changelog entry a bit weird. See below:

[ogrisel]

Still [WIP]? If not, +1 for merging and maybe Friday evening release ;) Or maybe not releasing today.

[ogrisel]

Suggested change

	- `threadpool_info` and `ThreadpoolController.limit` now accept the string
	- `threadpool_limit` and `ThreadpoolController.limit` now accept the string

[jeremiedbb]

maybe Friday evening release

I can't think of a better time to do this. But unfortunately I don't have time and I'm still not crazy enough to make a half release a friday evening and finish it during the week-end :D

[jeremiedbb]

Ok all green. Let's merge