feat: use another thread to write to mda zarr #258

ianhi · 2023-02-24T21:54:36Z

Fixes: #256

Untested because i'm not yet sure this is the fully correct way to do this. Would be great to get some eyes on this from @tlambert03 and @fdrgsp

This tries to do as much of the work of processing an MDA frame in a third thread as possible. I also wanted to ensure that the most recently captured image is the most recently displayed, so this end deque for its LIFO behavior and because it is higher performance than queue (https://bugs.python.org/issue15329#msg199368).

Then when the worker thread has a chance it processes all the previous indices that it didn't get a chance to deal with before a new image came in. This is the source of the complexity of checking _largest_idx

To user test:

Open napari-micromanager
Load the demo config
Set up MDA with one channel - 100 time points at 0 interval
run - check that gui is functional

ianhi · 2023-02-24T21:54:58Z

src/napari_micromanager/_mda_handler.py

        if (meta := sequence.metadata.get(SEQUENCE_META_KEY)) is not None:
+            # TODO: make sure io thread finishes before saving
+            # thread workers don't seem to have .join method?
+            # self._io_t.join()


@tlambert03 am I missing something here?

no, QRunnables are slightly more restricted than true QThreads. They don't have a join()... but the worker objects do have an is_running() method that you could wait on. They also emit a finished signal that you could connect to a new method that does the saving.

create_worker(self._watch_mda, _start_thread=True, _connect={'finished': self._on_io_finished})

Is there an advantage to QRunnables that comes with the restrictions? I'm not sure I fully grasp the breakdown of thread vs runnable

ianhi · 2023-02-24T21:55:49Z

src/napari_micromanager/_mda_handler.py

+                pass
+        import time
+
+        time.sleep(0.5)


todo: Make a real choice for this number. I chose this based on what was convenient for development. Ideally it would be also be adjustable - open to suggestions.

src/napari_micromanager/_mda_handler.py

ianhi · 2023-02-24T21:56:26Z

src/napari_micromanager/_mda_handler.py

+            except IndexError:
+                pass


maybe sleep for an extra long time here?

yeah I think something like this would work. doesn't need to be an extra long sleep, i think 100ms-200ms is a reasonable starting point (5-10 checks a second is no biggie). Also, add a yield to turn your worker into a Generator Worker (These are easier to kill from the main thread if need be, since each time they yield is an opportunity to communicate with it)

def _watch_mda(self) -> Generator[None, None, None]: while self._mda_running: if self._deck: self._process_frame(*self._deck.pop()) else: time.sleep(0.1) yield

src/napari_micromanager/_mda_handler.py

tlambert03

awesome!! it's way better 🤩

happy to go with this and use it for a while,

tlambert03 · 2023-02-27T18:50:49Z

src/napari_micromanager/_mda_handler.py

@@ -72,6 +72,7 @@ def __init__(self, mmcore: CMMCorePlus, viewer: napari.viewer.Viewer) -> None:

        # mapping of id -> (zarr.Array, temporary directory) for each layer created
        self._tmp_arrays: dict[str, tuple[zarr.Array, tempfile.TemporaryDirectory]] = {}
+        self._deck: deque = deque()


Suggested change

self._deck: deque = deque()

self._deck: Deque[tuple[np.ndarray, MDAEvent]] = Deque()

... and then change above to from typing import Deque

good call. Also for the future we should go to just collections.deque (at least once napari-micro is python 3.9+ only)

https://docs.python.org/3/library/typing.html#typing.Deque

Deprecated since version 3.9: collections.deque now supports subscripting ([]). See PEP 585 and Generic Alias Type.

Yep, all the generic collections can be updated then. 3.8 end of life is Oct 2024

tlambert03 · 2023-02-27T18:59:43Z

src/napari_micromanager/_mda_handler.py

+            except IndexError:
+                pass


yeah I think something like this would work. doesn't need to be an extra long sleep, i think 100ms-200ms is a reasonable starting point (5-10 checks a second is no biggie). Also, add a yield to turn your worker into a Generator Worker (These are easier to kill from the main thread if need be, since each time they yield is an opportunity to communicate with it)

def _watch_mda(self) -> Generator[None, None, None]: while self._mda_running: if self._deck: self._process_frame(*self._deck.pop()) else: time.sleep(0.1) yield

tlambert03 · 2023-02-27T19:03:44Z

src/napari_micromanager/_mda_handler.py

+            cs = list(self.viewer.dims.current_step)
+            for a, v in enumerate(im_idx):
+                cs[a] = v
+            self._update_viewer_dims(cs, layer_name, event)


slightly wary of accessing self.viewer from a thread. might want some locks in here. Also, I wonder it would be possible for the thing triggering _update_viewer_dims to not be _process_frame

one option here is to connect a method to the yielded event that self._io_t will emit (provided you make _watch_mda a generator). Then you could yield a (cs, layer_name, event) tuple back to the main thread.

I don't know though... it's possible that ensure_main_thread is equally effective and efficient here. But that would be a different option if we ever need to try it out

How about just moving that access in to the function that has ensure_main_thread?

sure, and then it would just be self._update_viewer_dims(layer_name, event)? if that works too, I like that

tlambert03 · 2023-02-27T19:17:41Z

src/napari_micromanager/_mda_handler.py

        if (meta := sequence.metadata.get(SEQUENCE_META_KEY)) is not None:
+            # TODO: make sure io thread finishes before saving
+            # thread workers don't seem to have .join method?
+            # self._io_t.join()


no, QRunnables are slightly more restricted than true QThreads. They don't have a join()... but the worker objects do have an is_running() method that you could wait on. They also emit a finished signal that you could connect to a new method that does the saving.

create_worker(self._watch_mda, _start_thread=True, _connect={'finished': self._on_io_finished})

ianhi · 2023-02-27T19:28:37Z

Something seems to have gone very wrong in the explorer tests - they now hang forever :(

ianhi · 2023-07-26T18:56:40Z

working on rebasing. but running into what seem like some issues in pymmcore-widgets:

    def _enable_run_btn(self) -> None:
        """Enable run button.
    
        ...if there is a channel group and a preset selected or the channel checkbox
        is checked and there is at least one channel selected.
        """
        if self._mmc.getChannelGroup() and self._mmc.getCurrentConfig(
            self._mmc.getChannelGroup()
        ):
            if self.ch_cbox.isChecked() and not self.channel_widget._table.rowCount():
                self.buttons_wdg.run_button.setEnabled(False)
            else:
                self.buttons_wdg.run_button.setEnabled(True)
>       elif not self.ch_cbox.isChecked() or not self.channel_widget._table.rowCount():
E       AttributeError: 'QWidget' object has no attribute 'isChecked'

attn @fdrgsp

ianhi · 2023-07-28T18:47:33Z

@tlambert03 I believe I addressed all review comments - this should be ok.

I went ahead with this because it was pretty close and is more self contained than #264 which has several unrelated changes making it trickier to parse through. If this one does get merged then I would very much support a follow up PR with performance enhancements (as isolated changes ideally).

tlambert03 · 2023-07-28T22:42:02Z

trying to get tests to run

fdrgsp · 2023-07-28T23:43:20Z

tests/test_layer_scale.py

I would make sure we are not saving the files in this test, I would switch from

sequence = mda_sequence_splits.replace(axis_order=axis_order)

to

sequence = mda_sequence_splits.replace( axis_order=axis_order, metadata={SEQUENCE_META_KEY: SequenceMeta(should_save=False)}, )

fdrgsp · 2023-07-28T23:43:38Z

#264 which has several unrelated changes making it trickier to parse through.

@ianhi yes that's true sorry about that:). Maybe you can ignore everything else and just have a look at the tests and the _mda_handler.py which are the only things that are important for this PR. I did test it on the microscope and works quite well without freezing the gui.

one think I found useful and I would add is to specifying the chunk size when creating the zarr since it improves the image assignment when they are popped from the _deck (https://github.com/fdrgsp/napari-micromanager/blob/3256672406344d387f8151fcfbc4b1214e96fd19/src/napari_micromanager/_mda_handler.py#L128)

I also added a comment about tests that might be relevant tests/test_layer_scale.py.

Anyway I think this PR is ok, I did not tested it on a real microscope yet but It's close to #264 which I did test.

tlambert03 · 2023-09-14T18:25:35Z

does #272 close this?

ianhi · 2023-09-14T20:39:21Z

superseded by #272

ianhi commented Feb 24, 2023

View reviewed changes

src/napari_micromanager/_mda_handler.py Outdated Show resolved Hide resolved

ianhi mentioned this pull request Feb 27, 2023

Should MDARunner manage an io thread for usage by anything writing to disk? pymmcore-plus/pymmcore-plus#189

Closed

tlambert03 approved these changes Feb 27, 2023

View reviewed changes

tlambert03 changed the title ~~ENH: use another thread to write to mda zarr~~ feat: use another thread to write to mda zarr Jul 10, 2023

fdrgsp mentioned this pull request Jul 21, 2023

feat: Update thread to write to mda zarr #264

Closed

ianhi mentioned this pull request Jul 26, 2023

fix: remove defunct sample explorer #266

Merged

ianhi closed this Jul 28, 2023

ianhi reopened this Jul 28, 2023

ianhi added 3 commits July 28, 2023 13:11

ENH: use another thread to write to mda zarr

27678b5

remove unncessary comment

bb29da2

remove debug print

2760982

ianhi force-pushed the io-thread branch from e12ee8c to 2760982 Compare July 28, 2023 17:17

ianhi added 3 commits July 28, 2023 14:33

enh: address review comments

b21e55e

chore: remove more explorer stuff

2d395f9

chore: fix typing

f441b2d

ianhi force-pushed the io-thread branch from b21a74a to f441b2d Compare July 28, 2023 18:41

chore: add note about saving

36c16f6

tlambert03 requested a review from fdrgsp July 28, 2023 19:34

tlambert03 closed this Jul 28, 2023

tlambert03 reopened this Jul 28, 2023

fdrgsp reviewed Jul 28, 2023

View reviewed changes

fdrgsp and others added 2 commits August 4, 2023 14:10

test: update

49bba2f

Merge branch 'main' into io-thread

526e77b

fdrgsp mentioned this pull request Aug 4, 2023

feat: Update thread to write to mda zarr #272

Merged

ianhi closed this Sep 14, 2023

ianhi deleted the io-thread branch September 14, 2023 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use another thread to write to mda zarr #258

feat: use another thread to write to mda zarr #258

ianhi commented Feb 24, 2023 •

edited

Loading

ianhi Feb 24, 2023

tlambert03 Feb 27, 2023 •

edited

Loading

ianhi Feb 27, 2023

ianhi Feb 24, 2023

ianhi Feb 24, 2023

tlambert03 Feb 27, 2023

tlambert03 left a comment

tlambert03 Feb 27, 2023

ianhi Feb 27, 2023

tlambert03 Feb 27, 2023

tlambert03 Feb 27, 2023

tlambert03 Feb 27, 2023

tlambert03 Feb 27, 2023

ianhi Feb 27, 2023

tlambert03 Feb 27, 2023

tlambert03 Feb 27, 2023 •

edited

Loading

ianhi commented Feb 27, 2023

ianhi commented Jul 26, 2023

ianhi commented Jul 28, 2023

tlambert03 commented Jul 28, 2023

fdrgsp Jul 28, 2023

fdrgsp commented Jul 28, 2023 •

edited

Loading

tlambert03 commented Sep 14, 2023

ianhi commented Sep 14, 2023

	self._deck: deque = deque()
	self._deck: Deque[tuple[np.ndarray, MDAEvent]] = Deque()

feat: use another thread to write to mda zarr #258

feat: use another thread to write to mda zarr #258

Conversation

ianhi commented Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

tlambert03 Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlambert03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlambert03 Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

ianhi commented Feb 27, 2023

ianhi commented Jul 26, 2023

ianhi commented Jul 28, 2023

tlambert03 commented Jul 28, 2023

Choose a reason for hiding this comment

fdrgsp commented Jul 28, 2023 • edited Loading

tlambert03 commented Sep 14, 2023

ianhi commented Sep 14, 2023

ianhi commented Feb 24, 2023 •

edited

Loading

tlambert03 Feb 27, 2023 •

edited

Loading

tlambert03 Feb 27, 2023 •

edited

Loading

fdrgsp commented Jul 28, 2023 •

edited

Loading