Repository navigation
Gradual Dependency Update - #1179
Merged
Merged
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 67.19% 68.71% +1.52%
===========================================
Files 210 211 +1
Lines 31074 31249 +175
===========================================
+ Hits 20879 21472 +593
+ Misses 10195 9777 -418
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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.
Retain TIFF metadata compatibility across legacy and current tifffile APIs, restore safe live channel and multiposition behavior, and preserve dataframe schemas. Restore current-develop waveform test isolation and advanced-popup coverage, correct the Windows resolution audit command, and narrow PIPython warning suppression.
Collaborator
Author
Recommended follow-up: Python 3.13After this PR merges, I recommend a separate follow-up PR that applies the same constrained, non-blocking approach to Python 3.13. Current readiness checks are encouraging:
Suggested implementation:
One caveat: this establishes compatibility for the normal production/development environment, not every optional hardware integration. The Photometrics/PyVCAM path looks ready, but the optional Kinesis/pylablib graph resolves through old Numba/llvmlite releases and should receive a separate real-install check. Longer term, once 3.12 is required and production deployments have migrated, we should set a date to retire the already-EOL Python 3.9 lane rather than permanently maintaining five Python lanes. References: Python 3.13 release schedule, PyVCAM package files. |
Collaborator
Author
|
Tested on our Benchtop MesoSPIM. No errors were identified during testing, and this PR is ready for review. |
Bring PR #1179 through origin/develop d267b41 without rewriting its existing history.\n\nConflict resolution:\n- retain the current WaveKit-backed Imagine Optics connection contract\n- combine dark-theme stack artwork selection with safe Tk image fallback\n- preserve current feature-popup and multiposition coverage from both branches\n- remove two superseded feature-list persistence assertions and adapt retained UI tests to the current controller state\n\nVerification:\n- 91 conflict-adjacent tests passed; 1 skipped\n- Black and git diff checks passed on resolved files
Goal:\nKeep controller threading guidance discoverable without maintaining duplicate or transient agent artifacts.\n\nRoot cause:\nThe PR added a plain .codex file that Codex does not discover by default, while repository-wide instructions lived under .github and included stale dependency pins. The Windows implementation plans and design notes also contained session-specific paths and execution details that were not durable project documentation.\n\nChanges:\n- add concise repository-wide guidance in root AGENTS.md\n- retain controller/README.md as the canonical architecture and threading reference\n- refresh the README startup sequence and autofocus/thread-pool details against current develop\n- remove the redundant .codex file and misplaced .github/AGENTS.md\n- remove the four PR-specific Superpowers plan/design artifacts\n\nVerification:\n- cross-checked documented controller symbols and startup order against controller.py\n- confirmed no local paths or Superpowers directives remain in the retained guidance\n- git diff --check passed
Goal: leave the updated PR head compliant with the repository's pinned pre-commit hooks after merging develop. Root cause: several files changed by the PR predated or bypassed the pinned Black and Ruff checks, leaving formatting drift, long lines, two unresolved forward-reference annotations, an unused pixel-size lookup, and an unnecessary f-string. Changes: apply the pinned Black formatter to all affected PR files; make the lint-only source adjustments without changing behavior; and retain the merged test coverage. Verification: pre-commit passed for every file changed relative to develop; git diff --check passed; 606 focused controller, compatibility, data-source, feature, stage, view, and theme tests passed with 2 skipped.
annie-xd-wang
approved these changes
Oct 8, 2026
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