Migrate to PyQt6 and run SAM/DINO inference in-process#4
Merged
Conversation
Validates the central hypothesis of the upcoming subprocess-removal work: that PyQt6 sidesteps the WinError 1114 DLL load-order conflict on Windows + Python 3.14 that motivated sam_worker.py / dino_worker.py (see ADR-011). Run manually before deleting any worker code. Exit code 0 unblocks Phase 2 of the PyQt5 -> PyQt6 + in-process inference migration. https://claude.ai/code/session_01ADoBX5VmUYpCrwbkecKMHL
End-to-end migration of the GUI binding. 34 files, ~360 lines changed. All 65 tests still pass on PyQt6 6.11; the full app constructs and renders headlessly via QT_QPA_PLATFORM=offscreen. What changed ------------ - Dependency pins: PyQt5>=5.15 -> PyQt6>=6.7 (requirements.txt, setup.py) - Bulk import rewrite: `from PyQt5...` -> `from PyQt6...` (28 files) - Symbol relocations: * QAction moved from QtWidgets to QtGui (annotator_window.py) * QDesktopWidget removed -> QGuiApplication.primaryScreen() (snake_game.py) - Enum namespacing (Qt6 requires fully-qualified names everywhere): * Qt.AlignmentFlag / MouseButton / KeyboardModifier / Key * Qt.PenStyle / BrushStyle / CursorShape / GlobalColor * Qt.WindowType / WindowModality / FocusPolicy / TransformationMode * Qt.ItemDataRole / ItemFlag / ContextMenuPolicy / ScrollBarPolicy * Qt.TextFormat / TextInteractionFlag / MatchFlag / CheckState * QMessageBox.StandardButton / .Icon / .ButtonRole * QDialog.DialogCode, QFileDialog.AcceptMode / FileMode / Option * QAbstractItemView.SelectionMode / SelectionBehavior / EditTrigger * QHeaderView.ResizeMode, QSlider.TickPosition * QPainter.RenderHint, QImage.Format.Format_* * QDialogButtonBox.StandardButton / .ButtonRole * QKeySequence.StandardKey - Modern event API in image_label.py: event.pos()/.x()/.y() -> event.position() returning QPointF end-to-end. Scrollbar setValue() takes int() of the QPointF delta (the boundary). - Removed dead workaround in annotator_window.py: clearing WindowContextHelpButtonHint from dialog flags. Qt6 already suppresses this; the flag itself was removed. - exec_() -> exec() in main.py entry point. - CI: add libegl1/libgl1 to the Linux apt-install list (Qt6 needs them). - Docs: CLAUDE.md, README.md, docs/02_architecture_constraints.md updated to reflect PyQt6 and the relaxed Linux support story. Not touched ----------- - sam_worker.py, dino_worker.py, sam_utils.py, dino_utils.py subprocess pattern (Phase 2, gated on Win+Py3.14 validation). - exec_() call sites outside main.py (still work as deprecated alias). https://claude.ai/code/session_01ADoBX5VmUYpCrwbkecKMHL
… wrapping
Phase 2 of the PyQt6 migration. The DLL conflict that motivated the
subprocess workers (ADR-011) no longer applies under PyQt6, so the
~1-2 s subprocess spawn per inference is gone, and models stay cached
across calls.
What changed
------------
- Deleted sam_worker.py (288 lines), dino_worker.py (231 lines),
tools/check_worker_isolation.py (134 lines).
- Rewrote sam_utils.py end-to-end:
* SAMUtils inherits QObject, caches the Ultralytics model in self._model
* change_sam_model() loads eagerly (on worker thread, UI stays alive)
* apply_sam_points / apply_sam_prediction / apply_sam_predictions_batch
all run inference on a QThread via the new _run_sync helper
* Lazy import of torch/ultralytics keeps app startup snappy
* Added unload() for future Tools-menu memory release
- Rewrote dino_utils.py with the same pattern (DINOUtils as QObject,
model cached across calls, transformers lazy-imported).
- Added _run_sync: spawns a QThread, pumps the calling thread's
QEventLoop until done. Public API stays synchronous so the existing
call sites in annotator_window.py work unchanged. UI events (timers,
redraws, progress dialog cancels) keep flowing during the wait.
- Removed the stale "If you are on Python 3.14, PyTorch may not yet be
fully supported" message in change_sam_model error path.
- utils.py docstring: drop sam_worker reference.
Docs
----
- ADR-011 marked Superseded, with pointer to ADR-013.
- New ADR-013 documents the in-process + QThread decision, the latency
win, and the trade-offs (re-entrancy via QEventLoop pump, no more
crash isolation).
- arc42 docs/05_building_block_view.md, docs/06_runtime_view.md,
docs/12_glossary.md updated to drop subprocess wording.
- CLAUDE.md SAM Integration section rewritten.
Verification
------------
- 65 tests pass (pytest, QT_QPA_PLATFORM=offscreen).
- Full app constructs and renders headlessly.
- _run_sync round-trip verified end-to-end against a 0.3 s sleep.
- Phase 0 PyQt6+torch+transformers+ultralytics coexistence smoke test
passes on Linux+Py3.11. Windows+Py3.14 verification is the user's
responsibility before this PR ships.
https://claude.ai/code/session_01ADoBX5VmUYpCrwbkecKMHL
The quality gate is blocking by design — the next steps (address P0s, push, open PR) depend on its findings. Backgrounding it just defers the work and risks shipping unreviewed code. https://claude.ai/code/session_01ADoBX5VmUYpCrwbkecKMHL
P0 — correctness ---------------- - _InferenceThread.run no longer swallows exceptions. Stores them on the thread instance; _run_sync re-raises on the calling thread. Silent model-load failures previously showed up as "No mask matches" / "No detections" dialogs with no way to diagnose. (sam_utils.py) - Added _inference_in_flight module flag + InferenceBusyError. The earlier QMutex draft would have deadlocked: same-thread re-acquisition of a non-recursive mutex hangs, and a recursive mutex would defeat the whole serialization point. A flag with an explicit exception surfaces re-entry instead of corrupting the model with concurrent .forward() calls. (sam_utils.py) - Added _sam_inference_in_flight guard in annotator_window.apply_sam_prediction — the SAM debounce timer can fire while a previous inference is pumping inside _run_sync; the guard skips the re-entrant call so the next click + debounce restart issues a fresh inference with the up-to-date point set. P1 — should-fix --------------- - dino_utils._detect_blocking no longer shuffles the model CPU<->GPU on every call. Moving a 1.9 GB DINO base over PCIe was wiping out the in-process caching gain the whole PR was meant to deliver. - change_sam_model now flips current_sam_model AFTER successful load, not before. On load failure the state stays consistent. - _qimage_to_numpy always returns a fresh copy. The fallback path was particularly broken: the converted QImage was local, would go out of scope at return, and the worker thread's numpy buffer would have aliased freed memory. - Updated misleading dino_utils.detect comment about marshaling — the safety actually comes from the .copy() inside _qimage_to_numpy, not from where it runs. - ADR-001 marked Superseded with pointer to new ADR-014. - New ADR-014 documents the PyQt5->PyQt6 migration decision. - Updated ADR-013 consequences to honestly describe the re-entrancy guards (replaced the "acceptable for now; revisit if users hit it" hand-wave the reviewer specifically called out). - docs/01_introduction_and_goals.md, docs/05_building_block_view.md (ASCII diagram), docs/06_runtime_view.md (app.exec_() typo) all updated for PyQt6. - CLAUDE.md: Testing Checklist no longer references deleted sam_worker /dino_worker; senior-reviewer agent prompt no longer references deleted check_worker_isolation.py. - .claude/agents/senior-reviewer.md retargeted from PyQt5 -> PyQt6 and rewritten to check ADR-013 re-entrancy guards instead of ADR-011's retired subprocess isolation. P2 — opportunistic ------------------ - tools/check_pyqt6_torch_coexistence.py now constructs a QApplication after importing torch. Pure import alone does not load Qt's native platform plugin (qwindows.dll on Windows) — which is the actual site of the historical WinError 1114. The previous green result was a false positive on the strictest test. - CI matrix gains Python 3.13. ADR-013 claims PyQt6+torch coexist on modern Pythons; this adds CI evidence (Py3.14 still manual via the coexistence script — pip wheels not yet broadly available). - CI apt-install list deduped (libxcb-cursor0 was listed twice). Verification ------------ - 65 tests pass. - Smoke-tested both fixes: exception propagation works (boom() raises ValueError out of _run_sync); re-entry detection works (timer-driven inner call raises InferenceBusyError while outer is pumping). - coexistence script with QApplication construction passes end-to-end on Linux+Py3.11. https://claude.ai/code/session_01ADoBX5VmUYpCrwbkecKMHL
P0 — correctness (regression unblocked by the first round's fix) ---------------------------------------------------------------- - annotator_window.py: import traceback at module level. The except block at the DINO call site (line 3022 pre-edit) referenced traceback.print_exc() without the module being importable in scope. Before the previous fix dino_utils.detect() returned None on error so the except was rarely entered; now it raises for real, so the NameError was about to start firing and leave the detect buttons permanently disabled with no user-visible dialog. P1 — should-fix --------------- - annotator_window.apply_sam_prediction now catches inference exceptions. The slot is driven by a QTimer; before this patch a CUDA OOM or InferenceBusyError would fall out into PyQt6's default unhandled-slot handler (stderr only). InferenceBusyError is suppressed silently (defense-in-depth alongside the call-site flag); other exceptions show a critical QMessageBox. - Same wrapping added to the unprotected SAM-batch calls inside both DINO flows (single image at line 3063, per-image loop at 3170). - Wired SAMUtils.unload() and DINOUtils.unload() to a new Tools menu entry "Unload AI Models (Free GPU Memory)". The DINO CPU<->GPU shuffle was removed in the previous round, which removes the automatic between-call free; this gives users on constrained GPUs a manual recovery path. - Bumped version 0.8.12 -> 0.9.0 in setup.py and __init__.py to signal the binding change (PyQt5 -> PyQt6) and the in-process inference rework. Anyone reading the wheel changelog now sees the binding switch in the version. - docs/05_building_block_view.md SAMUtils block rewritten to match the actual class shape (sam_model -> _model, qimage_to_numpy is a module-level helper not a method, _run_sync added). - Deleted PYTHON314_SETUP.md — it described the migration as future work, in the present tense, with the now-retired DLL workaround as a known issue. Easier to delete than keep coherent. P2 — cleanup ------------ - Dropped the unused `import traceback` in sam_utils.py (_InferenceThread captures exceptions on the instance now; no printing inside the worker). - The "No mask generated." batch fallback now builds a fresh dict per bbox via list comprehension instead of `[d] * N` (avoided shared-reference footgun). - Removed the dead `qimage_to_numpy` method on ImageAnnotator — module-level `_qimage_to_numpy` in sam_utils superseded it. - Folded the local `import traceback` inside `add_class`'s except block into the module-level import. Architectural belt-and-braces ----------------------------- - Added an assert at the top of `_run_sync`: the function MUST be called from the GUI thread. The `_inference_in_flight` flag is a plain global, not protected against cross-thread access — if a future contributor drives inference from a worker thread it becomes a true race. The assert is the tripwire. Reviewer flagged this as the kind of constraint that gets violated six months later when nobody remembers the design. Verification ------------ - 65 tests still pass. - Exception propagation and re-entry detection both re-tested in the full-app context — outer call returns 'done', timer-driven inner call raises InferenceBusyError, both as designed. - App constructs and renders headlessly. https://claude.ai/code/session_01ADoBX5VmUYpCrwbkecKMHL
P1 -- - TESTING.md: removed the "Known Issues — Python 3.14 + PyTorch Compatibility" section (the WinError 1114 it described is gone with the PyQt6 migration), removed the "Milestone 1.2: PyQt6 Migration" future-work entry (the migration is done), and added a brief "Headless Testing" section pointing at the CI deps list. Also bumped the CI Python row to mention 3.13. The file was not touched by earlier commits in this branch; the reviewer correctly pointed out that the branch is what made it wrong, so it's owed. P2 -- - Replaced the GUI-thread tripwire in sam_utils._run_sync with an explicit `if ...: raise RuntimeError(...)` instead of `assert`. `python -O` strips asserts; the tripwire was the kind of thing that would only matter once it had silently disappeared. Verification: 65 tests still pass. App still constructs. https://claude.ai/code/session_01ADoBX5VmUYpCrwbkecKMHL
Closes user-reported regressions and rough edges discovered during Windows manual testing of PR #4. Covers crashes, UX bugs, and silent failures that the 65-test pytest-qt suite doesn't exercise. PyQt5 → PyQt6 mechanical migration gaps: - 15× .exec_() → .exec() across annotator_window, dino_merge_dialog, image_patcher, project_search, snake_game, stack_to_slices. The QMenu crash on right-click in the class list (annotator_window:4607) was the first user-visible casualty. - Missing `import traceback` in dino_merge_dialog.py. - F2 (Snake game) moved from keyPressEvent to QShortcut(ApplicationShortcut) so QTableWidget's in-cell-edit doesn't swallow it. Canvas — pan + zoom-to-cursor: - Pan now uses event.globalPosition() so the widget shifting under the cursor mid-drag doesn't absorb half the delta (former half-speed pan). - New cursor-anchored Ctrl+wheel zoom; post-zoom offset derived analytically from viewport().width() instead of the stale self.width() that's wrong on zoom-out before layout settles. DINO panel + detection: - Threshold column widths (88 px fixed) + setFrame(True) so values "0,25" / "0,50" are readable. - PhraseEditorPanel auto-reveals on class-add; row-0 phrase is now renamable. Removed the silent class-name re-prepend in get_phrases_for + _run_for_class so a renamed row-0 actually reaches DINO. - Auto-accept dropdown now honored by both single + batch paths. - "Detect All Images" extended to multi-dim image slices via _collect_dino_batch_work_items (was silently skipping stacks). - New _navigate_to_image_or_slice handles slice names in batch review; orphan results are popped instead of leaving a half-state. - temp_annotations is a single field — _refresh_dino_temp_for_current syncs it on every switch_slice / switch_image so masks don't bleed between slices. - Application-wide _DINOReviewEventFilter makes Enter / Escape work during review regardless of which widget has focus. ADR-015 documents the choice over QShortcut and force-focus alternatives. - dino_batch_results initialised in __init__; dropped 4 lazy-hasattr checks. - Verbose [DINO] / [SAM] diagnostic prints at decision points (un-gated per user request — print is the project convention). Multi-dim TIFF loading: - load_tiff reads tifffile.series[0].axes and maps Y→H, X→W into the app's dimension vocab. DimensionDialog defaults to these hints when ndim matches. - Explicit ndim 3-6 fallback table, plus generic ["T"]*(ndim-2) + ["H","W"] for ndim ≥ 7. The earlier default_dimensions[-ndim:] of a 4-element list silently degraded for 5D TZCYX inputs and produced 2560 one-row "slices". Tools → Unload AI Models: - Three-step recipe: model.cpu() → gc.collect() → empty_cache + ipc_collect + synchronize. Disclosure dialog now mentions the per-process CUDA context that survives unload. - Resets both SAM + DINO dropdowns and disables Detect buttons on unload. YOLO export: - image_paths lookup uses exact-key match first, substring fallback only (prevents "bee.jpg" matching "honeybee.jpg" by substring). - Diagnostic [YOLO v5+] / [YOLO v4] prints, warning when a class isn't in class_mapping. Dark mode: - Dark mode now on by default at startup. - Removed hardcoded #e0e0e0 / #f5f5f5 from ClassThresholdTable header and lbl_dino_status (they punched bright boxes into the dark sidebar). - Added QRadioButton / QCheckBox / QHeaderView / QTableWidget / QSpinBox / QDoubleSpinBox / QComboBox / QGroupBox rules to soft_dark_stylesheet so dataset splitter radio buttons + DINO panel widgets render with adequate contrast. - Annotated-slice highlight changed from light blue (173,216,230) to muted steel-blue (58,95,140) on dark mode. Docs: - ADR-015 added — application-wide event filter for DINO review. - Cross-cutting concepts gained sections for Pan + Zoom Reference Frames, Dark Mode No Hardcoded Colors Rule, Releasing Model GPU Memory, DINO Temp Annotations (lifecycle / event filter / batch / navigation / auto-accept), Multi-dim TIFF Axis Defaults, Export Format Filename Matching. - CLAUDE.md gained a "Patterns introduced in v0.9.0" index table pointing at the arc42 deep-dives so new contributors don't re-derive them. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
2 participants
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.
Summary
Two coupled changes landing in one PR. The first unblocks the second.
QAction/QShortcutmodule relocation,QDesktopWidget→QGuiApplication.primaryScreen(),event.position()+QPointFend-to-end inimage_label.py. New ADR-014 documents the decision.sam_worker.py,dino_worker.py, andtools/check_worker_isolation.pyare deleted.sam_utils.pyanddino_utils.pynow load Ultralytics / Transformers models in-process and keep them cached on theSAMUtils/DINOUtilssingletons. Each inference call runs on a short-livedQThread; the calling thread pumps its event loop via_run_syncso the public API stays synchronous-looking and the UI stays responsive. ADR-011 is marked Superseded by new ADR-013.End result: ~1-2 s faster per SAM/DINO call on Windows (no subprocess spawn, no model reload), cleaner Linux runtime (Qt 6 native integration), and a long-term-maintained binding.
Why these are coupled
The subprocess workers existed to dodge
WinError 1114— a PyQt5+Torch DLL load-order conflict on Windows + Python 3.14, documented in the old ADR-011. Migrating to PyQt6 eliminates that conflict (Qt6 reshuffled its DLL packaging), so the entire subprocess isolation layer becomes dead code. Doing both in one PR avoids paying the migration tax twice.Phase 0 gate
New
tools/check_pyqt6_torch_coexistence.pyimports PyQt6 → torch → torchvision → transformers → ultralytics in that order and constructs aQApplication(the platform plugin is what actually loaded the conflicting DLL in the original failure). Pass = subprocess removal is safe. Verified on Linux + Python 3.11. Windows + Python 3.14 verification still required locally before merge.Threading + safety net
_run_syncruns the inference callable on aQThread, captures any exception on the worker instance, and re-raises on the calling thread (sochange_sam_model'stry/exceptactually catches now)._inference_in_flightmodule flag +InferenceBusyErrorexception serialise concurrent calls without deadlocking. AQMutexwas considered and rejected — same-thread re-acquisition of a non-recursive mutex deadlocks, recursive mutex defeats serialisation.apply_sam_predictioninannotator_window.pycarries its own_sam_inference_in_flightflag at the call site, because the SAM debounce timer can fire while an earlier inference is pumping inside_run_sync. Defence in depth._run_syncraisesRuntimeErrorif called off the GUI thread (explicitif … raise, notassert— survivespython -O)._qimage_to_numpyalways returns a fresh copy. The earlier alias-the-QImage-buffer pattern was a latent UAF in the fallback path that the migration to threading made more exposed.What's new in the UI
SAMUtils.unload()+DINOUtils.unload(). Constrained-GPU recovery path.QDesktopWidget→QScreenreplacement).Quality gate
Three senior-reviewer passes on the diff, each followed by a fix commit. Final verdict: "Mergeable as-is. Ship this commit." The iteration caught two real correctness bugs that the initial pass missed (exception-swallowing in the worker, a
tracebackNameError unblocked by fixing the first one).Version
Bumped 0.8.12 → 0.9.0 to signal the binding switch (anyone running
pip install --upgradeon an existing install needs to know PyQt5 is no longer pulled).Test plan
pytest tests/pass on Linux + Python 3.11 underQT_QPA_PLATFORM=offscreen_run_syncre-verified end-to-end (boom()→RuntimeErrorat caller)InferenceBusyErrorwhile outer pumpsQPointFarithmetic)Architecture docs touched
docs/05_building_block_view.md: SAMUtils block rewritten, DINO subprocess box dropped, ASCII diagram updated to (PyQt6)docs/06_runtime_view.md: SAM and DINO inference sequence updated for the in-process pathdocs/12_glossary.md: "Subprocess Worker" entry marked historicalCLAUDE.md: SAM Integration section, Testing Checklist, senior-reviewer checklist, platform support all updated; new bullet requiring foreground reviewer runsREADME.md,docs/01_introduction_and_goals.md,docs/02_architecture_constraints.md: PyQt5 → PyQt6PYTHON314_SETUP.mddeleted (described migration as future work)TESTING.md: stale Py3.14/DLL section excised, Py3.13 addedStats
47 files changed, +1406 / -1717. Net deletion (~300 lines) — the subprocess machinery was bulkier than its in-process replacement.
https://claude.ai/code/session_01ADoBX5VmUYpCrwbkecKMHL
Generated by Claude Code
Upstream coordination
This PR has been mirrored upstream:
cc @bnsreenu for visibility (the upstream PR mentions you too).
Manual-testing fix pack on top of the migration
The migration commits (
0604d6a,2e243c2,739fec4,2137bd6) shipped the PyQt6 binding + subprocess removal. The follow-up commit on this branch (67905f8) lands the manual-testing fix pack from a multi-round Windows verification session — closes a number of latent regressions and rough edges:.exec_()→.exec()+ missingimport traceback(latent NameError) + F2 →QShortcut(ApplicationShortcut)event.globalPosition(); cursor-anchored Ctrl+wheel zoom newly implementedDetect All Imagesnow processes multi-dim slices (was silently skipping them); slice-name navigation in batch review; mask-bleed-on-slice-switch fixed; application-wide event filter so Enter / Escape work during review regardless of focustifffile.series[0].axes; ndim 3-6 explicit defaults; ndim ≥ 7 fallbackmodel.cpu()+gc.collect()+empty_cache()+ipc_collect()+synchronize(); honest CUDA-context disclosure#e0e0e0/#f5f5f5removed; radio/checkbox/header/spinbox/combobox rules added tosoft_dark_stylesheet; annotated-slice highlight desaturateddocs/08_crosscutting_concepts.mdfor every new pattern; CLAUDE.md gained a patterns table cross-linking to the arc42 deep-dives