From f3c90c5b91f4f591e8442e1fce2117d4c90560e2 Mon Sep 17 00:00:00 2001 From: x01dc Date: Thu, 16 Jul 2026 13:59:12 +0200 Subject: [PATCH] fix(lamni): fix three real tomo_scan() crashes, mirroring flomni's fixes bec.active_account.decode() crashed on every real tomo_scan() call -- active_account is a plain str, not bytes. Flomni's equivalent never had the .decode() call and already guards an empty active_account (e.g. a dev/sim session) by skipping sample-database registration instead of crashing. Same bug fixed in DataDrivenLamNI.tomo_scan() (extra_tomo.py), which had an identical inline copy. write_pdf_report() read a nonexistent "mokev" device; the real device is ccm_energy. Reads it for real now, wrapped in a broad try/except falling back to "N/A" if unavailable, rather than a hardcoded placeholder. write_pdf_report()'s logbook/scilog write crashed when scilog isn't configured -- lamni.py already had this exact tolerance pattern one method over, in write_to_scilog(); applied the same try/except here. Co-Authored-By: Claude Sonnet 5 --- .../LamNI/AI_docs/TOMO_PARAMS_GUI_PORT.md | 74 +++++++++++++++--- .../plugins/LamNI/extra_tomo.py | 24 +++--- .../bec_ipython_client/plugins/LamNI/lamni.py | 54 +++++++++---- .../test_lamni_tomo_angles.py | 77 +++++++++++++++++++ 4 files changed, 192 insertions(+), 37 deletions(-) diff --git a/csaxs_bec/bec_ipython_client/plugins/LamNI/AI_docs/TOMO_PARAMS_GUI_PORT.md b/csaxs_bec/bec_ipython_client/plugins/LamNI/AI_docs/TOMO_PARAMS_GUI_PORT.md index 92b6ae6..aaf4841 100644 --- a/csaxs_bec/bec_ipython_client/plugins/LamNI/AI_docs/TOMO_PARAMS_GUI_PORT.md +++ b/csaxs_bec/bec_ipython_client/plugins/LamNI/AI_docs/TOMO_PARAMS_GUI_PORT.md @@ -1,16 +1,20 @@ -# Tomo-params GUI generalization + two sub_tomo_scan() angle bugs fixed +# Tomo-params GUI generalization + real lamni.py bugs found and fixed Companion to `AI_docs/TOMO_QUEUE_PORT.md` (the backend port this GUI work builds on) and `csaxs_bec/bec_ipython_client/plugins/flomni/AI_docs/FLOMNI_TO_LAMNI_COMPARISON.md` (which identified `TomoParamsWidget` as flomni-gated). This session's follow-up work: generalized the widget to also support lamni, then — while -verifying the widget's projection-count preview against the CLI — found and -fixed two real, pre-existing bugs in `LamNI.sub_tomo_scan()` that predate -this session and affected every lamni tomogram ever run with a non-trivially- -achievable `tomo_angle_stepsize`. +verifying the widget's projection-count preview against the CLI, and later +just running `lamni.tomo_scan()` for real — found and fixed five real, +pre-existing bugs in `lamni.py` that predate this session and would crash +(or silently corrupt sampling in) any real lamni tomogram: two in +`sub_tomo_scan()`'s angle math (§2) and three in `tomo_scan()`/ +`write_pdf_report()`'s "new scan" bookkeeping (§3) — all three of the +latter group already correctly handled in flomni's equivalent code. **Status:** All of the below implemented, tested, and live-verified against -the running simulated lamni deployment. Not yet committed as of this doc. +the running simulated lamni deployment. §1/§2 committed as `bac3fa8`; §3 +found afterward in the same session, not yet committed as of this doc. ## 1. GUI: `TomoParamsWidget` generalized to lamni @@ -121,6 +125,40 @@ must keep re-deriving the achievable grid every time it runs, regardless of which code path set `tomo_angle_stepsize` (wizard, GUI Submit, queue-job restore, or a direct script assignment). +## 3. `tomo_scan()`/`write_pdf_report()`: three crashes on a real scan + +Found by simply calling `lamni.tomo_scan()` for real (not just +`sub_tomo_scan()` in isolation, which §2's tests/live checks exercised) — +all three already fixed in flomni's equivalent code, so each was a direct +port of flomni's existing fix, not a new design: + +- **`bec.active_account.decode()`** (`tomo_scan()`'s new-scan branch): + `active_account` is a plain `str`, not `bytes` — `.decode()` raises + `AttributeError` on every real call. Flomni's equivalent code never had + this `.decode()` at all. Also added flomni's existing guard: an empty + `active_account` (e.g. a dev/sim session not logged into a real e-account) + now skips `add_sample_database()` and falls back to `tomo_id = 0`, instead + of crashing while trying to register a sample under `""`. Same fix + applied to `DataDrivenLamNI.tomo_scan()` (`extra_tomo.py`), which had an + identical inline copy of this bug. +- **`dev.mokev.read(...)` in `write_pdf_report()`**: `mokev` isn't a real + device name here — Mirko identified the correct one, `dev.ccm_energy` + (confirmed against `flomni_optics_mixin.py`/`eiger.py`'s own `ccm_energy` + usage elsewhere in the repo). Per Mirko's request, this isn't a flomni- + style hardcoded placeholder ("To be implemented") — it now does a real + `dev.ccm_energy.read(cached=True)[dev.ccm_energy.name]['value']` read, + wrapped in a broad `try/except` that falls back to `"N/A"` if the device + isn't configured/available in a given session, so a missing device is + tolerated rather than crashing report generation. +- **`bec.logbook.LogbookMessage()` in `write_pdf_report()`**: crashes with + `AttributeError` when scilog/logbook isn't configured (e.g. a dev/sim + session) — `lamni.py` already has exactly this tolerance pattern one + method up, in `write_to_scilog()` (broad `try/except`, warn and continue), + it just wasn't applied here. Wrapped the same logbook-message block in an + identical `try/except`, logging `"Failed to write PDF report to + scilog."` on failure — the PDF file itself is still written either way + (it happens earlier in the same function, unaffected). + ## Explicitly deferred (carried over / reconfirmed) - `zero_deg_reference_at_each_subtomo` for lamni — not implemented anywhere @@ -137,15 +175,20 @@ restore, or a direct script assignment). - New tests: `tests/tests_bec_ipython_client/test_lamni_tomo_params_widget_math.py` (16 tests: profile field_order/param_names/defaults guards, offset getter/setter round-trip including a same-axis-doesn't-clobber-the-other - check) and `test_lamni_tomo_angles.py` (48 tests: no-duplicate-angle check - per sub-tomogram/stepsize, and the pair/quad/full-8 equally-spaced - invariant per stepsize). The equally-spaced tests were sanity-checked by - temporarily reverting the phase-offset fix and confirming 9/12 of them - fail as expected, then restoring it. + check) and `test_lamni_tomo_angles.py` (50 tests: no-duplicate-angle check + per sub-tomogram/stepsize, the pair/quad/full-8 equally-spaced invariant + per stepsize, and two tests for §3's `tomo_scan()` account handling — one + confirming an empty `active_account` skips `add_sample_database()` + entirely with `tomo_id=0`, one confirming a real account string is passed + through as-is, not `.decode()`'d). The equally-spaced tests were + sanity-checked by temporarily reverting the phase-offset fix and + confirming 9/12 of them fail as expected; the account tests were + sanity-checked the same way against the `.decode()` bug specifically, + then both reverts restored. - Existing pinned test `test_tomo_params_widget_math.py` (flomni's math) untouched and still passing. - Full offline suite (`pytest tests/tests_bec_ipython_client tests/tests_scans`): - 185 passed. + 187 passed. - Live-sim verification (plain scripts against the running simulated lamni deployment, snapshotting/restoring `tomo_queue`/`tomo_progress` around each run): @@ -158,5 +201,12 @@ restore, or a direct script assignment). `[0, 60, 120, 180, 240, 300]`, spacing exactly 60° throughout — confirms the phase-offset fix holds against real (not just mocked) execution. + - A full real `lamni.tomo_scan()` (type 1, coarse stepsize giving N=1, + 8 total real acquisitions across all 8 sub-tomograms) ran end to end + against the sim without error after §3's fixes — `tomo_id` correctly + fell back to `0` (no active account in this session), the energy read + and PDF/logbook write no longer crashed the scan. (Required creating + `~/Data10/documentation/` locally, which didn't exist in this sandbox + but is expected to already exist on the real deployed system.) - No live GUI verification performed (per Mirko's instruction) — verified manually by Mirko instead. diff --git a/csaxs_bec/bec_ipython_client/plugins/LamNI/extra_tomo.py b/csaxs_bec/bec_ipython_client/plugins/LamNI/extra_tomo.py index fbae06c..3bdafc7 100644 --- a/csaxs_bec/bec_ipython_client/plugins/LamNI/extra_tomo.py +++ b/csaxs_bec/bec_ipython_client/plugins/LamNI/extra_tomo.py @@ -102,15 +102,21 @@ class DataDrivenLamNI(LamNI): self._current_special_angles = self.special_angles.copy() if subtomo_start == 1 and start_index is None: - self.tomo_id = self.add_sample_database( - self.sample_name, - str(datetime.date.today()), - bec.active_account.decode(), - bec.queue.next_scan_number, - "lamni", - "test additional info", - "BEC", - ) + # bec.active_account is already a plain str, not bytes -- see + # LamNI.tomo_scan()'s identical fix for why .decode() crashes and + # why an empty active_account needs its own guard. + if bec.active_account != "": + self.tomo_id = self.add_sample_database( + self.sample_name, + str(datetime.date.today()), + bec.active_account, + bec.queue.next_scan_number, + "lamni", + "test additional info", + "BEC", + ) + else: + self.tomo_id = 0 self.write_pdf_report() with scans.dataset_id_on_hold: diff --git a/csaxs_bec/bec_ipython_client/plugins/LamNI/lamni.py b/csaxs_bec/bec_ipython_client/plugins/LamNI/lamni.py index c095998..75586d5 100644 --- a/csaxs_bec/bec_ipython_client/plugins/LamNI/lamni.py +++ b/csaxs_bec/bec_ipython_client/plugins/LamNI/lamni.py @@ -1002,15 +1002,23 @@ class LamNI(TomoQueueMixin, LamNIAlignmentMixin, LamNIOpticsMixin, LamniGuiTools or (self.tomo_type == 2 and projection_number is None) or (self.tomo_type == 3 and projection_number is None) ): - self.tomo_id = self.add_sample_database( - self.sample_name, - str(datetime.date.today()), - bec.active_account.decode(), - bec.queue.next_scan_number, - "lamni", - "test additional info", - "BEC", - ) + # bec.active_account is already a plain str, not bytes -- .decode() + # crashes with AttributeError. Also guard against no active + # e-account (empty string, e.g. a dev/sim session not logged into + # a real account) rather than trying to register a sample under + # one -- mirrors Flomni.tomo_scan()'s equivalent check exactly. + if bec.active_account != "": + self.tomo_id = self.add_sample_database( + self.sample_name, + str(datetime.date.today()), + bec.active_account, + bec.queue.next_scan_number, + "lamni", + "test additional info", + "BEC", + ) + else: + self.tomo_id = 0 self.write_pdf_report() self.progress["tomo_start_time"] = datetime.datetime.now().isoformat() # reset stale estimates from any previous scan, otherwise the GUI @@ -1324,6 +1332,14 @@ class LamNI(TomoQueueMixin, LamNIAlignmentMixin, LamNIOpticsMixin, LamniGuiTools stitching = f"{self.lamni_stitch_x:.2f}/{self.lamni_stitch_y:.2f}" dataset_id = str(self.client.queue.next_dataset_number) _, _, report_total_projections = self._tomo_type1_actual_grid() + # The energy readback device is "ccm_energy", not "mokev" -- and may + # not be configured/available in every session (e.g. some simulated + # configs). Read it defensively so a missing/misbehaving device + # doesn't crash the whole report instead of just omitting one line. + try: + energy_str = f"{dev.ccm_energy.read(cached=True)[dev.ccm_energy.name]['value']:.4f}" + except Exception: + energy_str = "N/A" content = [ f"{'Sample Name:':<{padding}}{self.sample_name:>{padding}}\n", f"{'Measurement ID:':<{padding}}{str(self.tomo_id):>{padding}}\n", @@ -1333,7 +1349,7 @@ class LamNI(TomoQueueMixin, LamNIAlignmentMixin, LamNIOpticsMixin, LamniGuiTools f"{'Number of projections:':<{padding}}{report_total_projections:>{padding}}\n", f"{'First scan number:':<{padding}}{self.client.queue.next_scan_number:>{padding}}\n", f"{'Last scan number approx.:':<{padding}}{self.client.queue.next_scan_number + report_total_projections + 10:>{padding}}\n", - f"{'Current photon energy:':<{padding}}{dev.mokev.read(cached=True)['value']:>{padding}.4f}\n", + f"{'Current photon energy:':<{padding}}{energy_str:>{padding}}\n", f"{'Exposure time:':<{padding}}{self.tomo_countingtime:>{padding}.2f}\n", f"{'Fermat spiral step size:':<{padding}}{self.tomo_shellstep:>{padding}.2f}\n", f"{'Piezo range (FOV sample plane):':<{padding}}{piezo_range:>{padding}}\n", @@ -1351,12 +1367,18 @@ class LamNI(TomoQueueMixin, LamNIAlignmentMixin, LamNIOpticsMixin, LamniGuiTools subprocess.run( "xterm /work/sls/spec/local/XOMNY/bin/upload/upload_last_pon.sh &", shell=True ) - msg = bec.logbook.LogbookMessage() - logo_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "LamNI_logo.png") - msg.add_file(logo_path).add_text("".join(content).replace("\n", "

")).add_tag( - ["BEC", "tomo_parameters", f"dataset_id_{dataset_id}", "LamNI", self.sample_name] - ) - self.client.logbook.send_logbook_message(msg) + # Same tolerance as write_to_scilog(): a session without scilog/logbook + # configured (e.g. a dev/sim session) must not crash report generation + # over the logbook upload -- the PDF itself is already written above. + try: + msg = bec.logbook.LogbookMessage() + logo_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "LamNI_logo.png") + msg.add_file(logo_path).add_text("".join(content).replace("\n", "

")).add_tag( + ["BEC", "tomo_parameters", f"dataset_id_{dataset_id}", "LamNI", self.sample_name] + ) + self.client.logbook.send_logbook_message(msg) + except Exception: + logger.warning("Failed to write PDF report to scilog.") def get_calibration_of_capstops_left_and_right(self): import time diff --git a/tests/tests_bec_ipython_client/test_lamni_tomo_angles.py b/tests/tests_bec_ipython_client/test_lamni_tomo_angles.py index 17c5c32..6894201 100644 --- a/tests/tests_bec_ipython_client/test_lamni_tomo_angles.py +++ b/tests/tests_bec_ipython_client/test_lamni_tomo_angles.py @@ -14,6 +14,9 @@ without a live BEC session. Guards against two bugs fixed this session: same requirement). """ +import builtins +import types + import numpy as np import pytest @@ -125,3 +128,77 @@ def test_all_eight_subtomos_are_equally_spaced_when_combined(stepsize): angles = _all_subtomo_angles(stepsize) combined = sum((angles[n] for n in range(1, 9)), []) _assert_equally_spaced(combined, "full (all 8)") + + +# ── tomo_scan()'s new-scan account handling ───────────────────────────────── +# +# bec.active_account used to be passed through .decode() (as if it were +# bytes) -- it's a plain str, so this raised AttributeError on every real +# tomo_scan() call. Flomni's equivalent code never had this bug (no +# .decode() call at all) and already guards an empty active_account (e.g. a +# dev/sim session not logged into a real e-account) by skipping sample- +# database registration instead of trying to register under "". Both fixed +# to match. + + +class _FakeContextManager: + def __enter__(self): + return self + + def __exit__(self, *exc_info): + return False + + +class _FakeScans: + dataset_id_on_hold = _FakeContextManager() + + +def make_lamni_for_tomo_scan(tomo_angle_stepsize: float, active_account: str) -> LamNI: + """Bare LamNI instance with tomo_scan()'s "new scan" branch reachable, + everything downstream of it stubbed out (sample database, PDF report, + per-sub-tomogram scanning) so only the account-handling logic under + test actually runs.""" + obj = make_lamni(tomo_angle_stepsize) + obj.tomo_type = 1 + obj.special_angles = [] + obj.write_pdf_report = lambda: None + obj.sub_tomo_scan = lambda subtomo_number, start_angle=None: None + obj._print_progress = lambda: None + obj._format_duration = lambda seconds: "0s" + builtins.__dict__["bec"] = types.SimpleNamespace( + active_account=active_account, queue=types.SimpleNamespace(next_scan_number=1) + ) + builtins.__dict__["scans"] = _FakeScans() + return obj + + +def test_tomo_scan_skips_sample_database_when_no_active_account(): + """Empty active_account (e.g. a dev/sim session) must not crash and must + not try to register a sample -- tomo_id falls back to 0, mirroring + Flomni.tomo_scan()'s identical guard.""" + lamni = make_lamni_for_tomo_scan(45.0, active_account="") + lamni.add_sample_database = lambda *a, **k: (_ for _ in ()).throw( + AssertionError("add_sample_database must not be called with no active account") + ) + + lamni.tomo_scan() + + assert lamni.tomo_id == 0 + + +def test_tomo_scan_registers_sample_with_plain_string_account(): + """A real active_account must be passed through as-is -- calling + .decode() on it (a plain str, not bytes) raises AttributeError.""" + lamni = make_lamni_for_tomo_scan(45.0, active_account="e12345") + recorded = {} + + def _fake_add_sample_database(samplename, date, eaccount, scan_number, setup, info, user): + recorded["eaccount"] = eaccount + return 42 + + lamni.add_sample_database = _fake_add_sample_database + + lamni.tomo_scan() + + assert recorded["eaccount"] == "e12345" + assert lamni.tomo_id == 42