diff --git a/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md b/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md index efab46ed..5b1ed802 100644 --- a/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md +++ b/csaxs_bec/bec_ipython_client/plugins/omny/AI_docs/OPEN_ISSUES.md @@ -599,15 +599,42 @@ propagate it) and `test_tomo_queue_execute_marks_job_incomplete_on_keyboardinter `"running"`, and that the interrupt still propagates to the caller afterward -- it's not swallowed, just handled the same way any other mid-job exception already was). -**Still unexplained, needs more detail from Mirko to pursue further**: "i had two items in the -server queue" and "after clearing it also did not resume" -- ambiguous whether "server queue" -means the OMNY-side `tomo_queue` (this session's job list, cleared via `tomo_queue_clear()`, which -is fully destructive -- if that's what was cleared, "did not resume" is expected, there was -nothing left to resume) or BEC's own scan-server execution queue (a lower-level, separate concept; -inspectable via `bec.queue.scan_queue_status()` or the BECQueue GUI widget). Also unclear whether -the underlying GUI freeze itself (client-side Qt process on Mirko's Mac) has a deeper cause worth -chasing, or was a one-off. Not reproduced live this pass -- Mirko: "i cannot move to the machine -you are running on with my tests now." +**Follow-up, fully explained and fixed**: Mirko confirmed "i was talking becs own queue" (BEC's +own scan-server execution queue, not `tomo_queue`), and separately noticed "the gui is essentially +freezing while a scan is running... pressing queue control in the params gui, the window only +opened after the scan finished." Caught live with a stuck queue: `bec.queue.queue_storage +.describe_queue()` showed **two duplicate `device_rpc` entries stuck PENDING**, both +`device='omny_samples', func='get_sample_name_in_samplestage'` (different RIDs -- two separate +invocations), plus one `func='stop'` (routine BEC abort cleanup, unrelated), all stuck because the +queue was `PAUSED` from the earlier abort and never got serviced. + +Root cause: `OMNY_TomoParams`'s poll timer (`_on_poll()` -> `_refresh_sample_name()`, +`tomo_params.py`) calls `SETUP_PROFILES["omny"]["sample_name_getter"]` on every tick. +It was `lambda w: w.dev.omny_samples.get_sample_name_in_samplestage()` -- a device *method* call, +which BEC dispatches as a `device_rpc` request through the scan queue (a real round-trip), unlike +flomni's getter (`w.dev.flomni_samples.sample_names.sample0.get()`, a direct signal read) or +lamni's (`w.client.get_global_var(...)`, a Redis read). Polling a queue-mediated call every tick +means: (a) it blocks the widget's Qt thread waiting on the round-trip, so a busy/paused queue +freezes the *entire* GUI including unrelated actions like opening the queue-control dialog, and +(b) each tick fires a *new* request without checking if the previous one is still outstanding, so +they pile up as duplicate PENDING entries whenever the queue can't service them promptly -- +exactly what the "two items in the server queue" report was. + +`get_sample_name_in_samplestage()` (`omny_sample_storage.py:453-455`) turned out to be a one-line +wrapper: `return str(self.sample_in_samplestage_name.get())`, around an `auto_monitor=True` +EpicsSignal that's already directly readable. **Fixed**: `sample_name_getter` now reads +`w.dev.omny_samples.sample_in_samplestage_name.get(cached=True)` directly, matching flomni's +fast-getter pattern -- no more device_rpc round-trip, so no more GUI freeze and no more duplicate +queue entries from this source. Mirko, from prior advice from BEC developers: GUI code should +prefer `cached=True` where possible -- checked `DeviceBase.get()` (`bec_lib/device.py`): without +it, `.get()` still issues a live `self._run(cached=False, ...)` round-trip to the device server; +`cached=True` reads BEC's already-locally-updated status cache instead, no round-trip at all, so +used it here too (there's existing precedent for this exact pattern in GUI code, e.g. +`sample_storage.py`'s `oparkz.readback.get(cached=True)`). Regression test: +`test_omny_sample_name_getter_reads_signal_not_rpc_method` (fake device whose +`get_sample_name_in_samplestage()` raises `AssertionError` if called, and whose fake signal +records its `cached` kwarg -- confirms both that the getter only touches the signal and that it +passes `cached=True`). ## Environment note (not code, but will bite again if forgotten) diff --git a/csaxs_bec/bec_widgets/widgets/tomo_params/tomo_params.py b/csaxs_bec/bec_widgets/widgets/tomo_params/tomo_params.py index bab239fd..df9cc913 100644 --- a/csaxs_bec/bec_widgets/widgets/tomo_params/tomo_params.py +++ b/csaxs_bec/bec_widgets/widgets/tomo_params/tomo_params.py @@ -2439,7 +2439,13 @@ SETUP_PROFILES: dict[str, dict[str, Any]] = { "requested_to_stepsize": _requested_to_stepsize, "compute_fermat_positions": _compute_fermat_positions_flomni, "fermat_position_fields": ["tomo_shellstep", "fovx", "fovy"], - "sample_name_getter": lambda w: w.dev.flomni_samples.sample_names.sample0.get(), + # cached=True: no active bug here (this was always a direct signal + # read, never the device-method/device_rpc round-trip OMNY's getter + # had -- see SETUP_PROFILES["omny"]["sample_name_getter"]), but + # since this is also polled every tick (_on_poll()), avoid the live + # self._run(cached=False, ...) round-trip on principle -- standard + # advice for GUI/widget code. + "sample_name_getter": lambda w: w.dev.flomni_samples.sample_names.sample0.get(cached=True), "cli_hint_name": "flomni", "tomo_types": { 1: {"label": "8 equally spaced sub-tomograms", "n_subtomos": 8}, @@ -2547,7 +2553,28 @@ SETUP_PROFILES: dict[str, dict[str, Any]] = { "requested_to_stepsize": _requested_to_stepsize_omny, "compute_fermat_positions": _compute_fermat_positions_omny, "fermat_position_fields": ["tomo_shellstep", "fovx", "fovy"], - "sample_name_getter": lambda w: w.dev.omny_samples.get_sample_name_in_samplestage(), + # Read the signal directly with cached=True (like flomni's own + # getter), not the get_sample_name_in_samplestage() wrapper method: + # a device *method* call is dispatched as a device_rpc request + # through BEC's scan queue -- a real round-trip, not a local read -- + # so polling it every tick (see _on_poll()/_refresh_sample_name()) + # blocks the widget's Qt thread whenever the queue is busy/paused, + # and piles up duplicate PENDING device_rpc entries in BEC's primary + # queue when it is. Confirmed live: two duplicate + # get_sample_name_in_samplestage device_rpc entries stuck PENDING + # after an abort left the queue paused, plus a GUI freeze while a + # scan was running -- the wrapper method itself is just + # `return str(self.sample_in_samplestage_name.get())` + # (omny_sample_storage.py), so reading the signal directly is + # equivalent and free of both problems. cached=True goes one step + # further: without it, DeviceBase.get() still issues a live + # `self._run(cached=False, ...)` round-trip to the device server; + # cached=True reads BEC's already-locally-updated status cache + # instead, no round-trip at all -- standard advice for GUI/widget + # code (see e.g. sample_storage.py's oparkz.readback.get(cached=True)). + "sample_name_getter": lambda w: w.dev.omny_samples.sample_in_samplestage_name.get( + cached=True + ), "cli_hint_name": "omny", # OMNY has three "equally spaced sub-tomograms" flavors (2/4/8 # sub-tomograms), unlike flomni's/lamni's single always-8 flavor -- diff --git a/tests/tests_bec_ipython_client/test_omny_tomo_params_widget_math.py b/tests/tests_bec_ipython_client/test_omny_tomo_params_widget_math.py index 6146c670..f39de1bd 100644 --- a/tests/tests_bec_ipython_client/test_omny_tomo_params_widget_math.py +++ b/tests/tests_bec_ipython_client/test_omny_tomo_params_widget_math.py @@ -197,3 +197,54 @@ def test_validate_tomo_type_condition_accepts_omny_types_4_and_5(): assert tomo_type in tomo_types, f"tomo_type {tomo_type} should be valid for omny" for invalid in (0, 6, 99, None): assert invalid not in tomo_types, f"tomo_type {invalid} should be rejected for omny" + + +# --- sample_name_getter: regression guard against the device_rpc polling bug -- + + +def test_omny_sample_name_getter_reads_signal_not_rpc_method(): + """sample_name_getter used to call get_sample_name_in_samplestage(), a + device *method* -- dispatched by BEC as a device_rpc request through the + scan queue, a real round-trip, not a local read. The widget's poll timer + calls this every tick (_on_poll()/_refresh_sample_name()), so with the + queue busy or paused this blocked the widget's Qt thread (GUI "freeze" + while a scan ran) and piled up duplicate PENDING device_rpc entries in + BEC's primary queue (confirmed live, two stuck entries after an abort + left the queue paused). The method itself is just + `return str(self.sample_in_samplestage_name.get())` + (omny_sample_storage.py) -- fixed to read that signal directly, matching + flomni's own getter's pattern (a direct .get(), no RPC).""" + + class _FakeSignal: + def __init__(self, value): + self._value = value + self.get_calls = [] + + def get(self, cached=False): + self.get_calls.append(cached) + return self._value + + class _FakeOmnySamples: + def __init__(self): + self.sample_in_samplestage_name = _FakeSignal("my_sample") + + def get_sample_name_in_samplestage(self): + raise AssertionError( + "sample_name_getter must not call the device_rpc-dispatched " + "method -- it should read the signal directly" + ) + + class _FakeDev: + omny_samples = _FakeOmnySamples() + + class _FakeWidget: + dev = _FakeDev() + + signal = _FakeDev.omny_samples.sample_in_samplestage_name + result = SETUP_PROFILES["omny"]["sample_name_getter"](_FakeWidget()) + + assert result == "my_sample" + # cached=True avoids DeviceBase.get()'s live self._run(cached=False, ...) + # round-trip to the device server, reading BEC's local status cache + # instead -- standard advice for GUI/widget code. + assert signal.get_calls == [True] diff --git a/tests/tests_bec_ipython_client/test_tomo_params_widget_math.py b/tests/tests_bec_ipython_client/test_tomo_params_widget_math.py index 1f1b1f6f..2f4f77ac 100644 --- a/tests/tests_bec_ipython_client/test_tomo_params_widget_math.py +++ b/tests/tests_bec_ipython_client/test_tomo_params_widget_math.py @@ -10,6 +10,7 @@ import pytest from csaxs_bec.bec_ipython_client.plugins.flomni.flomni import Flomni from csaxs_bec.bec_widgets.widgets.tomo_params.tomo_params import ( + SETUP_PROFILES, _compute_fermat_positions_flomni, _compute_type1, _requested_to_stepsize, @@ -73,3 +74,37 @@ def test_compute_fermat_positions_flomni_zero_step_is_safe(): ) assert count == 0 assert min_positions == FlomniFermatScan.MIN_POSITIONS + + +def test_flomni_sample_name_getter_uses_cached_read(): + """Not fixing a bug here (unlike SETUP_PROFILES["omny"]["sample_name_getter"] + -- this getter was always a direct signal read, never a device-method call + dispatched as a device_rpc through the scan queue), but this is also polled + every tick (_on_poll()), so avoid DeviceBase.get()'s live + self._run(cached=False, ...) round-trip on the same principle.""" + + class _FakeSignal: + def __init__(self, value): + self._value = value + self.get_calls = [] + + def get(self, cached=False): + self.get_calls.append(cached) + return self._value + + class _FakeSampleNames: + sample0 = _FakeSignal("my_sample") + + class _FakeFlomniSamples: + sample_names = _FakeSampleNames() + + class _FakeDev: + flomni_samples = _FakeFlomniSamples() + + class _FakeWidget: + dev = _FakeDev() + + result = SETUP_PROFILES["flomni"]["sample_name_getter"](_FakeWidget()) + + assert result == "my_sample" + assert _FakeSampleNames.sample0.get_calls == [True]