Repository navigation
Conversation
itamarst
changed the title
itamarst
changed the title
ogrisel
left a comment
ogrisel
left a comment
There was a problem hiding this comment.
Thanks for the fixes. Overall LGTM besides the following minor suggestions.
| total_original_info = self._original_info.stack.pop() | ||
| for lib_controller, original_info in zip( | ||
| self._controller.lib_controllers, total_original_info | ||
| ): |
There was a problem hiding this comment.
This assumes that if the decorated function triggers the import of a new controller (e.g. by importing an openmp extension or a BLAS linked package and then threadpool_info afterwards), it will be appended to the existing self._controller.lib_controllers and zipping with a shorter total_original_info list should automatically discard the newly imported trailing controllers. I think this should be fine but I think we need an inline comment to make that assumption explicit and maybe even a non-regression test for that particular case if none of the existing tests already cover that particular edge case.
| assert lib.get_num_threads() == 1 | ||
|
|
||
| recurse(1) | ||
| assert lib.get_num_threads() == orig_limit |
There was a problem hiding this comment.
We could assert that no controlling thread is registered at this point.
|
|
||
| @wrap(limits=1, user_api="blas") | ||
| def recurse(depth): | ||
| assert lib.get_num_threads() == 1 |
There was a problem hiding this comment.
We could also assert that the current thread is in control here.
|
|
||
| @threadpool_limits.wrap(limits=1, user_api="blas") | ||
| def recurse(depth): | ||
| assert lib.get_num_threads() == 1 |
There was a problem hiding this comment.
Same comment about asserting that the current thread is in control.
| assert lib.get_num_threads() == 1 | ||
| if depth: | ||
| recurse(depth - 1) | ||
| assert lib.get_num_threads() == 1 |
There was a problem hiding this comment.
Maybe we could even spawn and then join a worker thread here to check that it cannot take control.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Fixes #292
Fixes #293