fix(omny): stop polling sample name via a device_rpc queue round-trip
CI for csaxs_bec / test (push) Successful in 1m51s
CI for csaxs_bec / test (push) Successful in 1m51s
OMNY_TomoParams's poll timer called sample_name_getter every tick, which for OMNY was dev.omny_samples.get_sample_name_in_samplestage() -- a device method, dispatched by BEC as a device_rpc request through the scan queue rather than a local read. 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 via bec.queue.queue_storage .describe_queue()). The method was just a one-line wrapper around an already-readable EpicsSignal, so read it directly with cached=True instead, matching flomni's fast-getter pattern and avoiding even the signal round-trip. Also applied cached=True to flomni's own getter, which didn't have the device_rpc bug but is polled the same way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLrD7sVYGLAzsQjLVJpCgt
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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 --
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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]
|
||||
|
||||
Reference in New Issue
Block a user