Embed parallelization into the multi_voxel_fit decorator. #2593

arokem · 2022-05-08T03:00:50Z

I've started playing around with the idea that the multi_voxel_fit decorator could use paramap instead of iterating over voxels. If we can make this work generally that would be pretty cool. So far, I've only tested this with the fwdti model, and in that case, the change to the additional changes to the code are rather minimal, which gives me hope that we might be able to use this wherever we use this decorator, so in csd, dsi, forecast, fwdti, gqi, ivim, mapmri, mcsd, qtdmri, and shore (!).

pep8speaks · 2022-05-08T03:00:52Z

Hello @arokem, Thank you for updating !

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

Comment last updated at 2024-05-16 04:28:10 UTC

skoudoro · 2022-05-08T17:07:35Z

Thank you for starting this @arokem!

Have you looked at #1418? I think some ideas can be reused here.

arokem · 2022-05-08T22:43:11Z

Oh yeah - that’s a great pointer! I’ll try to incorporate the ideas you implemented there into this PR.

…

On Sun, May 8, 2022 at 10:07 AM Serge Koudoro ***@***.***> wrote: Thank you for starting this @arokem <https://github.com/arokem>? Have you looked at #1418 <#1418>? I think some ideas can be reused here. — Reply to this email directly, view it on GitHub <#2593 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA46NTHC7HEWHHJSVVXWKDVI7YGFANCNFSM5VLIP45A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

codecov · 2022-05-09T04:56:06Z

Codecov Report

Attention: Patch coverage is 80.85106% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 83.72%. Comparing base (f741b88) to head (f593491).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2593      +/-   ##
==========================================
- Coverage   83.75%   83.72%   -0.04%     
==========================================
  Files         153      153              
  Lines       21343    21364      +21     
  Branches     3445     3451       +6     
==========================================
+ Hits        17876    17887      +11     
- Misses       2611     2620       +9     
- Partials      856      857       +1

Files	Coverage Δ
dipy/reconst/csdeconv.py	`87.38% <100.00%> (ø)`
dipy/reconst/dsi.py	`80.21% <100.00%> (ø)`
dipy/reconst/forecast.py	`92.82% <100.00%> (ø)`
dipy/reconst/fwdti.py	`94.28% <100.00%> (ø)`
dipy/reconst/gqi.py	`54.00% <100.00%> (ø)`
dipy/reconst/ivim.py	`96.00% <100.00%> (ø)`
dipy/reconst/mapmri.py	`92.09% <100.00%> (ø)`
dipy/reconst/mcsd.py	`88.69% <100.00%> (ø)`
dipy/reconst/qtdmri.py	`93.56% <100.00%> (ø)`
dipy/reconst/shore.py	`91.90% <100.00%> (ø)`
... and 2 more

... and 1 file with indirect coverage changes

arokem · 2022-05-09T17:06:26Z

I ran a benchmark on a beefy 24-cpu compute server with the recent commit.I get a roughly 13x speedup for fitting the fwdti model with engine="joblib" relative to the default serial mode. I should maybe mention that the server is also doing a bunch of other work, so it's not the cleanest benchmark, but still quite promising.

arokem · 2022-05-16T20:26:33Z

Does anyone understand why half the CI actions are still pending? They have been pending since Friday!

skoudoro · 2022-05-16T21:50:42Z

No, but I will restart them first

skoudoro · 2022-05-17T13:01:39Z

Hi @arokem,

It seems we have a new issue with DIPY installation. I do not know yet what changes. the CI's are failing in all PR.
I will start to dig into it

arokem · 2022-05-17T16:09:26Z

Just rebased on top of #2595

arokem · 2022-05-18T18:50:15Z

Does anyone understand these CI failures? I don't think they are related to the content of the PR, but I might be missing something.

skoudoro · 2022-05-18T20:27:24Z

Does anyone understand these CI failures? I don't think they are related to the content of the PR, but I might be missing something.

Both failures are on the parallelization CI's with a memory leaks issue. This might be due to some of the parallel packages that might change some environment variable flags. These flags could have an impact on this parallelized function.

All supposition, this is what comes first to my mind.

skoudoro · 2022-05-18T20:27:57Z

the failing function are using openmp

arokem · 2022-05-29T21:05:29Z

Hey @skoudoro, I noticed that you did not pin the ray version in #2600, instead pinning only protobuf, but I am seeing this again on the CI: https://github.com/dipy/dipy/runs/6634820045?check_suite_focus=true#step:9:119, which suggests to me that I should pin ray to 0.11 for now. Does that make sense to you? I'll give it a try here.

arokem · 2022-05-29T21:06:25Z

Or possibly 1.11.1

arokem · 2022-05-30T03:02:42Z

We're back to this failure: https://github.com/dipy/dipy/runs/6645881563?check_suite_focus=true#step:9:3751

Interestingly, I can't get this to fail locally on my machine (in an env with dask, ray and joblib installed). I also don't exactly understand how this is related to openmp. Does set_number_of_points use openmp?

arokem · 2022-12-13T19:46:48Z

dipy/reconst/multi_voxel.py

+            single_voxel_with_self = partial(single_voxel_fit, self)
+            n_jobs = kwargs.get("n_jobs", multiprocessing.cpu_count() - 1)
+            vox_per_chunk = np.max([data_to_fit.shape[0] // n_jobs, 1])
+            chunks = [data_to_fit[ii:ii + vox_per_chunk]


This might duplicate memory. Need to benchmark.

We can use this: https://pypi.org/project/memory-profiler/

arokem · 2022-12-13T20:10:26Z

Plan to make progress here:

Set up experimental datasets: All of the models except for DSI can use multi-shell data. Only CSD (I think) can run on single-shell data. For multi-shell datasets we can use HBN and HCP. For DSI, I guess we can use the dsi dataset we have in our data fetchers. We'll need to set up fetchers for HBN data (see Replace CENIR multishell with HBN POD2 data #2695) and for HCP (see Port HCP fetcher from pyAFQ into here #2696).
Set up experimental scripts (separate repo, probably): these should run every one of the models that are decorated in this PR with:
1. Serial mode.
2. Parallelized by voxel with dask, ray, joblib.
3. Parallelized by chunk with dask, ray, joblib.
4. Parallelized with different backends if possible.
5. For ray/dask, parallelize on a big distributed AWS cluster.
Run the experiments. We'll need to have some uniform hardware settings. We'll want to run this on different OS (Windows, Linux, Mac OS) and maybe on different kinds of computational architectures (e.g., distributed cluster vs. one big machine).
Separately benchmark timing (this is straightforward) and memory (using https://github.com/pythonprofilers/memory_profiler).
Compare and contrast 😄

skoudoro

For now, it works with `engin in ["serial", "dask"]

my laptop crash with ray
See below for joblib issue.

I will share the timing when those 2 are fixed.

Thanks @arokem

skoudoro · 2023-01-10T19:08:47Z

dipy/reconst/multi_voxel.py

+                    _parallel_fit_worker,
+                    chunks,
+                    func_args=[single_voxel_with_self],
+                    **kwargs)


dask did not complain but joblib fails with this:

TypeError: __init__() got an unexpected keyword argument 'vox_per_chunk'

we need to update paramap function

arokem · 2024-04-23T21:25:30Z

The errors on the CI are really puzzling and hard to reproduce locally and we worry that it might be some funky interaction with joblib, so I am removing the multi_voxel decorator for that model and we can reinstate it on a separate PR later on, if we fix everything else here.

arokem · 2024-04-23T23:27:08Z

OK - having removed the decorator from the SFM model, it seems that the only remaining error on the CI is completely unrelated. Or am I missing something?

arokem · 2024-04-23T23:31:15Z

At any rate, @asagilmore has now completed an extensive set of experiments with this PR and we are glad to say that Ray in particular provides a substantial speedup for reconstruction models (on the order of 10x) across two different models, and across a pretty wide range of hardware setups and chunking schemes. We're writing up a report about this here and we'd be happy to have input on the results and ideas that we are developing there (the repo for that report is here: https://github.com/nrdg/2024-dipy-parallelization).

With that said, I think that this code and the results we report are ready for a review, and are hopefully close to a shape where they can be merged for an upcoming release.

arokem · 2024-04-26T11:21:12Z

Looks like installing pytables on mac is (newly?) broken: https://github.com/dipy/dipy/actions/runs/8847440312/job/24295269319?pr=2593#step:6:125

skoudoro · 2024-04-26T11:27:13Z

Yes, I saw that with pytable.

Not sure what we can do apart from reporting.

The last release was 6month ago so not sure what changed last week.

Maybe a release of h5py...

arokem · 2024-04-26T11:30:16Z

There was one on April 10th: https://pypi.org/project/h5py/3.11.0/

I'm trying to pin to 3.10 in cd0e653. Let's see what we learn.

arokem · 2024-04-26T12:59:37Z

Following #3202, what's the right way to catch a warning thrown when importing one of the optional dependencies? I am getting:

reconst/tests/test_multi_voxel.py::test_multi_voxel_fit
  /home/runner/work/dipy/dipy/venv/lib/python3.10/site-packages/ray/_private/pydantic_compat.py:2: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
    from pkg_resources import packaging

reconst/tests/test_multi_voxel.py::test_multi_voxel_fit
  /home/runner/work/dipy/dipy/venv/lib/python3.10/site-packages/pkg_resources/__init__.py:2832: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('google')`.
  Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
    declare_namespace(pkg)

I believe this is emitted when importing ray. Should I explicitly ignore it in a context manager around that import?

skoudoro · 2024-04-26T13:04:14Z

The "right way" would be to add the warning message with ignore status in the pyproject.toml.

see here:

dipy/pyproject.toml

Lines 184 to 186 in 60669b8

    
           filterwarnings = [ 
        
               'ignore:.*You do not have FURY installed.*:UserWarning', 
        
           ]

skoudoro · 2024-04-26T13:05:09Z

if it is too much for the pyproject.toml, we could add/create a specific python file for that.

skoudoro · 2024-04-26T13:07:37Z

For example, mne-python do it directly in the conftest.py instead of the pyproject.toml.

see here: https://github.com/mne-tools/mne-python/blob/main/mne/conftest.py#L131-L178

So, this is something to decide together, what is your opinion @arokem and @jhlegarreta ?

arokem · 2024-04-26T13:11:09Z

I like it better in the conftest

jhlegarreta · 2024-04-26T13:42:50Z

Hats off for this work, Ariel.

So, this is something to decide together, what is your opinion @arokem and @jhlegarreta ?

Having filtering rules both in the pyproject.toml and the conftest.py file would make things easier to follow, as we would need to look at two files instead of one. Also, not sure if pytest would end up by overriding the rules in one file by the ones that it reads in the last place.

So probably as Serge says, if the list of rules becomes too lengthy, better to keep them all in the conftest.py file.

skoudoro · 2024-04-26T13:47:11Z

OK, thank you for your feedback @arokem and @jhlegarreta.

I will create a PR later today to update that. Also, I think we should add it to the developer guide concerning the warnings policy.

arokem · 2024-04-26T13:58:39Z

Thanks! I will wait for your PR to see how this is done.

In the meanwhile, it doesn't look like pinning h5py helped with the pytables installation.

skoudoro added the type:New feature label May 8, 2022

arokem changed the title ~~WIP: Embed parallelization into the multi_voxel_fit decorator.~~ Embed parallelization into the multi_voxel_fit decorator. May 9, 2022

arokem force-pushed the para_multivoxel branch from 892d26c to 7628a78 Compare May 17, 2022 16:09

arokem force-pushed the para_multivoxel branch from 7628a78 to a9b3c2f Compare May 28, 2022 05:07

arokem force-pushed the para_multivoxel branch from 48f7b3f to 8d7f71b Compare May 30, 2022 02:53

arokem commented Dec 13, 2022

View reviewed changes

arokem force-pushed the para_multivoxel branch from 8d7f71b to 173160c Compare December 13, 2022 19:51

arokem force-pushed the para_multivoxel branch from 173160c to ddac5c2 Compare December 19, 2022 19:39

skoudoro reviewed Jan 10, 2023

View reviewed changes

skoudoro force-pushed the master branch 5 times, most recently from 7e158ff to dda2ffa Compare December 8, 2023 16:00

arokem added 5 commits April 16, 2024 14:19

Merge branch 'master' into para_multivoxel

779938c

Remove duplicated imports.

6f46746

Used ruff --fix to fix mis-ordered import blocks.

01486d8

Pin a newer version of ray for parallelization tests.

6db5fbe

BF: Use elif to continue the same logical sequence.

54c9c34

arokem mentioned this pull request Apr 16, 2024

Should parallelization be handled upstream? nipreps/eddymotion#174

Open

Does removing the multi_voxel decorator from the SFM fix the CI?

b1b84a1

arokem mentioned this pull request Apr 26, 2024

Hide model fit progress bar? nipreps/eddymotion#181

Open

arokem added 2 commits April 26, 2024 06:14

Merge branch 'master' into para_multivoxel

54fa651

Allows setting a verbose flag for fitting, which would disable tqdm.

44127d5

Test pinning h5py.

cd0e653

skoudoro mentioned this pull request Apr 29, 2024

[CI] Move filterwarnings from pyproject to conftest #3209

Merged

arokem added 4 commits May 3, 2024 13:37

Merge branch 'master' into para_multivoxel

bdaefbb

Merge branch 'master' into para_multivoxel

89fcda8

Filter upstream warnings.

09fb24d

Merge branch 'master' into para_multivoxel

f593491

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embed parallelization into the multi_voxel_fit decorator. #2593

Embed parallelization into the multi_voxel_fit decorator. #2593

arokem commented May 8, 2022

pep8speaks commented May 8, 2022 •

edited

skoudoro commented May 8, 2022 •

edited

arokem commented May 8, 2022 via email

codecov bot commented May 9, 2022 •

edited

arokem commented May 9, 2022

arokem commented May 16, 2022

skoudoro commented May 16, 2022

skoudoro commented May 17, 2022

arokem commented May 17, 2022

arokem commented May 18, 2022

skoudoro commented May 18, 2022

skoudoro commented May 18, 2022

arokem commented May 29, 2022

arokem commented May 29, 2022

arokem commented May 30, 2022

arokem Dec 13, 2022

arokem Dec 13, 2022

arokem commented Dec 13, 2022 •

edited

skoudoro left a comment

skoudoro Jan 10, 2023

arokem commented Apr 23, 2024

arokem commented Apr 23, 2024

arokem commented Apr 23, 2024

arokem commented Apr 26, 2024

skoudoro commented Apr 26, 2024

arokem commented Apr 26, 2024

arokem commented Apr 26, 2024

skoudoro commented Apr 26, 2024

skoudoro commented Apr 26, 2024 •

edited

skoudoro commented Apr 26, 2024 •

edited

arokem commented Apr 26, 2024

jhlegarreta commented Apr 26, 2024

skoudoro commented Apr 26, 2024

arokem commented Apr 26, 2024

Embed parallelization into the multi_voxel_fit decorator. #2593

Are you sure you want to change the base?

Embed parallelization into the multi_voxel_fit decorator. #2593

Conversation

arokem commented May 8, 2022

pep8speaks commented May 8, 2022 • edited

Comment last updated at 2024-05-16 04:28:10 UTC

skoudoro commented May 8, 2022 • edited

arokem commented May 8, 2022 via email

codecov bot commented May 9, 2022 • edited

Codecov Report

arokem commented May 9, 2022

arokem commented May 16, 2022

skoudoro commented May 16, 2022

skoudoro commented May 17, 2022

arokem commented May 17, 2022

arokem commented May 18, 2022

skoudoro commented May 18, 2022

skoudoro commented May 18, 2022

arokem commented May 29, 2022

arokem commented May 29, 2022

arokem commented May 30, 2022

arokem Dec 13, 2022

Choose a reason for hiding this comment

arokem Dec 13, 2022

Choose a reason for hiding this comment

arokem commented Dec 13, 2022 • edited

skoudoro left a comment

Choose a reason for hiding this comment

skoudoro Jan 10, 2023

Choose a reason for hiding this comment

arokem commented Apr 23, 2024

arokem commented Apr 23, 2024

arokem commented Apr 23, 2024

arokem commented Apr 26, 2024

skoudoro commented Apr 26, 2024

arokem commented Apr 26, 2024

arokem commented Apr 26, 2024

skoudoro commented Apr 26, 2024

skoudoro commented Apr 26, 2024 • edited

skoudoro commented Apr 26, 2024 • edited

arokem commented Apr 26, 2024

jhlegarreta commented Apr 26, 2024

skoudoro commented Apr 26, 2024

arokem commented Apr 26, 2024

pep8speaks commented May 8, 2022 •

edited

skoudoro commented May 8, 2022 •

edited

codecov bot commented May 9, 2022 •

edited

arokem commented Dec 13, 2022 •

edited

skoudoro commented Apr 26, 2024 •

edited

skoudoro commented Apr 26, 2024 •

edited