Open
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.
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.
Add developer guidance and documentation for the Controller threading/UI model (.codex and src/navigate/controller/README.md) and significantly expand unit test coverage for sub-controllers. New tests added for features_popup and plugins controllers; numerous tests extended/added for menus, multiposition, tiling, waveform_popup and controller behavior to cover edge cases, file I/O branches, UI interactions, and validation logic. These changes improve documentation of main-thread Tk rules and increase confidence in controller/sub-controller behavior via additional fixtures and mocked interactions.
Replace hardcoded "123.4" assertion with a captured expected_position from tiling_wizard_controller.stage_position_vars["x"].get() in test_position_handler_stops_stage_and_sets_widget. This makes the test assert the actual stored value rather than a brittle hardcoded string.
Update .github/workflows/push_checks.yaml to run tests across Python 3.9–3.13 and set strategy.fail-fast: false so all matrix jobs run even if one fails. The workflow continues to target windows-latest for the operating system.
Replace uses of deprecated/old tifffile.imsave with tifffile.imwrite across codebase (camera_map.py, tiff_data_source.py, volume_search.py) and adjust image writer import to alias tifffile.imwrite. Update docstring to reference imwrite. Fix unit test (test_menus.py) to skip when Tk is unavailable and only destroy root if it exists to avoid test failures in headless environments.
Make tests and PI stage code compatible with newer pipython by importing GCSError from pipython.pidevice.gcserror with a fallback to the older pipython import. Suppress pipython deprecation warnings via pytest.ini filterwarnings. Add a pytest_collection_modifyitems hook to skip tests marked as hardware on GitHub Actions by default (opt in with NAVIGATE_RUN_HARDWARE_TESTS=1). Also remove explicit @pytest.mark.hardware decorators from a couple of fixtures and update test imports accordingly.
Limit GitHub Actions matrix to Python 3.9/3.10 on Windows to speed up/check CI expectations. Unpin nidaqmx in pyproject to avoid strict version constraint. Fix escaping in configuration example path and clean up docstring escaping for readability. Update concurrency tests by renaming TestClass to DummyClass and replacing usages accordingly.
Tidy up multiple issues across the codebase: - tiff_data_source: Adjusted tiff write call to pass resolution as a numeric pair and the unit via resolutionunit="CENTIMETER" (matches tifffile API and avoids embedding the unit in the resolution tuple). - thorlabs/kcube_inertial.py & kcube_steppermotor.py: Use raw string literals for Windows DLL paths (r"C:\Program Files\..."), preventing accidental escape-sequence issues. - thorlabs/kcube_steppermotor.py: Added missing newline at end of file. - view/popups/autofocus_setting_popup.py: Initialize tk.BooleanVar with keyword value=False instead of a positional False for clarity and compatibility. These changes fix potential runtime errors and improve clarity/compatibility with library APIs.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes Python 3.9/3.10 compatibility and CI behavior while hardening Tkinter thread-safety and updating TIFF I/O to newer tifffile APIs, accompanied by expanded/adjusted test coverage (including CI-safe hardware/Tk handling).
Changes:
- Refactored controller/UI update flow to run Tk work on the main thread (dispatcher + coalesced display/histogram updates + optional Tk thread-guard).
- Updated TIFF writing calls to
tifffile.imwriteand addressed resolution metadata API changes. - Adjusted CI/test infrastructure: skip hardware tests on GitHub Actions by default, broadened CI Python matrix, and added/updated tests for controllers/sub-controllers.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/model/devices/stages/test_pi.py | Test import compatibility for pipython GCSError path. |
| test/model/devices/camera/test_hamamatsu.py | Removes fixture-level hardware mark (keeps class hardware marking). |
| test/model/devices/APIs/hamamatsu/test_hamamatsu_api.py | Removes fixture-level hardware mark (keeps class hardware marking). |
| test/model/concurrency/test_concurrency_tools.py | Renames helper class to avoid pytest collection warnings. |
| test/controller/test_controller.py | Adds tests for new controller main-thread dispatch/event pump logic; fixture cleanup hardening. |
| test/controller/sub_controllers/test_waveform_popup.py | Adds targeted unit tests for waveform popup controller behaviors. |
| test/controller/sub_controllers/test_tiling.py | Expands tiling wizard tests for more branch/edge coverage. |
| test/controller/sub_controllers/test_plugins.py | Adds tests for plugin controller install/load/uninstall branches. |
| test/controller/sub_controllers/test_multiposition.py | Expands multiposition controller tests (load/export/move/row ops/custom events). |
| test/controller/sub_controllers/test_menus.py | Makes Tk tests CI-robust and adds branch coverage for menu actions. |
| test/controller/sub_controllers/test_features_popup.py | Adds new tests for features popup/list graph logic and validation paths. |
| test/conftest.py | Skips hardware tests on GitHub Actions unless opted in via env var. |
| src/navigate/view/popups/autofocus_setting_popup.py | Fixes tk.BooleanVar initialization for Python 3.10. |
| src/navigate/tools/tk_thread_guard.py | Adds optional runtime guard logging off-main-thread Tk calls. |
| src/navigate/tools/multipos_table_tools.py | Replaces deprecated DataFrame.append usage; adds rowcolor sync helper and warning filtering. |
| src/navigate/model/features/volume_search.py | Switches debug TIFF output to tifffile.imwrite. |
| src/navigate/model/features/image_writer.py | Switches MIP TIFF writes to tifffile.imwrite (aliased). |
| src/navigate/model/features/autofocus.py | Fixes docstring escape sequences/formatting for newer Python warnings. |
| src/navigate/model/devices/stage/pi.py | Updates pipython GCSError import with fallback for older versions. |
| src/navigate/model/devices/APIs/thorlabs/kcube_steppermotor.py | Fixes invalid escape sequence in Windows DLL path string literal. |
| src/navigate/model/devices/APIs/thorlabs/kcube_inertial.py | Fixes invalid escape sequence in Windows DLL path string literal. |
| src/navigate/model/data_sources/tiff_data_source.py | Updates TIFF resolution metadata usage to match newer tifffile API. |
| src/navigate/controller/sub_controllers/multiposition.py | Removes pandas-append usage; adds redraw warning filtering; uses new rowcolor sync helper. |
| src/navigate/controller/sub_controllers/histogram.py | Moves histogram updates onto Tk thread via coalesced after_idle scheduling. |
| src/navigate/controller/sub_controllers/camera_view.py | Replaces background display thread+queue with coalesced Tk-thread after_idle rendering. |
| src/navigate/controller/sub_controllers/camera_map.py | Updates TIFF export calls to tifffile.imwrite. |
| src/navigate/controller/controller.py | Introduces Tk main-thread dispatcher/event pump; routes capture UI updates through main thread; installs thread guard. |
| src/navigate/controller/README.md | Documents controller architecture and new threading/event pump model. |
| src/navigate/config/configuration_database.py | Fixes invalid escape sequences in example paths. |
| pytest.ini | Adds warning filters for pipython deprecations. |
| pyproject.toml | Unpins several dependencies; updates requirements to support broader compatibility. |
| .github/workflows/push_checks.yaml | Expands CI matrix to Python 3.9 + 3.10; sets fail-fast: false. |
| .codex | Adds repo guidance emphasizing main-thread-only Tk access and controller dispatch pattern. |
Comments suppressed due to low confidence (2)
pyproject.toml:42
- These dependencies were changed from pinned versions to fully unpinned (e.g.,
pandas,scipy,tifffile,nidaqmx). Givenrequires-python = ">=3.9.7"and the tight coupling withpandastable==0.12.2.post1, leaving them unbounded increases the risk of future resolver picks breaking runtime (or CI) as upstream drops Python 3.9/changes APIs. Recommend adding upper bounds (or known-good version ranges) at least for pandas/scipy/tifffile to preserve the intended 3.9/3.10 compatibility envelope.
requires-python = ">=3.9.7"
dependencies = [
'matplotlib-inline==0.1.3',
'PyYAML==6.0',
'pyserial==3.5',
'PIPython==2.6.0.1',
'nidaqmx',
'tifffile',
'scipy',
'pyusb==1.2.1',
'pandas',
'pandastable==0.12.2.post1',
'opencv-python',
'numpy; sys_platform != "darwin"',
'numpy; sys_platform == "darwin"',
'scikit-image',
'zarr',
'fsspec; sys_platform != "darwin"',
'fsspec; sys_platform == "darwin"',
'h5py',
'requests',
test/controller/test_controller.py:170
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1179 +/- ##
===========================================
+ Coverage 54.22% 56.28% +2.06%
===========================================
Files 192 192
Lines 24757 24823 +66
===========================================
+ Hits 13424 13972 +548
+ Misses 11333 10851 -482
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:
|
Change made: Replaced strict equality with: expected_focus = dummy_microscope.central_focus + float(channel_dict["defocus"]) assert ... == pytest.approx(expected_focus, abs=1e-2) Why this helps: get_stage_position() rounds to 2 decimals, while direct float math can keep tiny binary residue, causing random CI mismatches.
Import tkinter and introduce a module-scoped `controller_root` fixture that creates an isolated `tk.Tk()` root and performs safe cleanup (update_idletasks/update/destroy) to avoid shared session root side effects across tests. Update the existing `controller` fixture to consume `controller_root` (passing it as the Controller `root`), ensuring tests use the isolated root instance.
Make stack acquisition image loading robust: load cubic_bottom_to_top.png with error handling and check for empty PhotoImage handles, logging warnings and falling back to a text label if loading or attaching the image fails. Adds a unit test that monkeypatches PhotoImage to simulate an empty handle and verifies the fallback behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
Stabilized Python compatibility around 3.9.7 and 3.10.11 and aligned CI to those versions.
Fixed Tk variable initialization for Python 3.10 in autofocus_setting_popup.py by replacing positional tk.BooleanVar(False) with tk.BooleanVar(value=False).
Unpinned NI-DAQ dependency in pyproject.toml:
from nidaqmx==0.5.7 to nidaqmx.
Reduced workflow matrix in push_checks.yaml to:
python-version: ["3.9", "3.10"].
Test Infrastructure and Reliability
Skipped hardware tests on GitHub Actions by default via conftest.py with override env NAVIGATE_RUN_HARDWARE_TESTS=1.
Removed deprecated fixture-level marks causing PytestRemovedIn9Warning:
test_hamamatsu_api.py
test_hamamatsu.py
Made Tk-based menu tests robust on CI environments missing Tcl/Tk install:
test_menus.py.
Fixed pytest collection warning by renaming helper class:
test_concurrency_tools.py (TestClass -> DummyClass).
Warning Cleanup
Fixed invalid escape sequence warnings:
Added new tests for features popup logic:
Confirmed requested tests run in both environments (dep_update and python310) where applicable.
Key physical-machine validation targets for this PR:
NI filter wheel with current nidaqmx (un-pinned)
Files: pyproject.toml, ni.py
TIFF writing during acquisition (OME + non-OME)
Stage hardware integrations still behave normally