Use thread-local limiting APIs when possible, and fix deadlock

[itamarst]

Fixes #216
Fixes #229

I figured out a way to measure thread-creation that works on Linux, at least. Or that's the hope, anyway. (ltrace would be much more robust but I can't get it to work on Conda binaries).

[itamarst]

Not sure what's up with openmp suddenly failing to compile on macOS. Seems unrelated to this PR.

[ogrisel]

Thanks very much @itamarst for the PR. Overall, this LGTM but there are a few points to address:

[ogrisel]

I asked an LLM to know if this was a bug or not. Apparently, this is not a bug but a consequence of a being an implementation of an older version of the spec:

Suggested change

	# Windows OpenMP is process-wide:
	# Windows OpenMP is process-wide as it claims to implement
	# the OpenMP 2.0 spec. This would be a violation of the
	# OpenMP 3.0 spec which mandates per-task
	# Internal Control Variables such as `nthreads-var`.
	# https://learn.microsoft.com/en-us/cpp/build/reference/openmp-enable-openmp-2-0-support?view=msvc-180

For information, recent MSVC can be configured to build with the -openmp:llvm flag to leverage libomp instead: https://devblogs.microsoft.com/cppblog/improved-openmp-support-for-cpp-in-visual-studio/

I don't know if there is an easy way to snif libomp specific symbols on such MSVC generated binary files.

[itamarst]

I imagine there's no reason not to use omp_set_num_threads on Windows, it might just not be thread-local (but it won't be worse than openblas_set_num_threads). So as a first pass I will:

• Change the code to run on Windows too.
• Still leave the test disabled on Windows.
• Open an issue to see if detection can be done.
• Open an issue with OpenBLAS on Conda-Forge to use this flag, with the presumption I will verify that it doesn't already.

[itamarst]

Ah, sounds like that VS flag is experimental for now, so maybe I won't suggest it.

[ogrisel]

I think it's still worth expanding the inline comment to explain that the current windows OpenMP runtime implement semantics from an older version of the OpenMP spec but that it is hopefully expected to change in the future, at which point we might want to update this test to make sure it passes on all platforms.

[itamarst]

Not really sure what's going on with the new compiler failures...

[itamarst]

Dug into compiler failures a bit: conda-forge/python-feedstock#914 (comment)

[itamarst]

There's a new PR in conda-forge/python-feedstock to fix the compilation issue, so hopefully by tomorrow it should be working again, will just need to re-run.

[ogrisel]

I retriggered a build and the compiler problems went away but pylatest_flexiblas has the test_setting_limit_on_thread_local_blas_api_is_actually_thread_local[thread_local_blas_libs1] that fails with:

        # _limit_blas runs BLAS operations in 2 Python threads, so by changing the
        # BLAS limit from 1 to 4 we expect an extra 2 * (4 - 1) == 6 threads.
        extra_threads = num_threads_created(4) - num_threads_created(1)
>       assert extra_threads == 6
E       assert 0 == 6

on top of the unrelated macOS OpenMP failures.

[ogrisel]

More feedback on the new iteration of this PR.

[itamarst]

I retriggered a build and the compiler problems went away but pylatest_flexiblas has the test_setting_limit_on_thread_local_blas_api_is_actually_thread_local[thread_local_blas_libs1] that fails with:

        # _limit_blas runs BLAS operations in 2 Python threads, so by changing the
        # BLAS limit from 1 to 4 we expect an extra 2 * (4 - 1) == 6 threads.
        extra_threads = num_threads_created(4) - num_threads_created(1)
>       assert extra_threads == 6
E       assert 0 == 6

on top of the unrelated macOS OpenMP failures.

I've seen occasional results (in CI only) where the number of threads is unexpectedly when number of threads was increased. My current theory is that maybe OpenMP caps number of threads to number of cores, so I'll see if I can figure out hardware used in that run.

[itamarst]

Oh I wonder if that's just because it's flexiblas. So setting the number of threads is using the flexiblas API which doesn't use the _local() MKL variant. I'll open an issue for flexiblas to look at separately since I don't want that to block this PR.

[ogrisel]

LGTM!