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] ENH: add TBB support #47

Closed
wants to merge 9 commits into from
Closed

Conversation

jeremiedbb
Copy link
Collaborator

@jeremiedbb jeremiedbb commented Oct 23, 2019

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.

@jeremiedbb
Copy link
Collaborator Author

jeremiedbb commented Oct 23, 2019

for the first todo point, there are 2 things

  • currently calling threadpool_info when tbb4py is not installed but libtbb is loaded will display 'num_threads: None'. I would not show None but something like "not available (install tbb4py)". Is it too verbose for this field ?

  • currently calling threadpool_limits(1, 'tbb') will silently do nothing if tbb4py is not installed. I think it's not nice. I would raise a warning. Do you agree ?

@ogrisel
Copy link
Contributor

ogrisel commented Oct 23, 2019

currently calling threadpool_info when tbb4py is not installed but libtbb is loaded will display 'num_threads: None'. I would not show None but something like "not available (install tbb4py)". Is it too verbose for this field ?

I think it's fine to return num_threads: None. Maybe we could have a new "comment", "cause" or "status" field in the dict we return that would be valued "tbb4py required" in case we cannot instrospect the number of threads.

currently calling threadpool_limits(1, 'tbb') will silently do nothing if tbb4py is not installed. I think it's not nice. I would raise a warning. Do you agree ?

We should at least warn, possibly raise ImportError or RuntimeError to tell the user to install tbb4py but only when calling threadpool_limits on tbb explicitly.

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
Copy link

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?

Copy link
Collaborator Author

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

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)
Copy link
Collaborator

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.

@tomMoral
Copy link
Collaborator

tomMoral commented Oct 24, 2019

  • currently calling threadpool_info when tbb4py is not installed but libtbb is loaded will display 'num_threads: None'. I would not show None but something like "not available (install tbb4py)". Is it too verbose for this field ?

I agree with @ogrisel that it is nice to keep it to None but add a status field to give some hints.

  • currently calling threadpool_limits(1, 'tbb') will silently do nothing if tbb4py is not installed. I think it's not nice. I would raise a warning. Do you agree ?

I agree that a warning should be issued if threadpool_limits(1) is called on a program where libtbb is linked without tbb4py and an RuntimeError if threadpool_limits(1, 'tbb') is called.

@jeremiedbb
Copy link
Collaborator Author

@anton-malakhov is it intended that when using tbb as mkl threading layer, MKL_NUM_THREADS or mkl_set_num_threads have no effect ?

@anton-malakhov
Copy link

@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.

@jeremiedbb
Copy link
Collaborator Author

Thanks

@jeremiedbb
Copy link
Collaborator Author

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.

@jeremiedbb jeremiedbb closed this Jan 28, 2022
@anton-malakhov
Copy link

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

@jeremiedbb
Copy link
Collaborator Author

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

@anton-malakhov
Copy link

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

@jeremiedbb
Copy link
Collaborator Author

Not sure, it's been a long time since I haven't touched this PR :)
Maybe I did something wrong, I can give it another try in a new PR. The code base has changed so much since then that it can't be synced with master anyway now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for TBB introspection and set_num_thread
4 participants