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][FIX] option to disable multithreading while multiprocessing #2547

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skoudoro
Copy link
Member

@skoudoro skoudoro commented Mar 4, 2022

This PR fixes #2519. It permits to avoid thread oversubscription. In the close future, we need to improve the design and insert it in the parallel module. Small Notes: the loky backend from joblib solve this issue. This fix might be useful for the other joblib backends

@pep8speaks
Copy link

pep8speaks commented Mar 4, 2022

Hello @skoudoro, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2022-03-04 04:02:02 UTC

@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

Merging #2547 (b37318d) into master (120fe8f) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2547      +/-   ##
==========================================
- Coverage   85.02%   85.02%   -0.01%     
==========================================
  Files         127      127              
  Lines       17426    17445      +19     
  Branches     2963     2967       +4     
==========================================
+ Hits        14817    14832      +15     
  Misses       1936     1936              
- Partials      673      677       +4     
Impacted Files Coverage Δ
dipy/utils/multiproc.py 78.78% <76.47%> (-2.47%) ⬇️
dipy/direction/peaks.py 84.65% <100.00%> (+0.15%) ⬆️

Copy link
Contributor

@arokem arokem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether it wouldn't be more elegant to implement this as an object that stores current_openblas and current_mkl as state, rather than storing those in environment variables.

current_openblas = os.environ.get('OPENBLAS_NUM_THREADS', '')
current_mkl = os.environ.get('MKL_NUM_THREADS', '')

# import ipdb; ipdb.set_trace()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debugging statement



def disable_np_threads():
"""Reduce OPENBLAS and MKL thread numbers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to make the number of threads an argument to this function? Right now, this always reduces to 1, but maybe sometimes we want to reduce down to number of cpus?

@skoudoro
Copy link
Member Author

skoudoro commented Mar 7, 2022

I wonder whether it wouldn't be more elegant to implement this as an object that stores current_openblas and current_mkl as state, rather than storing those in environment variables.

Not a bad idea. I will look into it. Also, I will turn this into a work in progress. It needs testing and exploration. Thank you for the feedback!

@skoudoro skoudoro changed the title [FIX] option to disable multithreading while multiprocessing [WIP][FIX] option to disable multithreading while multiprocessing Mar 7, 2022
@skoudoro skoudoro force-pushed the master branch 7 times, most recently from 1419292 to ca6268a Compare December 8, 2023 22:25
@skoudoro skoudoro force-pushed the master branch 3 times, most recently from 5935e1e to 765963e Compare January 24, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an option to disable multithreading while multiprocessing
3 participants