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

[ROI] Per-ROI parallelization introduces dask overhead (with respect to per-chunk parallelization) #26

Closed
tcompa opened this issue Jul 27, 2022 · 5 comments
Assignees
Labels
High Priority Current Priorities & Blocking Issues Tables AnnData and ROI/feature tables

Comments

@tcompa
Copy link
Collaborator

tcompa commented Jul 27, 2022

TL;DR
Working on an array through ROI indices (rather than chunks) produces more complex dask graphs and has a (small?) time overhead. This is not surprising, given the increased flexibility we aim for.


With per-ROI parallelization, the elements of a dask array (identified by their indices) are populated with values obtained from a delayed function (e.g. illumination correction, or labeling). The parallelization based on this loop of assignments through indices is more complex than the one provided by map_blocks, which rather acts directly onto chunks.

As an example, here are two dask graphs for the OLD (per-chunk) and NEW (per-ROI) illumination-correction tasks (with overwrite=False, but that doesn't matter), when acting on an artificial array with shape (2, 2, 4320, 2560).
The on-disk array has 8 chunks (2 channels, 2 Z planes and 2 images), and indeed we notice 8 branches in the OLD graph. The NEW graph also has 8 branches (one per ROI), but with a more complex structure.

Timing of these tests shows a small additional overhead in the ROI-based version (about 0.5 s of overhead, for a total runtime of about 5 s). This is not something we can get rid of, as we switched from a natural (per-chunk) parallelization scheme to an arbitrary one. Still, we should check that it is still under control for an example at scale.


OLD (per-chunk):
fig_old_graph_overwrite_0

NEW (per-ROI):
fig_new_graph_overwrite_0

@tcompa tcompa self-assigned this Jul 27, 2022
@tcompa
Copy link
Collaborator Author

tcompa commented Jul 27, 2022

Closing this issue would require a comparison of run times for the per-chunk and per-FOV illumination correction tasks on the same dataset.

@jluethi
Copy link
Collaborator

jluethi commented Jul 27, 2022

Cool, very visual way to understand this, thanks @tcompa !
If we remain at ~10% overhead for arbitrary ROIs, that's totally fine :)

Can we run e.g. the illumination correction for the 10 well, 5x5 case example in the old and new setup? If they remain within a ~10% range of run time, I don't think we'd need to worry much about this.

Also, this explains why it's save to run multi-ROI in parallel within a single job I'd guess :)

@tcompa
Copy link
Collaborator Author

tcompa commented Aug 2, 2022

Closing this issue would require a comparison of run times for the per-chunk and per-FOV illumination correction tasks on the same dataset.

The current discussion (see #27) is rather on memory, while running times of illumination correction are under control (they are even better, in a certain per-ROI version, than old per-chunk ones).
Notice that illumination correction is the only task where a time/memory comparison between the two schemes is directly meaningful, as the per-ROI version still does exactly the same thing as the per-chunk one (i.e. it still works at level 0, and with 2D ROIs). The only similar case is per-FOV labeling, when used at level 0, while any task that runs at level>0 cannot be used for a direct comparison.

I think we can close this issue as soon as we are happy with #27 (cause otherwise subsequent changes will require re-testing the running times).

@tcompa tcompa changed the title Per-ROI parallelization introduces dask overhead (with respect to per-chunk parallelization) [ROI] Per-ROI parallelization introduces dask overhead (with respect to per-chunk parallelization) Aug 2, 2022
@jluethi jluethi transferred this issue from fractal-analytics-platform/fractal-client Sep 2, 2022
@tcompa
Copy link
Collaborator Author

tcompa commented Sep 19, 2022

This is in principle under control, after #79 and #80, and it will be even more under control after #83.
Let's close once we have a convincing test, e.g. the one in #30.

@tcompa tcompa added the High Priority Current Priorities & Blocking Issues label Sep 20, 2022
@jluethi
Copy link
Collaborator

jluethi commented Sep 27, 2022

@tcompa Have you checked the resource usage for #30? If that's in check, I think we can close this issue & the milestone :) It runs on cpu setups that don't take a full node, so from my experience, that looks good :)

@jluethi jluethi closed this as completed Sep 28, 2022
@tcompa tcompa added the Tables AnnData and ROI/feature tables label Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority Current Priorities & Blocking Issues Tables AnnData and ROI/feature tables
Projects
None yet
Development

No branches or pull requests

2 participants