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 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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", "</p><p>")).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", "</p><p>")).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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user