Refactor Tk threading, UI dispatch, and deps #1177
Conversation
Add robust main-thread dispatch and guard for Tk access, coalesce UI updates, and relax pinned dependencies. Key changes: - pyproject.toml: loosen many exact pins (remove specific versions for tifffile, scipy, pandas, opencv-python, numpy, scikit-image, zarr, fsspec, h5py, requests) to allow installer to pick compatible versions. - src/navigate/tools/tk_thread_guard.py: new module that logs off-main-thread Tk calls and installs a best-effort guard on Tk methods. - src/navigate/controller/controller.py: large refactor - add typing annotations and return types, small docstring improvements - install_tk_thread_guard called during init - replace background Tk-pumping thread with an after-based event pump that drains a queue of cross-thread callbacks - add _run_on_main_thread, _drain_main_thread_dispatch_queue, and helpers to schedule/stop the pump - move many UI operations to run on the main thread (capture start/stop, display updates, finish UI updates) - improved lifecycle handling for event pump on shutdown - various minor API type changes and docstring updates - src/navigate/controller/sub_controllers/camera_view.py: replace dedicated display worker/queue with coalescing pending-image + after_idle flush to ensure display occurs on Tk thread and drop intermediate frames; rate-limiting and retry behavior preserved - src/navigate/controller/sub_controllers/histogram.py: switch histogram redraws to a coalesced after_idle model and keep only latest pending image; avoid spawning threads for histogram updates - src/navigate/controller/sub_controllers/multiposition.py and src/navigate/tools/multipos_table_tools.py: - replace deprecated DataFrame.append usage with pd.concat - suppress noisy pandas/pandastable FutureWarning about convert_dtype during table redraws - small API/typing and helper _refresh_table_view added Motivation: - Prevent crashes and subtle bugs caused by Tk calls from background threads by centralizing UI updates on the main Tk thread, and adding a guard that logs illegal access. - Reduce threading complexity by coalescing frequent UI updates (frames/histograms) to idle callbacks. - Address pandas deprecation warnings and modernize DataFrame appends. - Relax pinned dependencies to improve compatibility when installing/upgrading the project.
|
Quickly created a new environment on the BT-MesoSPIM @annie-xd-wang to test this out. I got the following warning: I'm not sure if this is because I updated the dependencies, or if it is because I created a new environment But the image display times now show much better performance, at about 5 ms. I think scheduling with tkinter off of the main loop historically has been a problem... The histogram is still bad, but now that the scheduling is avoided, I could introduce some of the other fixes I identified previously that reduced it down to ~5 ms. |
Add environment-aware guard to skip installing the Tk thread guard in certain environments. Introduces _guard_disabled_by_environment() which checks NAVIGATE_DISABLE_TK_THREAD_GUARD to outright disable, and skips installation under pytest unless NAVIGATE_ENABLE_TK_THREAD_GUARD_IN_TESTS is set. Also imports os and sys, logs when the guard is skipped, and returns False from install_tk_thread_guard when not installed. This avoids platform-specific Tcl thread errors in CI/test fixture runs while allowing explicit enablement.
Keep a strong reference to the real subprocess-backed model in the controller test fixture before replacing controller.model with a MagicMock to avoid ObjectInSubprocess finalization during setup. Store it as controller._real_model_for_cleanup and explicitly call terminate() on it in the fixture teardown (errors ignored) to ensure the subprocess-backed model is cleaned up properly.
Introduce update_rowcolors in multipos_table_tools to synchronize pandastable rowcolors with the table DataFrame and handle pandas v2 removal of DataFrame.append. The helper prefers calling table.update_rowcolors() if available, and falls back to adjusting rowcolors to match df indices and columns (adding/dropping columns and rows as needed). Update multiposition controller to import and call this helper instead of relying on table.update_rowcolors(), avoiding AttributeError and improving compatibility.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1177 +/- ##
===========================================
- Coverage 52.86% 52.83% -0.03%
===========================================
Files 197 198 +1
Lines 24032 24183 +151
===========================================
+ Hits 12704 12778 +74
- Misses 11328 11405 +77
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors Tk threading, UI dispatch, and dependency management to prevent crashes caused by off-main-thread Tk calls and improve installation flexibility. The changes centralize UI updates on the main Tk thread using an event-driven dispatch mechanism, replace background display threads with coalesced idle callbacks, modernize pandas DataFrame operations, and relax pinned dependencies.
Changes:
- Introduces a new Tk thread guard module that logs off-main-thread Tk access attempts for debugging
- Refactors controller.py to use a main-thread dispatch queue and event pump instead of background Tk-pumping threads
- Replaces dedicated display worker threads in camera_view.py and histogram.py with coalesced after_idle callbacks
- Updates pandas DataFrame.append() calls to pd.concat() to address pandas 2.x deprecation
- Relaxes exact version pins for multiple dependencies (tifffile, scipy, pandas, opencv-python, numpy, scikit-image, zarr, fsspec, h5py, requests)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/navigate/tools/tk_thread_guard.py | New module that wraps Tk methods to log off-main-thread access with sampling and stack traces |
| src/navigate/controller/controller.py | Major refactor: adds type annotations, replaces background thread with after-based event pump, implements _run_on_main_thread dispatcher, moves UI operations to main thread |
| src/navigate/controller/sub_controllers/camera_view.py | Replaces worker thread + queue with coalesced after_idle display updates |
| src/navigate/controller/sub_controllers/histogram.py | Switches to coalesced after_idle histogram updates, removes threading |
| src/navigate/controller/sub_controllers/multiposition.py | Adds _refresh_table_view helper, uses update_rowcolors from tools, suppresses pandas FutureWarnings |
| src/navigate/tools/multipos_table_tools.py | Replaces DataFrame.append with pd.concat, adds update_rowcolors function for pandas 2.x compatibility |
| test/controller/test_controller.py | Improves test cleanup by storing reference to real model before mocking and explicitly terminating it |
| pyproject.toml | Removes exact version pins for multiple dependencies to improve installation flexibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| camera_view_controller.try_to_display_image( | ||
| self._run_on_main_thread( | ||
| camera_view_controller.try_to_display_image, | ||
| image=data_buffer[image_id], |
There was a problem hiding this comment.
Missing wait parameter in asynchronous _run_on_main_thread call. The display_images function runs in a background thread (line 1650) and calls try_to_display_image via _run_on_main_thread without wait=True or wait=False. This call should likely be synchronous (wait=True) to ensure the image display completes before moving to the next image, similar to the pattern used at lines 1566-1570 and in the main capture_image method at line 1509.
| image=data_buffer[image_id], | |
| image=data_buffer[image_id], | |
| wait=True, |
| if self._event_pump_after_id: | ||
| try: | ||
| self.view.root.after_cancel(self._event_pump_after_id) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| if real_model is not None: | ||
| try: | ||
| real_model.terminate() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Best-effort cleanup in test teardown: ignore termination errors so they | |
| # do not mask the actual test outcome. |
2 participants
Add robust main-thread dispatch and guard for Tk access, coalesce UI updates, and relax pinned dependencies.
Key changes:
Motivation: