From ff50711dc9738b3a5ae2df4bbd478173d85820b3 Mon Sep 17 00:00:00 2001 From: Mirko Holler Date: Sun, 12 Jul 2026 23:25:33 +0200 Subject: [PATCH] test(flomni): add tomo-queue e2e harness against the simulated flOMNI Sections A-C of TOMO_QUEUE_TESTING.md's checklist, run against a live sim session: params-restored-per-job, legacy queue migration, empty/all-done no-ops, start_index semantics, exception/SIGKILL crash-resume, resume- before-fresh ordering, and concurrent queue edits from a second client. All 10 tests pass together with no cross-test interference. The flomni_sim fixture and _bootstrap.py factor out what it takes to construct a live Flomni against the sim (builtins/reload bootstrap, side-effect neutralization, RT-feedback/fsamx setup) for reuse by both in-process tests and the SIGKILL-based subprocess tests. Also adds the AI_docs/ handoff docs (testing checklist results, command- jobs plan with the action-registry decisions) that this work updated. Co-Authored-By: Claude Sonnet 5 --- .../AI_docs/TOMO_QUEUE_COMMAND_JOBS_PLAN.md | 510 ++++++++++++++++++ .../flomni/AI_docs/TOMO_QUEUE_TESTING.md | 352 ++++++++++++ tests/e2e/_bootstrap.py | 113 ++++ tests/e2e/_queue_helpers.py | 120 +++++ tests/e2e/_run_queue_subprocess.py | 36 ++ tests/e2e/conftest.py | 37 ++ tests/e2e/test_smoke.py | 8 + tests/e2e/test_tomo_queue.py | 159 ++++++ tests/e2e/test_tomo_queue_concurrency.py | 120 +++++ tests/e2e/test_tomo_queue_crash.py | 89 +++ tests/e2e/test_tomo_queue_crash_subprocess.py | 89 +++ 11 files changed, 1633 insertions(+) create mode 100644 csaxs_bec/bec_ipython_client/plugins/flomni/AI_docs/TOMO_QUEUE_COMMAND_JOBS_PLAN.md create mode 100644 csaxs_bec/bec_ipython_client/plugins/flomni/AI_docs/TOMO_QUEUE_TESTING.md create mode 100644 tests/e2e/_bootstrap.py create mode 100644 tests/e2e/_queue_helpers.py create mode 100644 tests/e2e/_run_queue_subprocess.py create mode 100644 tests/e2e/conftest.py create mode 100644 tests/e2e/test_smoke.py create mode 100644 tests/e2e/test_tomo_queue.py create mode 100644 tests/e2e/test_tomo_queue_concurrency.py create mode 100644 tests/e2e/test_tomo_queue_crash.py create mode 100644 tests/e2e/test_tomo_queue_crash_subprocess.py diff --git a/csaxs_bec/bec_ipython_client/plugins/flomni/AI_docs/TOMO_QUEUE_COMMAND_JOBS_PLAN.md b/csaxs_bec/bec_ipython_client/plugins/flomni/AI_docs/TOMO_QUEUE_COMMAND_JOBS_PLAN.md new file mode 100644 index 0000000..745120c --- /dev/null +++ b/csaxs_bec/bec_ipython_client/plugins/flomni/AI_docs/TOMO_QUEUE_COMMAND_JOBS_PLAN.md @@ -0,0 +1,510 @@ +# Plan: Command Jobs in the FlOMNI Tomo Queue + +**Status:** design agreed, **and the §10 open questions are now resolved with Mirko** +(one small detail — `optimize_idgap`'s peak-detection signal — deferred to the +implementation session). **Step 1 (executor rework) is implemented and its sim +checklist has passed** (`TOMO_QUEUE_TESTING.md` §4, sections A–C). Steps 2 and 3 +below are not yet written — this is now unblocked to start. Implement in a dedicated +session — this touches the CLI (`flomni.py`), the GUI widget (`tomo_params.py` / +`TomoQueueDialog`), and the web page generator (`flomni_webpage_generator.py`). + +**Prerequisite (met):** the checklist in `TOMO_QUEUE_TESTING.md` has passed against +the simulated flOMNI. That document is the companion to this one: it covers *what +Step 1 changed and how to test it*; this one covers *what comes next*. + +> **This document supersedes the earlier version of the same name.** The earlier +> draft assumed the old index-based executor, single-action command jobs, and left +> the registry-distribution question open. All three have since been decided. + +**Goal:** Allow the tomo queue to hold, in addition to tomogram jobs, "command" jobs +that reconfigure the beamline between tomograms — move a device, change energy, move +optics in/out, optimize a parameter — so one queue can express *"tomogram A, then +reconfigure, then tomogram B"*, unattended. + +--- + +## 0. Chosen model, and what was rejected + +**Chosen: a named-action registry.** Command jobs name actions from a curated +allow-list, plus keyword arguments. Everything stays JSON-serializable, auditable in +`client.get_global_var("tomo_queue")`, and safe to persist across kernel restarts. + +**Rejected: storing Python source strings and `exec`-ing them later.** Not auditable; +fails at run time (hours into an overnight run) rather than at add time; breaks the +crash-resume model. If the registry ever can't express something, adding an entry is a +small, reviewed change — preferable to a permanent arbitrary-code channel on a live +beamline. + +**Also rejected: a code editor in the GUI with a syntax check.** Syntax-valid is nearly +worthless as a safety check — `umv(dev.fsamx, -1e9)` parses perfectly and drives the +stage into a hard stop. A syntax check gives the operator *false confidence*. The +structured builder (§4) is both safer and friendlier: there is nothing to type wrong. + +--- + +## 1. Current state + +### Step 1 — DONE (executor rework) + +`tomo_queue_execute()` is now an **id-based pick-next loop**: it re-reads the queue +from its global var at the start of every job and addresses jobs by a stable `id` +(not by list index). `_TomoQueueProxy` gained `update_by_id()` and `ensure_ids()`; +`tomo_queue_add()` stamps a uuid on each job. + +This is what makes it safe to **append, delete a pending job, and reorder the pending +tail while the queue runs** — the foundation the GUI work (§5) depends on. It also +introduced **resume-before-fresh** ordering: any `incomplete`/`running` job runs before +any fresh `pending` one, protecting the single shared `progress` global var. + +See `TOMO_QUEUE_TESTING.md` §1 for the full rationale and the invariants to preserve. + +### Job dict schema today + +```python +{ + "id": str, # uuid4 hex; stable across reorder/edit + "label": str, + "params": {<_TOMO_QUEUE_PARAM_NAMES>: value, ...}, + "status": "pending" | "running" | "incomplete" | "done", + "added_at": ISO datetime string, +} +``` + +Every job today is *implicitly* a tomogram. Adding command jobs = making the job +**type-tagged** and branching in the executor. + +### The GUI as it stands + +`TomoQueueDialog` (in `tomo_params.py`) reads/writes the same `tomo_queue` global var +directly, renders one row per job, colour-codes by status, and polls every 2 s. +Execution is CLI-only (the GUI server process can't run the blocking scan loop). + +--- + +## 2. Schema: type-tagged jobs + +Add a `"kind"` field. **Backward compatible:** a job with no `kind` is treated as +`"tomo"`, so existing persisted queues and existing GUI code keep working. + +### Tomo job (unchanged except the tag) +```python +{ + "kind": "tomo", # NEW; absent => "tomo" + "id": str, + "label": str, + "params": {...}, + "status": ..., + "added_at": ..., +} +``` + +### Command job (new) — a **list of steps**, not a single action +```python +{ + "kind": "command", + "id": str, + "label": str, + "steps": [ # ordered; run in sequence within the one job + {"action": "move", "kwargs": {"positions": {"mokev": 6.2}}}, + {"action": "optimize_idgap", "kwargs": {"search_range": 0.5}}, + ], + "idempotent": bool, # true iff EVERY step is idempotent + "status": ..., + "added_at": ..., +} +``` +(`foptics_out`/`foptics_in`-style steps aren't in the initial registry — see §3 — +this example only uses the two actions Step 2 actually ships with.) + +**Why a list.** "Between tomogram A and B: change energy, re-peak idgap" is one +*intent* and should be one queue row — it reads correctly, and it is atomic to reason +about. Composing it from three separate queue rows is worse on both counts. + +Notes: +- `kwargs` must be JSON-serializable. No device objects, no callables — actions resolve + device names to `dev.*` themselves at execution time. +- Steps within a job are **independent**: none of them consumes another's runtime + result. Anything result-dependent belongs *inside* a single action (§3.2). + +--- + +## 3. The action registry + +A class-level dict on `Flomni` mapping an action name to a spec. Callables are existing +`Flomni`/mixin methods or small reviewed wrappers, so the queue can only ever invoke +reviewed beamline operations. + +**Each registry entry has four things:** + +| field | meaning | +|---|---| +| `func` | name of the bound method to call | +| `idempotent` | default: safe to re-run after a crash? | +| `help` | short operator-facing description | +| `params` | **schema** of the action's arguments (possibly empty) | + +**Decided with Mirko: the initial registry is deliberately minimal — `move` and +`optimize_idgap` only.** No `foptics_in`/`foptics_out`/`feye_in`/`feye_out`/shutter +action for now, even though the first three already exist as callable methods — +Step 2 ships with move-on-allow-listed-devices plus one compound action; further +one-click actions are "dedicated scripts" to be written and added later as their own +reviewed changes, not part of this rollout. No dedicated energy-change wrapper either +— energy changes go through `move` on `mokev` (see §3.1), not a special action. + +```python +_TOMO_QUEUE_ACTIONS = { + "move": { + "func": "_queue_action_move", + "idempotent": True, # absolute moves re-run harmlessly + "help": "Move device(s) to absolute position(s).", + "params": { + "positions": {"type": "device_positions"}, # {device: target}, allow-listed + }, + }, + # compound actions — see §3.2 + "optimize_idgap": { + "func": "optimize_idgap", + "idempotent": True, + "help": "Scan idgap over a search range and move to the peak.", + "params": { + "search_range": { + "type": "float", "default": 0.5, "min": 0.0, "max": 2.0, + "unit": "mm", "label": "Search range", + }, + }, + }, +} +``` + +**Still open (confirm before/while implementing `optimize_idgap`):** which +signal/device the scan reads to locate "the peak" — deferred by Mirko to the +implementation session, not resolved yet. The `search_range` default/bounds above +are confirmed as "roughly right" placeholders; unit (`mm`) not explicitly +reconfirmed but not contradicted either. + +### 3.1 Curated device allow-list (NOT "any device in `dev`") + +The `move` action may only touch devices on an explicit, reviewed allow-list — the +things you actually reconfigure between tomograms. "Any device in `dev`" is the +difference between *a reconfiguration tool* and *a remote control for the whole +endstation*. + +**Decided with Mirko: `mokev` (energy) and `idgap` (undulator gap) — nothing else +yet.** Neither device is in the flomni sim config (`simulated_flomni.yaml`); both +live in `bl_frontend.yaml` (shared beamline front-end devices), so `move` on either +is only exercisable against the real front end, not the flomni sim used for +`TOMO_QUEUE_TESTING.md`. + +```python +_TOMO_QUEUE_MOVE_DEVICES = { + "mokev": {"label": "Energy", "unit": "keV"}, + "idgap": {"label": "Undulator gap", "unit": "mm"}, # unit unconfirmed, see above +} + +def _queue_action_move(self, positions: dict): + """positions: {device_name: absolute_target, ...}. Absolute umv only.""" + args = [] + for name, target in positions.items(): + if name not in self._TOMO_QUEUE_MOVE_DEVICES: + raise ValueError(f"Queue move: device '{name}' is not queue-movable.") + if name not in dev: + raise ValueError(f"Queue move: unknown device '{name}'.") + args += [dev[name], target] + umv(*args) +``` + +The GUI's device dropdown is built from this allow-list, so an operator cannot even +select a device that isn't sanctioned. + +### 3.2 Compound actions, and where result-dependent logic lives + +**Rule: anything where step B needs step A's *runtime result* is ONE named action, not +several queue steps.** + +The motivating case is "scan idgap, move to peak". That is not three primitives — it is +one compound operation with an internal result. It becomes a single registry entry +(`optimize_idgap`) whose method does the scan, finds the peak, and moves there. The +cleverness lives in reviewed Python; the queue only ever holds the *intent*: + +```python +{"action": "optimize_idgap", "kwargs": {"search_range": 0.5}} +``` + +Why this matters: the queue then **never has to carry runtime values between steps**, so +`kwargs` stays plain JSON and crash-resume stays honest. Trying to express "move to +wherever the peak turned out to be" as separate queue steps is exactly what would force +a code channel. + +**Compound actions may take parameters** (`search_range` above) — declared in the same +`params` schema as any other action. Parameters are *scalar inputs the operator sets at +add time*: numbers, bools, a choice from a fixed list, a device from the allow-list. The +schema is deliberately **not** expressive enough to encode logic, expressions, or +references to other jobs' results. That limitation is the point — a parameter is a value +you can validate at add time. + +### 3.3 Validation is two-layer + +- **Schema validation at add time** (GUI + `tomo_queue_add_command`): types, min/max, + device on the allow-list, OK disabled until valid. This is for *ergonomics* — catch + the typo now, not at 2am. +- **Method validation at execution time**: each action re-checks its own arguments + against live hardware. This is the *safety boundary*, because the schema and the + hardware can disagree (limits changed; a value arrived via the CLI bypassing the + form). A refusal here trips the executor's `incomplete`-and-pause path, which is the + correct outcome. + +--- + +## 4. New / changed CLI methods (`flomni.py`) + +### `tomo_queue_add_command(steps, label=None, idempotent=None)` + +Validate every step's `action` against `_TOMO_QUEUE_ACTIONS`; validate its `kwargs` +against the action's `params` schema (including the device allow-list for `move`); +confirm JSON-serializability (round-trip `json.dumps`); resolve `idempotent` +(explicit arg > **all steps idempotent**); append a `kind="command"` job with a fresh +uuid. Return the new index. Mirror the print style of `tomo_queue_add`. + +Convenience for the common single-step case is fine (`steps` may accept a single dict), +but the stored schema is always a list. + +```python +flomni.tomo_queue_add_command( + [{"action": "move", "kwargs": {"positions": {"mokev": 6.2}}}, + {"action": "optimize_idgap", "kwargs": {"search_range": 0.5}}], + label="reconfigure to 6.2 keV", +) +``` + +### `tomo_queue_add(...)` — unchanged behaviour +Keep as-is, but write `"kind": "tomo"` explicitly going forward (new jobs +self-describing; old ones without the field still default to tomo on read). + +### `tomo_queue_show()` — render both kinds +Branch on `kind`. Command rows: show the step sequence compactly + an idempotency +marker. Keep column alignment so the two kinds read as one list. + +``` +[2] pending reconfigure 6.2keV CMD move{mokev:6.2} > optimize_idgap{search_range:0.5} [idem] +``` + +### `tomo_queue_execute()` — branch on kind +Inside the existing **pick-next loop** (do not reintroduce an index loop), after +selecting the job and determining `resume_job`: + +```python +kind = job.get("kind", "tomo") +self._tomo_queue_proxy.update_by_id(job_id, status="running") +try: + if kind == "command": + self._run_command_job(job, resume_job) + else: # "tomo" + for name, value in job["params"].items(): + setattr(self, name, value) + if resume_job: + self.tomo_scan_resume() + else: + self.tomo_scan() +except Exception as exc: + self._tomo_queue_proxy.update_by_id(job_id, status="incomplete") + ... # existing pause/hint/raise +self._tomo_queue_proxy.update_by_id(job_id, status="done") +``` + +Note the tomo branch reads `job["params"]` — the command branch must never touch it +(command jobs have no `params` key). + +### `_run_command_job(job, resume_job)` +Apply the resume policy (§5), then run each step in order: resolve the action spec, +`getattr(self, spec["func"])`, call with `**step["kwargs"]`. + +--- + +## 5. Crash-resume semantics + +Tomo jobs resume mid-scan via `tomo_scan_resume()` because progress lives in global +vars. Command jobs have no such state. Policy: + +**No per-step resume cursor.** Deliberately: tracking which step of a multi-step job +completed means more persisted state and more edge cases than this warrants. + +**The job-level `idempotent` flag decides:** + +- `idempotent == True` (all steps idempotent — absolute moves, in/out, a scan-to-peak): + silently **re-run the whole step sequence from the top**. Re-running an absolute move + or an in-out is harmless. +- `idempotent == False` (any relative/stateful step): **do not silently re-run.** Stop + and prompt the operator (`OMNYTools.yesno`): *"Command job '