-
Notifications
You must be signed in to change notification settings - Fork 30
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
[MRG] ENH Add BLIS support #23
Conversation
still WIP because not tested in CI. I'll try to setup some builds with numpy linked against BLIS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Is this still a WIP?
The uncovered line is the handling of single-threaded blis. In tests I enabled multi-threaded blis. The timeout job is unrelated to blis. It comes from mkl-clang bad interaction. |
The problem is that apparently only this PR can trigger the issue. So maybe it's related to BLIS indirectly somehow? |
In addition to the global methods based on environment variables and runtime function calls, BLIS also offers a local, per-call method of requesting parallelism at runtime. This method has the benefit of being thread-safe and flexible; your application can spawn two threads at the application level, with each thread requesting different degrees of parallelism from their respective calls to level-3 BLIS operations. Note: Neither way (automatic nor manual) of specifying multithreading via the local runtime API can be used via the BLAS interfaces. |
&beta, &C[i * chunk_size, 0], &n) | ||
with nogil, parallel(num_threads=nthreads): | ||
if openmp.omp_get_thread_num() == 0: | ||
with gil: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured out what was the cause of the hanging. Note exactly the cause but at least which line of the code was faulty. It happens if we try to call threadpool_info
in another thread than the main thread (I was doing that in a previous commit).
I don't know why but it's probably related to the bad state of OpenMP threadpool in non main thread when there are multiple openmp loaded.
Anyway, it's unrelated to this PR and should be investigated separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I see that you opened #27 to track this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides the following:
# by default BLIS is single-threaded and get_num_threads returns -1. | ||
# we map it to 1 for consistency with other libraries. | ||
if module['num_threads'] == -1 and module['internal_api'] == 'blis': | ||
module['num_threads'] = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is allegedly not covered by any test according to codecov. Do you think it would be possible to make sure it's covered? I believe the only way is to introduce a new build configuration without export BLIS_NUM_THREADS=4
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
According to @glemaitre the coverage issue is a false positive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Fixes #15
By default, BLIS is sequential and
num_threads
will be -1 in the infos.It can be a bit confusing since -1 means all cores sometimes, e.g. sklearn. I think we should map -1 to +1. wdyt ?