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.

Sign up for GitHub

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

Jump to bottom

Slice layers asynchronously #4795

Open
andy-sweet opened this issue Jul 8, 2022 · 0 comments
Open

Slice layers asynchronously #4795

andy-sweet opened this issue Jul 8, 2022 · 0 comments
Assignees
@andy-sweet
Labels
Epic feature New feature or request vision

Comments

@andy-sweet
Copy link
Member

andy-sweet commented Jul 8, 2022 •
edited
Loading

🚀 Feature

In napari, slicing a layer refers to the act of generating a partial view of the layer’s data. The main use of slicing is to define the data that should be rendered in the canvas based on the dimension slider positions and the current field of view. This feature is about performing slicing asynchronously (i.e. not on the main thread).

Motivation

Currently, all slicing is performed synchronously. For example, if a dimension slider is moved, napari blocks while slicing each layer before updating the canvas. When slicing layers is slow, this blocking behavior makes interacting with data difficult and napari may be reported as non-responsive by the host operating system.

sync-em-multires.mp4

Consider two reasons why slicing can be slow.

  1. Some layer specific slicing operations perform non-trivial calculations (e.g. points).
  2. The layer data is read lazily (i.e. it is not in RAM) and latency from the source may be non-negligible (e.g. stored remotely, napari-omero plugin).

By slicing asynchronously, we can keep napari responsive while allowing for slow slicing operations.

Pitch

Add at least one non-main thread to keep the napari responsive to user input (e.g. panning, zooming, sliding) while a slice is being generated or data is being read. This should at least work for image layers, and should ideally generalize to any layer types or any dimension.

Alternatives

There is experimental support for asynchronous slicing in napari already, which can be enabled in preferences or by setting the environment variable NAPARI_ASYNC=1 before running napari. This is being used by some with success and contains some advanced behavior, but is not complete and also complicates slicing logic in napari.

Additional context

This issue is the main tracking issue for this work, which has been in a planning/prototyping phase for the last few months through architecture working group meetings and on Zulip. I created this as a tracking issue as a more public way to collect feedback.

Draft project plan / ideas: https://hackmd.io/YN6Qhu18QriV-rtVI6Pzog?view
GitHub project board: https://github.com/orgs/napari/projects/18/views/1
@andy-sweet andy-sweet added feature New feature or request Epic vision labels Jul 8, 2022
@andy-sweet andy-sweet self-assigned this Jul 8, 2022
@andy-sweet andy-sweet mentioned this issue Jul 15, 2022
Closed
This was referenced Aug 1, 2022
Merged
Merged
@andy-sweet andy-sweet mentioned this issue Aug 18, 2022
Merged
@andy-sweet andy-sweet mentioned this issue Sep 1, 2022
Merged
7 tasks
This was referenced Sep 8, 2022
Merged
Merged
@andy-sweet andy-sweet mentioned this issue Oct 27, 2022
Merged
8 tasks
@andy-sweet andy-sweet mentioned this issue Jan 18, 2023
Merged
1 task
andy-sweet added a commit that referenced this issue Jan 23, 2023
@andy-sweet
Remove layer ndisplay event (#5491)
7b1be74 
# Description
This removes the private event `Layer.events._ndisplay`. This was needed
to notify the layer control widgets, some of which should only be
presented in 2D others in 3D. Now, each layer control widget stores
`ndisplay` as a public mutable property and triggers similar updates
when `ViewerModel.dims.ndisplay` changes.

There are two motivations for this change.

1. It makes `Layer._slice_dims` simpler and more clearly similar to
`Layer.refresh`.
2. We use and test against the public API of the widgets and model
rather than rely on a private event.

The main downside of this change is that the layer control widget has
some extra state (`ndisplay`) that we need to keep in sync. Initially, I
hoped we could pass through the new value of `ndisplay` through events
(rather than store it as state), but things are complex enough in
`QtImageControls` to make that difficult.

## Type of change
This is mostly a refactor with no napari public API changes.

# References
Partly motivated by #4795

# How has this been tested?
- [x] all existing tests pass with my change
@andy-sweet andy-sweet mentioned this issue Feb 9, 2023
Merged
@kcpevey kcpevey mentioned this issue Apr 11, 2023
Closed
10 tasks
This was referenced May 9, 2023
Merged
Merged
brisvag pushed a commit that referenced this issue May 15, 2023
@andy-sweet
Add test coverage for async slicing of points and fix rounding bug (#…
e720318 
…5783)

# Description

This PR adds some async slicing integration tests for points, one of
which was failing due to unintended rounding. It also adds some debug
logs across sync and async slicing paths to make it easier to debug
issues. Finally, it changes the main log message format to include the
thread name, which is incredibly helpful when debugging concurrency
issues.

# References

This was split off from #5711 to avoid bloating that already large PR.
Credit to @kcpevey, who found the the bug and wrote most of #5711. Also
this part of the larger #4795

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# How has this been tested?
- [x] added new tests to cover issues
- [x] all existing tests pass with my change

I manually tested behavior and logging with async off and on.
@brisvag brisvag mentioned this issue Jun 8, 2023
Merged
Czaki pushed a commit that referenced this issue Jun 18, 2023
@andy-sweet @Czaki
Remove layer ndisplay event (#5491)
a778232 
This removes the private event `Layer.events._ndisplay`. This was needed
to notify the layer control widgets, some of which should only be
presented in 2D others in 3D. Now, each layer control widget stores
`ndisplay` as a public mutable property and triggers similar updates
when `ViewerModel.dims.ndisplay` changes.

There are two motivations for this change.

1. It makes `Layer._slice_dims` simpler and more clearly similar to
`Layer.refresh`.
2. We use and test against the public API of the widgets and model
rather than rely on a private event.

The main downside of this change is that the layer control widget has
some extra state (`ndisplay`) that we need to keep in sync. Initially, I
hoped we could pass through the new value of `ndisplay` through events
(rather than store it as state), but things are complex enough in
`QtImageControls` to make that difficult.

This is mostly a refactor with no napari public API changes.

Partly motivated by #4795

- [x] all existing tests pass with my change
Czaki pushed a commit that referenced this issue Jun 19, 2023
@andy-sweet @Czaki
Remove layer ndisplay event (#5491)
16ca7f1 
This removes the private event `Layer.events._ndisplay`. This was needed
to notify the layer control widgets, some of which should only be
presented in 2D others in 3D. Now, each layer control widget stores
`ndisplay` as a public mutable property and triggers similar updates
when `ViewerModel.dims.ndisplay` changes.

There are two motivations for this change.

1. It makes `Layer._slice_dims` simpler and more clearly similar to
`Layer.refresh`.
2. We use and test against the public API of the widgets and model
rather than rely on a private event.

The main downside of this change is that the layer control widget has
some extra state (`ndisplay`) that we need to keep in sync. Initially, I
hoped we could pass through the new value of `ndisplay` through events
(rather than store it as state), but things are complex enough in
`QtImageControls` to make that difficult.

This is mostly a refactor with no napari public API changes.

Partly motivated by #4795

- [x] all existing tests pass with my change
Czaki pushed a commit that referenced this issue Jun 21, 2023
@andy-sweet @Czaki
Remove layer ndisplay event (#5491)
c8886ac 
This removes the private event `Layer.events._ndisplay`. This was needed
to notify the layer control widgets, some of which should only be
presented in 2D others in 3D. Now, each layer control widget stores
`ndisplay` as a public mutable property and triggers similar updates
when `ViewerModel.dims.ndisplay` changes.

There are two motivations for this change.

1. It makes `Layer._slice_dims` simpler and more clearly similar to
`Layer.refresh`.
2. We use and test against the public API of the widgets and model
rather than rely on a private event.

The main downside of this change is that the layer control widget has
some extra state (`ndisplay`) that we need to keep in sync. Initially, I
hoped we could pass through the new value of `ndisplay` through events
(rather than store it as state), but things are complex enough in
`QtImageControls` to make that difficult.

This is mostly a refactor with no napari public API changes.

Partly motivated by #4795

- [x] all existing tests pass with my change
Czaki pushed a commit that referenced this issue Jun 21, 2023
@andy-sweet @Czaki
Remove layer ndisplay event (#5491)
ee4ed70 
This removes the private event `Layer.events._ndisplay`. This was needed
to notify the layer control widgets, some of which should only be
presented in 2D others in 3D. Now, each layer control widget stores
`ndisplay` as a public mutable property and triggers similar updates
when `ViewerModel.dims.ndisplay` changes.

There are two motivations for this change.

1. It makes `Layer._slice_dims` simpler and more clearly similar to
`Layer.refresh`.
2. We use and test against the public API of the widgets and model
rather than rely on a private event.

The main downside of this change is that the layer control widget has
some extra state (`ndisplay`) that we need to keep in sync. Initially, I
hoped we could pass through the new value of `ndisplay` through events
(rather than store it as state), but things are complex enough in
`QtImageControls` to make that difficult.

This is mostly a refactor with no napari public API changes.

Partly motivated by #4795

- [x] all existing tests pass with my change
Czaki pushed a commit that referenced this issue Jun 21, 2023
@andy-sweet @Czaki
Remove layer ndisplay event (#5491)
91f8b53 
This removes the private event `Layer.events._ndisplay`. This was needed
to notify the layer control widgets, some of which should only be
presented in 2D others in 3D. Now, each layer control widget stores
`ndisplay` as a public mutable property and triggers similar updates
when `ViewerModel.dims.ndisplay` changes.

There are two motivations for this change.

1. It makes `Layer._slice_dims` simpler and more clearly similar to
`Layer.refresh`.
2. We use and test against the public API of the widgets and model
rather than rely on a private event.

The main downside of this change is that the layer control widget has
some extra state (`ndisplay`) that we need to keep in sync. Initially, I
hoped we could pass through the new value of `ndisplay` through events
(rather than store it as state), but things are complex enough in
`QtImageControls` to make that difficult.

This is mostly a refactor with no napari public API changes.

Partly motivated by #4795

- [x] all existing tests pass with my change
jni added a commit that referenced this issue Jun 22, 2023
@andy-sweet @kcpevey
@jni @pre-commit-ci @Carreau
Replace old async loading with new approach (#5816)
54bfba1 
# Fixes/Closes

Closes #5517
Closes #2790

# Description

This replaces [the old approach to asynchronous
loading](https://napari.org/0.4.16/guides/rendering.html) with [the new
approach described in
NAP-4](https://napari.org/stable/naps/4-async-slicing.html).

See the old approach (on `main`) on some remote 3D multi-resolution
images from Janelia via [@chili-chiu's fantastic bio sample data
plugin](https://www.napari-hub.org/plugins/napari-bio-sample-data):

https://github.com/napari/napari/assets/2608297/e8673d4e-c387-49bc-9254-30d0fc9c1cd2

And the new approach on the same data that is more responsive:

https://github.com/napari/napari/assets/2608297/0b9f1f12-d5e0-4980-be05-c5c58285c1a2

This PR also removes the old approach to tiled rendering (AKA octree)
since that is tied to the old approach to asynchronous loading. Most of
the code change delta consists of deleted files that are experimental
modules and tests associated with those old approaches.

The other changes remove the old integration and adjust the new one to
effectively achieve replacement. The key changes are as follows.

1. Remove async/sync_only marks from pytest since these should only be
needed for the old approach.
2. Remove command that hides/shows whether octree chunks are visible.
I'm unsure if these commands are publicly exposed anywhere, though this
one currently appears to be broken.
3. Redefine the existing experimental setting previously used to enable
the old approach to enable the new one instead.
4. Add support for `Layer.loaded` for the new approach by adding an ID
to each slice request. This could probably be moved to a separate PR if
desired (e.g. see andy-sweet#51)
5. Absorb slicing logic spread across old classes into the slice request
call.

This PR currently includes #5783, which should be reviewed and merged
first to make this large PR as small as possible. For the difference
between this and #5783, see andy-sweet#50

# References

I think this improves the simplicity of slicing logic as described in
#2156
Also see #5711 for the first version of this PR from @kcpevey, which was
split into this and #5783
Part of #4795

## Type of change

There probably need to be some documentation updates related to this
since the old approach is well documented on napari.org. We should
probably write some developer docs based off the async slicing NAP and
mark the old docs as deprecated and/or remove them to avoid confusion.

# How has this been tested?

- [x] added an integration test for checking `Layer.loaded` is updated
correctly
- [x] all existing tests pass with my change

I also did some manual testing by running `NAPARI_ASYNC=1 napari` and
interacting with some remote and non-remote data.

---------

Co-authored-by: Kim Pevey <kcpevey@gmail.com>
Co-authored-by: Kim Pevey <kcpevey@quansight.com>
Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Matthias Bussonnier <bussonniermatthias@gmail.com>
@andy-sweet andy-sweet mentioned this issue Jun 30, 2023
Merged
melissawm pushed a commit to melissawm/napari that referenced this issue Jul 3, 2023
@andy-sweet @kcpevey
@jni @pre-commit-ci @Carreau @melissawm
Replace old async loading with new approach (napari#5816)
7ddbb63 
# Fixes/Closes

Closes napari#5517
Closes napari#2790

# Description

This replaces [the old approach to asynchronous
loading](https://napari.org/0.4.16/guides/rendering.html) with [the new
approach described in
NAP-4](https://napari.org/stable/naps/4-async-slicing.html).

See the old approach (on `main`) on some remote 3D multi-resolution
images from Janelia via [@chili-chiu's fantastic bio sample data
plugin](https://www.napari-hub.org/plugins/napari-bio-sample-data):

https://github.com/napari/napari/assets/2608297/e8673d4e-c387-49bc-9254-30d0fc9c1cd2

And the new approach on the same data that is more responsive:

https://github.com/napari/napari/assets/2608297/0b9f1f12-d5e0-4980-be05-c5c58285c1a2

This PR also removes the old approach to tiled rendering (AKA octree)
since that is tied to the old approach to asynchronous loading. Most of
the code change delta consists of deleted files that are experimental
modules and tests associated with those old approaches.

The other changes remove the old integration and adjust the new one to
effectively achieve replacement. The key changes are as follows.

1. Remove async/sync_only marks from pytest since these should only be
needed for the old approach.
2. Remove command that hides/shows whether octree chunks are visible.
I'm unsure if these commands are publicly exposed anywhere, though this
one currently appears to be broken.
3. Redefine the existing experimental setting previously used to enable
the old approach to enable the new one instead.
4. Add support for `Layer.loaded` for the new approach by adding an ID
to each slice request. This could probably be moved to a separate PR if
desired (e.g. see andy-sweet#51)
5. Absorb slicing logic spread across old classes into the slice request
call.

This PR currently includes napari#5783, which should be reviewed and merged
first to make this large PR as small as possible. For the difference
between this and napari#5783, see andy-sweet#50

# References

I think this improves the simplicity of slicing logic as described in
napari#2156
Also see napari#5711 for the first version of this PR from @kcpevey, which was
split into this and napari#5783
Part of napari#4795

## Type of change

There probably need to be some documentation updates related to this
since the old approach is well documented on napari.org. We should
probably write some developer docs based off the async slicing NAP and
mark the old docs as deprecated and/or remove them to avoid confusion.

# How has this been tested?

- [x] added an integration test for checking `Layer.loaded` is updated
correctly
- [x] all existing tests pass with my change

I also did some manual testing by running `NAPARI_ASYNC=1 napari` and
interacting with some remote and non-remote data.

---------

Co-authored-by: Kim Pevey <kcpevey@gmail.com>
Co-authored-by: Kim Pevey <kcpevey@quansight.com>
Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Matthias Bussonnier <bussonniermatthias@gmail.com>
andy-sweet added a commit that referenced this issue Jul 6, 2023
@andy-sweet
Update layer's loaded state before vispy and thumbnail (#6026)
8640313 
# Description

This moves the update to `Layer.loaded` to be before rather than after
the updates to the vispy layer and qt thumbnail. I made that change for
a few reasons.

1. One of [the async slicing tests
flaked](https://github.com/napari/napari/actions/runs/5424665334/jobs/9864350221#step:8:306)
maybe because it was waiting on the vispy update and assumed that loaded
would also be updated.
2. Consistent with the previous definition of `Layer.loaded` with
regards to the old async.
3. The vispy layers still read the layer's slice state (after
`Layer.events.set_data`) has been emitted, and in [at least one case
that includes
`Layer.loaded`](https://github.com/napari/napari/blob/acddda2000ef6498731c4687cfb5ac60e378a68a/napari/_vispy/layers/labels.py#L11).

# References
Related to #4795
andy-sweet pushed a commit that referenced this issue Jul 14, 2023
@brisvag @melonora
@jni @Carreau
Use Request/Response pattern for Vectors layer (#5918)
929e30c 
# Description
Part of the async work (#4795), bring `Vectors` up to speed. Mostly
mirrors `Points`.

---------

Co-authored-by: Wouter-Michiel Vierdag <w-mv@hotmail.com>
Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Co-authored-by: Matthias Bussonnier <bussonniermatthias@gmail.com>
@andy-sweet andy-sweet mentioned this issue Jul 25, 2023
Merged
andy-sweet added a commit that referenced this issue Aug 18, 2023
@andy-sweet @Carreau
@pre-commit-ci @melonora @brisvag
A few small improvements to async slicing (#6080)
47b166c 
# Description
This contains a few small improvements and bug fixes related to the
currently experimental async slicing.

1. Only slice visible layers. This avoids unnecessary work when a layer
is not visible. When it changes to visible, it will be resliced.
2. Disconnect slice observers on shutdown. This removes the cyclic
dependency that may have been causing some of the teardown errors in
`test_async_slicing`.
3. Store weak references to layers. This helps to avoid unnecessary
memory consumption and also avoids post-slicing updates if the layer is
gone.

I grouped these together to avoid PR management overhead, but it's easy
to split them up if desired.

# References

Part of #4795

---------

Co-authored-by: Matthias Bussonnier <bussonniermatthias@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Wouter-Michiel Vierdag <w-mv@hotmail.com>
Co-authored-by: Lorenzo Gaifas <brisvag@gmail.com>
@andy-sweet andy-sweet mentioned this issue Aug 30, 2023
Merged
andy-sweet added a commit that referenced this issue Sep 5, 2023
@kcpevey @andy-sweet
Add test coverage for async slicing of labels (#5325)
16ea9d1 
# References and relevant issues
Part of #4795.

# Description
Adds test coverage to check that the layer slicer handles labels layers
appropriately.

---------

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
This was referenced Oct 23, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andy-sweet andy-sweet

Labels
Epic feature New feature or request vision
Projects
Async slicing
Status: Todo
napari global roadmap
Status: Late Progress
Milestone
No milestone
Development

No branches or pull requests
1 participant
@andy-sweet