-
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
[WIP] ENH: add TBB support #47
Conversation
for the first todo point, there are 2 things
|
I think it's fine to return
We should at least warn, possibly raise |
threadpoolctl.py
Outdated
@@ -262,6 +276,9 @@ def _get_version(dynlib, internal_api): | |||
return _get_openblas_version(dynlib) | |||
elif internal_api == "blis": | |||
return _get_blis_version(dynlib) | |||
elif internal_api == "tbb": | |||
# tbb does not expose it's version |
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.
hmm.. that's an interesting question. adding an issue oneapi-src/oneTBB#189
There is no way to get ready-to-use version in python package release format like 2019.2. TBB has TBB_runtime_interface_version() which return internal version which is not connected to release versions. Would that be enough or you want it to be the same as the package version?
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.
Actually we need the version of libtbb DSO. So I guess TBB_runtime_interface_version() is what we want. Is that right ?
Thanks
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.
Yes, I believe so.
threadpoolctl.py
Outdated
def get_func(): | ||
if tbb is not None: | ||
return tbb.global_control.active_value( | ||
tbb.global_control.max_allowed_parallelism) |
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.
Maybe move the definition outside of the function scope (something like tbb_*_num_threads
). It would make the code more readable IMO.
I agree with @ogrisel that it is nice to keep it to
I agree that a warning should be issued if |
@anton-malakhov is it intended that when using tbb as mkl threading layer, MKL_NUM_THREADS or mkl_set_num_threads have no effect ? |
@jeremiedbb This is up to MKL implementation of this layer. I guess it happens because it is harder to control the number of TBB threads used in a library. There is a choice - use a separate task_arena for all the library calls, which can control concurrency but adds overheads, or let application to control context in which TBB is executing. I guess they chose the later. |
Thanks |
Closing since it seems that TBB is not meant to have its threads controlled as easily as for the other libs. The threadpoolctl API expects things we can't guarantee with TBB. |
What's wrong with tbb::global_control knob to do this? https://spec.oneapi.io/versions/0.5.0/oneTBB/task_scheduler/tbb_global_control.html |
The issue I encountered is that I can lower the number of threads that TBB can use but then I can't restore it to its original value, which is expected since threadpoolctl is mainly meant to be used as a context manager |
When you create the global_control class instance, do you release it before/after you are creating new one? If yes, then it might be a bug which worth filling |
Not sure, it's been a long time since I haven't touched this PR :) |
fixes #43
TBB's api can't be accessed with ctypes (c++ class), so we need to access it through tbb4py.
todo:
decide what to do when tbb4py is not installed but
libtbb
appears in the module infos.add tests either through mkl threading layer or directly with tbb threadpools.