Feat: GUI updated and using process manager - #170
Conversation
…d fix orphaned device-server processes - Server/MCP GUIs: system light/dark theming, real checkmark glyphs on checkboxes, collapsed-by-default sections, larger/resizable terminal, MCP GUI tool-count badge, Digital Twin/Spectra300 host toggle (MCP GUI only - Server GUI's instrument identity is config-driven, so the toggle would silently mismatch host vs instrument there) - Server GUI: drop the Instrument file selector; it's config-driven now - process_manager.py: stale-PID cleanup now signals the whole process group, not just the recorded PID, so a hard-killed `uv run` wrapper no longer orphans the device-server child it spawned - run_servers.py: request_shutdown is now idempotent, since a duplicate SIGTERM (e.g. relayed by `uv run`) could interrupt shutdown_all() mid- cleanup and leave processes running - Resolve the llm.py/mcp_server.py merge conflicts from the upstream pull in favor of the versions consistent with the rest of each file (recursion_limit/_complete in llm.py; Tiled-backed get_data_from_key in mcp_server.py, dropping the incomplete local-path rewrite it conflicted with) - pyproject.toml: tighten pyqt6 upper bound to <6.10 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dropped upstream in a change unrelated to process management; without it, the .processes/<name>.json state file is never written during a normal run (save() only fires from stop_process()), so the next launch's _cleanup_stale_state() has nothing to reap after a crash - reopening the orphaned-device-server bug this session's process_manager.py fix addressed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWQ3GnHuE2uY2yfqKchc6U
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWQ3GnHuE2uY2yfqKchc6U
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…quisition path get_parameters no longer DevFails on the DigitalTwin: the twin now implements _get_parameters (JSON status), and every abstract stub in ElectronMicroscope raises NotImplementedError naming the class and method instead of silently returning None into a DevString - the old behavior surfaced as "can't translate python object to C char*" with no traceback. PyTango's DeviceMeta bypasses ABCMeta, so @AbstractMethod alone never enforced these overrides. get_parameters reports detectors honestly: device proxy roles under device_proxies, and the names each acquisition path actually accepts under scan_detectors / spectrum_detectors / camera_detectors, instead of publishing "stage" and "data" as detectors. acquire_scanned_image and acquire_spectrum carry doc_in, so the MCP bridge exposes a named, described parameter (detector_list / detector_name) instead of a blind "arg". acquire_spectrum rejects an unknown detector with a clean error naming the configured devices instead of an AttributeError on None. A failed image preview now returns "image preview unavailable: <reason>" as a text block instead of silently degrading to text-only, so a Tiled outage is distinguishable from a command that never produces images. Also rides along: the LLM device's Complete command and tools attribute (the counterpart of sciagentgui's llm_bridge.py), camera proxy + simulated camera acquisition on the twin, and the emptied include_only_functions allowlist in mcp_dt.yaml. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Good catch, for linux we should be using killpg because we already start processes in their own process group. No complaints!
There was a problem hiding this comment.
Not sure why the microscope hosts are hardcoded into the shared... otherwise looks fine
| ) | ||
|
|
||
|
|
||
| DIGITAL_TWIN_HOST = 'localhost' |
|
|
||
|
|
||
| DIGITAL_TWIN_HOST = 'localhost' | ||
| SPECTRA300_HOST = '10.46.217.241' |
There was a problem hiding this comment.
I think this logic (along with that in run_servers) can be consolidated into a single helper function in ProcessManager
There was a problem hiding this comment.
Good, but same comment as in run_mcp
Addresses review feedback on ff77a76: - mcp_gui.py/server_gui.py launched their subprocess via a lightweight ManagedCommand that only sent a single SIGTERM with no wait/escalation, so orphaned children could survive the Stop button. ManagedCommand now delegates to ProcessManager.start_process/stop_process for killpg + bounded wait + SIGKILL escalation, and window close also shuts the managed process down. - Consolidated the duplicated SIGTERM/SIGHUP/SIGBREAK-to-KeyboardInterrupt boilerplate in run_mcp.py and run_servers.py into a single install_shutdown_signal_handler() helper in ProcessManager. - Added on_output callback support to ProcessManager.start_process so the GUI can stream live output while still using the shared process lifecycle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix: route GUI Stop button and startup scripts through ProcessManager
There was a problem hiding this comment.
Pull request overview
Updates the Asyncroscopy startup GUIs and startup scripts to route process lifecycle through the shared ProcessManager, aiming to ensure the Stop action terminates full subprocess trees and to modernize the GUI layout/styling.
Changes:
- Refactors startup GUIs to launch/stop commands via
ProcessManager(plus new shared UI components/theme helpers). - Adds
install_shutdown_signal_handler()so SIGTERM/SIGHUP/SIGBREAK unwind launcher scripts viaKeyboardInterruptand triggerProcessManagercleanup. - Extends
ProcessManagerto optionally stream stdout/stderr lines to a callback, with new/updated tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_startup_guis.py | Adds tests for new shared GUI helpers and ManagedCommand delegating stop/shutdown. |
| tests/test_process_manager.py | Adds tests for output streaming callback and shutdown signal handler behavior. |
| startup_scripts/run_servers.py | Switches to shared shutdown signal handler installation. |
| startup_scripts/run_mcp.py | Installs shared shutdown signal handler at startup. |
| startup_guis/shared.py | Major shared GUI refactor: theming, components, instrument discovery, and ManagedCommand now uses ProcessManager. |
| startup_guis/server_gui.py | Updates server GUI layout/components and uses new shared helpers + managed command shutdown on close. |
| startup_guis/qt_compat.py | Extends Qt compatibility exports/constants used by new shared components. |
| startup_guis/mcp_gui.py | Updates MCP GUI layout/components; adds instrument preset picker + tool-count badge parsing. |
| asyncroscopy/utils/process_manager.py | Adds output streaming callback support; improves stale PID cleanup and adds shutdown signal handler helper. |
| asyncroscopy/mcp/mcp_server.py | Emits a “MCP ready: … tool(s)” line to support GUI tool-count badge. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| command=command, | ||
| env=env, | ||
| stderr=subprocess.STDOUT, | ||
| on_output=self.output_ready.emit, |
| for line in iter(stream.readline, b""): | ||
| buf.append(line.decode(errors="replace").rstrip()) | ||
| text = line.decode(errors="replace").rstrip() | ||
| buf.append(text) | ||
| if on_line is not None: | ||
| on_line(text) |
| tango = data.get('tango') | ||
| if not isinstance(tango, dict) or 'host' not in tango or 'port' not in tango: | ||
| continue | ||
| label = (data.get('instrument') or {}).get('description') or path.stem |
…acquired spectra Tango's 3000 ms client default killed the first acquire_camera_image mid-run (API_DeviceTimedOut) while the command kept executing server-side; the bridge's tool proxies now allow 30 s, inside SciAgentGUI's 60 s HTTP window, matching the timeout handling auto_script.py already does for its own proxies. acquire_spectrum returned only the saved .h5 key, so a chat client had nothing to display. The bridge now reads the 1D dataset back from Tiled and attaches a matplotlib PNG plot, the same way camera and scanned images already ship their 2D previews. The digital twin saves its per-element composition labels as a dataset attribute so the preview can draw a labeled bar chart; spectra without labels fall back to an honest counts-vs-channel line. Also removes a stray debug print from _create_wrapper. This commit also carries the operator's completed extraction of describe_tiled_node into asyncroscopy/data/data_reader.py: the bridge file in its current state already imports it, so committing one without the other would not build. The unrelated in-progress stage-realism work in digital_twin.py is deliberately left uncommitted. Covered by 8 new offline tests in tests/test_mcp_server.py and verified against the live Tiled server with a previously acquired spectrum. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both branches reworked the startup GUIs and ProcessManager in parallel. The initial-testing state is the one verified end-to-end against the live digital twin, MCP bridge, and SciAgentGUI today (timeout fix, spectrum previews, and the full offline MCP test suite), so this merge records main's history while keeping the working content unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Updated GUIs by ensuring that it utilizes the process manager because the stop button wasn't killing all subprocess and improved design (increasing terminal sizing, removing duplicate buttons, etc.).