Audit the ophyd-async devices and their mock tests by sligara7 · Pull Request #88 · NSLS2/hextools

Anthony Sligar (sligara7) · 2026-09-21T15:04:06Z

Audits the ophyd-async devices and their mock tests. Bluesky plans are deliberately out of scope and will follow in a separate PR.

Why

The suite could not be collected at all. tests/test_motors.py imported DoubleObjCamera, which was renamed to FOV_2_4_mm_Camera when motors.py was split, and a stale import at collection time aborts the whole run rather than failing one file. That is why the Unit Tests job has been reporting nothing since 17 Sep rather than reporting failures.

Underneath that, five tests were failing and two detectors had no tests at all.

Results

	before	after
suite	aborts at collection	148 passed, 0 failed
Phantom	13 tests, 4 failing	15, all passing
GeRM	3 tests, 1 failing	6
Kinetix	0	6
Perkin Elmer	0	3
SampleTower / OpticsTable / RotationMotor	0	6
NSLS2StorageRing	0	2

Verified in both the dev and py313 environments.

Device fixes

Each of these replaces a failure that presents as something else:

germ.py had from tkinter.font import names — an accidental editor auto-import that runs at import time. On a machine whose Python was built without Tk (common in minimal conda environments and containers) import hextools raises ImportError before any beamline code runs.
Phantom wait_for_idle indexed its cine DeviceVector with a raw value off the IOC, so an out-of-range number surfaced as a bare KeyError at the operator's console. It now says what was reported, what the device was built with, and where to look.
Phantom start_acquiring waited for the array counter to be exactly equal to the post-trigger count. That counter is monotonic and watched over Channel Access, where updates coalesce — a missed value meant a good acquisition fell into the timeout branch and reported a cine-write failure that had not happened. Now >=.
Phantom start_acquiring also broke out of its trigger retry loop when the value stream ended, not only when the trigger arrived, so an exhausted stream read as a successful trigger.
GeRM prepare_internal documented livetime=0 as "keep what is configured" and then set 0 regardless, wiping the exposure time. Now guarded the way PhantomTriggerLogic.prepare_internal already does.
calculate_scan_time compared its two optional arguments with == where it meant !=, so it rejected both legal calls and accepted the one it forbids, leaving the motion-limited branch unreachable.
Two print() calls in library code that wrote to the operator's console mid-scan are removed; the values they carried now go in the timeout message, where they are useful.

Test notes

test_germ_detector_describes_elements_then_energy_bins is the one worth a look. Nothing anywhere constructed GeRMDetector — test_germ.py imported the class and never used it. This test pins the frame as (elements, energy bins). I verified it by reintroducing a transposed NDArrayDescription, watching the test fail, and restoring it.

The three drifted Phantom tests were describing an older frame-counting version of the download path; they now drive the signals the implementation actually uses. test_detector_full_stack additionally left the Proc plugin's num_filter at the mock default of 0, which divided by zero inside ophyd-async; a real NDPluginProcess reports at least 1. Its final assertion also expected (11, 3, 4) where the stream holds (1, 11, 3, 4) — one event carrying 11 frames, consistent with the descriptor shape and single stream_datum the same test already asserts.

test_germ_acquire_logic asserted device state after a fixed asyncio.sleep(0.2) and only ever exercised start_acquiring. It now drives the transition explicitly and awaits wait_for_idle, which had no test.

One thing to decide

perkin_elmer_factory is the only new public API here. The PE had no hextools code at all — it was constructed inline in profiles/collection.py, which pytest excludes, so "test the PE device" was otherwise unanswerable. The factory mirrors kinetix_factory and makes the plugin chain explicit (HDF behind Proc1 behind CB1) rather than relying on a default. It is isolated in its own commit and can be dropped without unpicking anything else.

Not in this PR

flyers.py still has no tests — it sits on the device/plan boundary and belongs with whichever half you prefer.
The Lint job will still fail. The device modules are clean, but the package as a whole has remaining errors in tomography/, profiles/ and photon_delivery_system/shutter.py, plus undocumented enum classes in germ.py and two invalid-class-name hits on FOV_2_4_mm_Camera / FOV_20_40_mm_Camera — left alone since renaming those would churn the profile.
FRAME_PERIOD_MARGIN is still one constant carrying four detectors' values in trailing comments. Where per-detector readout headroom should live is a design question, not something to settle inside a coverage change.

motors.py renamed DoubleObjCamera to FOV_2_4_mm_Camera when the module was split, and tests/test_motors.py was not updated. The stale import raises at collection time, which aborts the ENTIRE suite rather than failing one file, so no test results are reported at all. Pure rename; the test bodies are unchanged. FOV_2_4_mm_Camera keeps the same signals the tests use (_obj_selector_home_sts, _at_left_objective, _at_right_objective) and still defaults its name to "double_obj_camera". All 3 motors tests pass. Full suite now collects: 5 failed, 121 passed, where previously it reported nothing.

The guard compared the two optional arguments with == where it meant !=, so it rejected exactly the two cases its own error message calls legal and accepted the one it calls illegal: neither given -> raised "must be provided together or not at all" both given -> raised the same only one given -> returned a result, silently ignoring it That made the motion-limited branch below it unreachable: travel_distance / max_velocity could never be evaluated. Nothing calls this function yet and nothing tested it, which is why the error survived. Adds tests/detectors/test_detector_utils.py covering all four argument combinations. Test file is named test_detector_utils.py rather than test_utils.py because tests/ has no __init__.py, so pytest derives module names from basenames and a second test_utils would collide with tests/test_utils.py at collection.

Removes a stray 'from tkinter.font import names' in germ.py that ran at import time and would raise ImportError on any machine whose Python was built without Tk, plus four other unused imports there, an unused epics_signal_rw_rbv in phantom.py, import ordering, and blank lines after docstrings in shutter.py. Mechanical only - no behaviour change. Suite unchanged: 5 failed, 121 passed.

Phantom, wait_for_idle: the selected cine index came straight off the IOC and was used to index a DeviceVector keyed 1..num_cines, so an out-of-range value raised a bare KeyError at the operator's console. Check it and say what was reported, what the device was built with, and where to look. Phantom, start_acquiring: waiting for the post-trigger frames compared the array counter for EXACT equality. That counter is monotonic and watched over Channel Access, where updates coalesce - a missed value meant the match never happened and a good acquisition fell into the timeout branch, reporting a cine-write failure that had not occurred. Use >=. Phantom, start_acquiring: the trigger wait broke out of its retry loop when the value stream ENDED, not only when the trigger arrived, so an exhausted stream read as a successful trigger. Track it explicitly. GeRM, prepare_internal: the docstring says livetime=0 means "keep what is set" and the code set 0 regardless, wiping the configured exposure time. Guard it the way PhantomTriggerLogic.prepare_internal already does. Also drops two debug print() calls that wrote to the operator's console mid-scan, and folds the counts they carried into the timeout message where they are actually useful.

…MDetector The five failing tests were describing behaviour the devices no longer have. Phantom download path had NO passing test at all - the three that covered it all failed. wait_for_idle now completes on the selected cine's cine_content_saved signal rather than counting frames, and start_acquiring grew a guard on how many frames the camera actually holds; the tests still described the older frame-counting version. Updated to drive the real signals, and added coverage for the two guards: an out-of-range cine index, and asking for more frames than were recorded. test_detector_full_stack additionally left the Proc plugin's num_filter at the mock default of 0, which divided by zero inside ophyd-async when the detector read back its own state. A real NDPluginProcess reports at least 1. Its final assertion also expected (11, 3, 4) where the stream holds (1, 11, 3, 4) - one event carrying 11 frames, consistent with the descriptor shape and single stream_datum the same test already asserts. test_germ_acquire_logic asserted device state after a fixed 0.2s sleep and only ever exercised start_acquiring. It now drives the Acquire transition explicitly and awaits wait_for_idle, which had no test before. NEW, and the one that matters: test_germ_detector_describes_elements_then_ energy_bins constructs GeRMDetector - which nothing did - and pins the frame as (elements, energy bins). Verified by reintroducing the transposed NDArrayDescription and watching this test fail, then restoring it. Also pins the livetime=0 contract on GeRM prepare_internal. Full suite: 131 passed, 0 failed.

Neither detector had a single test. Both are thin wrappers over stock ophyd-async classes, so what needs pinning is the HEX wiring - the PV prefix and the plugin chain - not the upstream driver. Kinetix (6 tests): the PV prefix for each detector number, that Proc1 is wired so frames can be averaged, and that the name survives construction (it becomes the asset directory, so a mangled one misfiles data). Perkin Elmer: there was no hextools code to test at all - the detector was constructed inline in profiles/collection.py, which pytest excludes. Adds detectors/perkin_elmer.py with perkin_elmer_factory, mirroring kinetix_factory, and points the profile at it so the wiring has one home. That makes the plugin chain explicit rather than relying on a default: HDF behind Proc1 behind CB1, which is what the free-running PE needs. 3 tests cover the prefix, both plugins, and the name. NOTE FOR REVIEW: perkin_elmer_factory is the only new public API in this branch. If Jakub would rather the PE stayed inline in the profile, this commit can be dropped on its own - nothing else depends on it. Full suite: 140 passed.

None of these devices had a test. SampleTower: pins that a read records the three VIRTUAL axes (y, pitch, roll) and not the seven real motors that combine to give them. Ten axes in every event document would bury the three that mean something, and the real motors stay reachable as attributes for alignment work. That intent lived only in a comment. OpticsTable: pins all ten axes appear in a read. RotationMotor: counts per revolution is 360 degrees times the encoder resolution. tomo_flyscan positions by encoder count, so this conversion decides where a rotation scan actually starts. NSLS2StorageRing: pins the facility DCCT PV, which is a contract with the accelerator rather than something HEX can rename - the beam-drop suspender watches it - and that beam current is reported as configuration. Full suite: 148 passed.

ContAcqDetector gets its hdf attribute dynamically from the writer factory, which the type checker cannot see, so touching it in the fixture added a diagnostic. None of the three PE tests need it - they check the PV prefix, the plugin chain and the name. Type diagnostics: 75 -> 74, which is main's count. This branch now adds none.

Copilot

Copilot review overview

🟡 Changes recommended

The new RotationMotor test calls an instance method as an unbound method with None for self, which is incorrect usage and likely to fail ty check in CI.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Call get_encoder_counts_per_rev on a RotationMotor instance · New

What changed in this PR

This PR restores test suite collectability and expands mock-based coverage for multiple ophyd-async devices at HEX, while fixing several device-level behaviors that previously surfaced as misleading runtime errors/timeouts.

Changes:

Fix Phantom and GeRM acquisition/idle logic edge cases and improve timeout/error reporting.
Add/repair mock tests for motors, storage ring, detector factories (Kinetix/Perkin Elmer), and shared detector utilities.
Introduce perkin_elmer_factory and update the collection profile to use it.

File	Description
tests/test_motors.py	Updates camera class import and adds coverage for SampleTower/OpticsTable reads and RotationMotor encoder conversion.
tests/test_machine.py	Adds mock tests for `NSLS2StorageRing` PV contract and configuration readback.
tests/detectors/test_phantom.py	Aligns Phantom tests with updated trigger/download semantics and adds regression cases for frame availability and cine selection.
tests/detectors/test_perkin_elmer.py	Adds tests validating Perkin Elmer factory wiring and PV prefix/name behavior.
tests/detectors/test_kinetix.py	Adds tests validating Kinetix factory PV prefixing, proc plugin wiring, and naming.
tests/detectors/test_germ.py	Expands GeRM trigger/acquire tests and adds detector description shape regression coverage.
tests/detectors/test_detector_utils.py	Adds tests covering `calculate_scan_time` argument contract and expected timing.
src/hextools/profiles/collection.py	Switches Perkin Elmer construction to `perkin_elmer_factory`.
src/hextools/photon_delivery_system/shutter.py	Modernizes `Hashable` import and minor formatting cleanups.
src/hextools/photon_delivery_system/dclm.py	Minor formatting cleanup in docstring area.
src/hextools/detectors/utils.py	Fixes `calculate_scan_time` validation logic for optional velocity/distance arguments.
src/hextools/detectors/phantom.py	Fixes trigger wait loop semantics, relaxes post-trigger equality to `>=`, improves cine validation and timeout diagnostics, removes console `print()`.
src/hextools/detectors/perkin_elmer.py	Introduces `perkin_elmer_factory` with explicit proc/CB plugin chain.
src/hextools/detectors/kinetix.py	Minor import/docstring formatting cleanup around factory.
src/hextools/detectors/germ.py	Removes accidental Tk import, fixes `livetime=0` behavior, removes console `print()`, tidies imports.
src/hextools/detectors/__init__.py	Comment formatting cleanup for `FRAME_PERIOD_MARGIN`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🟢 Approval recommended

The changes are well-scoped to fixing device behavior and restoring/expanding tests, with only a minor maintainability nit identified in a new unit test.

Review effort: Lite
Findings: 1

Open (1)

Call get_encoder_counts_per_rev on a RotationMotor instance

Passing None for self worked at runtime because the method does not touch it, but it is wrong usage and ty rejected it: Expected `RotationMotor`, found `None` (tests/test_motors.py:109) Construct the motor under init_devices(mock=True) and call the method on it. The assertion sits OUTSIDE the context manager, since init_devices connects the device on exit. Type diagnostics 74 -> 73, one below main.

Anthony Sligar (sligara7) · 2026-09-21T15:15:24Z

Good catch — fixed in 5023faf.

Confirmed it was not just style: ty rejected it outright.

tests/test_motors.py:109:50
    RotationMotor.get_encoder_counts_per_rev(None, encoder_resolution)
                                             ^^^^ Expected `RotationMotor`, found `None`

I took the instance approach as suggested, with one change to the placement: the assertion goes outside the init_devices(mock=True) block rather than inside it. init_devices connects the devices on __exit__, so asserting inside the block runs against an unconnected device. The test also needs the RE fixture, since init_devices requires the bluesky event loop.

def test_rotation_motor_counts_per_rev(
    RE, encoder_resolution: float, expected_counts_per_rev: int
):
    with init_devices(mock=True):
        motor = RotationMotor("TEST:ROT:")

    assert (
        motor.get_encoder_counts_per_rev(encoder_resolution)
        == expected_counts_per_rev
    )

Type diagnostics for the branch go 74 -> 73, one below main. All 9 motors tests pass; full suite still 148 passed.

Copilot

Copilot review overview

🔵 Needs a closer look

The new public perkin_elmer_factory has a docstring/API mismatch (mentions cb_suffix as configurable when it is hard-coded), which should be corrected before merge.

Review effort: Lite
Findings: None

Resolved since last review (1)

Call get_encoder_counts_per_rev on a RotationMotor instance

Previously missed (1)

In code that hasn't changed since last review

Docstring incorrectly describes hard-coded cb_suffix as configurable

src/hextools/detectors/perkin_elmer.py:15

The docstring mentions cb_suffix as if it were a configurable parameter, but the factory doesn’t accept it; it is hard-coded to CB1: below. This is confusing for API users—either expose cb_suffix as an argument or describe the fixed suffix in the docstring.

get_encoder_counts_per_rev returned int(360.0 * encoder_resolution). encoder_resolution is the motor record's ERES, whose own definition is "Encoder Step Size (EGU)" - the size of ONE count, in degrees. Counts per revolution is therefore 360 divided by it, not multiplied. int(360.0 * 0.0009) = 0 <- what it returned at a fine encoder int(360.0 / 0.0009) = 400000 <- correct It returned ZERO rather than a plausible wrong number: 0.324 truncates to 0 for any encoder finer than about 0.0028 deg/count. Cross-checked against a beamline measurement - 2000 counts is about 1.8 degrees, and 2000 x 0.0009 is exactly 1.8, which fixes the units independently of any docstring. Nothing reads the derived signal yet, so no plan was affected. THE TEST NOW PINS THE INVARIANT, NOT THE OUTPUT. It asserts that a revolution's worth of counts times the size of a count is 360 degrees, within one count of truncation. The previous test in this branch asserted specific outputs and passed the wrong formula happily - which is how a test can make a defect harder to fix rather than easier. Verified by reintroducing the multiply: 3 of the 4 cases fail. The fourth is ERES 1.0, where multiplying and dividing agree and no test could tell them apart. Suite: 148 passed. Type diagnostics unchanged at 73.

Copilot

Copilot review overview

🟢 Approval recommended

The changes are coherent with the PR goals (restoring collection and expanding mock-test coverage) and the only noted feedback is a minor test-name grammar nit.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Use plural agreement: “velocity and distance are allowed”

tests/detectors/test_detector_utils.py:11

Grammar: when referring to two items (“velocity” and “distance”), the test name should use “are allowed” rather than “is allowed”.

Jakub: "I don't think we need a factory function here. There's only one of these detectors." His call, and a fair one - a factory parameterised by detector number is over-general for a single instrument. Removes src/hextools/detectors/perkin_elmer.py and its tests, and restores the inline ContAcqDetector construction in profiles/collection.py exactly as it was. Done surgically rather than by reverting 8a434ec, because that commit also carries the Kinetix tests, which are not in question. Verified I added no lint findings by diffing the finding SET for collection.py before and after, not the count. WHAT THIS COSTS, stated so it is not lost: the Perkin Elmer is back to zero test coverage. The factory existed only because the PE's wiring lives in profiles/collection.py, which pytest excludes, so there was no hextools code to test. Removing it does not change that - it just stops working around it. The real question is whether the profile should be reachable from tests at all, which is a bigger decision than this PR. Suite 145 collected, 144 passed; the one failure is the pre-existing radiography flake, which fired locally here for the first time today after passing 10/10 this morning.

Copilot

Copilot review overview

🔵 Needs a closer look

One updated Phantom error-path message can still raise an unintended exception in an edge case (empty cines) and should be made robust before approval.

Review effort: Lite
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Handle empty cines before calling min() or max()

src/hextools/detectors/phantom.py:526

This ValueError message uses min()/max() on self.driver.cines; if PhantomIO is ever constructed with num_cines=0 (or cines is otherwise empty), min/max will raise before your intended error is raised. Using a safe default avoids an unexpected exception in that edge case.

Correct test name grammar for multiple parameters

tests/detectors/test_detector_utils.py:11

Grammar: the test name refers to two parameters, so "are allowed" reads correctly and avoids a minor wording error in the public test suite.

This test has been failing intermittently since at least 2026-09-18 and is the only thing keeping this branch's CI red. It failed locally 1 run in 5, on CI it failed on a DIFFERENT PAIR of Python versions each run (3.11+3.13 one run, 3.11+3.12 the next, with the third passing), and it fired locally here today after passing 10 consecutive runs this morning. CAUSE, recorded 2026-09-18 and confirmed here: bps.repeat emits a sleep only when delay-minus-elapsed is still positive, so whether a sleep appears at all is a race against how fast the machine ran the acquisition. FIXED BY REMOVING THE RACE, not by loosening the assertion. _FrozenClock stands in for the time module bps.repeat measures elapsed time with, so the full delay always survives. The only two uses of time in bluesky.plan_stubs are the pair inside that calculation, so nothing else is affected. 20 consecutive runs pass. AND IT WAS ASSERTING THE WRONG NUMBER. With the race gone the count is num_acquisitions, not num_acquisitions - 1: take_radiograph passes a SCALAR delay to bp.count, which becomes itertools.repeat - an iterator that never exhausts - so a sleep is emitted after every acquisition INCLUDING THE LAST. The old assertion only passed when timing happened to swallow exactly one sleep, which is why it failed in both directions - 3 observed on 09-18, 0 on CI, 5 here. That trailing wait is real behaviour, not a test artefact: the plan waits time_gap once more after the final frame. Harmless at 0.01s, a real cost for any sizeable gap. Recorded rather than changed - altering the plan is out of scope for this branch. The magnitude assertion is now exact rather than "<= wait", so if bluesky ever measures elapsed time some other way this fails loudly instead of quietly going back to being a race. Suite: 145 passed.

Copilot

Copilot review overview

🟢 Approval recommended

The changes appear correct and well-covered by updated/new tests, with only minor non-blocking suggestions on avoiding unnecessary RE fixture construction in a couple of tests.

Review effort: Lite
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Remove unused RE fixture from kinetix

tests/detectors/test_kinetix.py:16

The kinetix fixture requests the RE fixture but never uses it, which needlessly constructs a RunEngine + event loop thread. Remove the unused dependency to keep tests lighter.

This issue also appears on line 28 of the same file.

Remove unused RE fixture from storage_ring

tests/test_machine.py:8

The storage_ring fixture requests the RE fixture but never uses it, which needlessly constructs a RunEngine + event loop thread for this test. Drop the unused RE dependency to keep the test lighter and faster.

All 23 diagnostics CI reports, in four groups. FOURTEEN: set_mock_value given 1 or 0 for signals typed SignalR[bool] - waiting_for_trigger, trigger_received, complete_and_valid. Now True/False. These worked because the mock stores whatever it is handed; the types were simply a lie about what the IOC reports. ONE: PhantomDetector has no attribute .proc as far as any checker can tell, because AreaDetector attaches plugins with setattr. Silenced with an explicit ty:ignore and a reason, NOT worked around - getattr would only have traded the type error for ruff's get-attr-with-constant, which is how a count goes down while nothing improves. TWO: Slits("XF:TEST:", 1) passed an int where the constructor takes a str name. Now "slits". Tests unaffected - 39 passed. THREE: ignore comments in a syntax ty does not honour. "# type: ignore (TODO: ...)" with a parenthetical, and two mypy-style "# type: ignore[code]", became "# ty: ignore[code]". These were meant to silence exactly the errors they were failing to silence. MOSTLY NOT MINE. Only the phantom ones sit in code this branch added; the Slits, utils and test_utils ones are pre-existing on main. Fixed anyway because the job is red either way and they are small - say if you would rather they were split out. Verified I added no lint findings by diffing the finding SET for the touched files, not the count. Suite: 145 passed.

CI reported the previous commit's directive as unused: there is no diagnostic on that line to suppress. The original problem was only the comment's syntax - '# type: ignore (TODO: ...)' with a parenthetical, which ty could not parse and reported on. With it gone the line is clean, and the TODO moves above the statement where it does not fight the line length. Type diagnostics in CI: 23 -> 1 -> expected 0.

Copilot

Copilot review overview

🔵 Needs a closer look

The updated encoder counts-per-revolution logic in RotationMotor introduces a potential ZeroDivisionError without input validation and should be hardened before approval.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Validate encoder resolution before division

src/hextools/motors.py:144

get_encoder_counts_per_rev() now divides by encoder_resolution, which will raise a ZeroDivisionError (or return nonsense for negative values) if the motor reports 0 or an invalid step size. Since this behavior changed from multiplication to division in this PR, it would be safer to validate the input and raise a clear ValueError.

Copilot

Copilot review overview

🟡 Changes recommended

PhantomAcquireLogic.start_acquiring() can still spin/hang if observe_value() ever terminates normally without yielding a trigger, so the trigger-wait loop should explicitly treat stream exhaustion as a retryable timeout.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Handle exhausted observe_value stream as timeout · New

+        got_trigger = False
+        while not got_trigger:
            try:
                async for trigger_received in observe_value(
                    self.driver.trigger_received, done_timeout=DEFAULT_TIMEOUT


All 61, and pre-commit now passes every hook. MOSTLY MECHANICAL: 24 auto-fixable (import ordering, unused imports outside the profile, blank lines, trailing whitespace), 12 long lines wrapped, 11 missing docstrings written - the ten GeRM enums now say what each mode is for rather than just naming it, which is worth having on its own. THREE JUDGEMENT CALLS, all kept rather than "fixed": 1. profiles/collection.py imports. A beamline profile's "unused" imports are frequently the names staff type at the prompt, and deleting them would break an operator's session with a NameError. Checked each: bp, bps, bpp and show_docs are namespace re-exports; NDStatsIO, PluginSignalDataLogic and SuspendFloor are referenced by commented-out blocks that are clearly meant to come back. All kept, each with the reason on the noqa. Only Path and KinetixDetector were removed - neither appears anywhere in the file, not even in a comment. 2. FOV_2_4_mm_Camera and FOV_20_40_mm_Camera do not match CapWords, and the names state the field of view in millimetres. Renaming would churn the profile and every caller to satisfy a convention the names are breaking deliberately. Silenced with the reason. 3. perkin_elmer._name is a private poke with a TODO already explaining it. Silenced pointing at that TODO rather than removing a workaround nobody has replaced. Also moved FRAME_PERIOD_MARGIN below the imports, where it stops being a module-level statement before an import block, added it to __all__ since radiography.py imports it, and left its TODO intact: it is still one constant carrying the Kinetix value while three other detectors want their own. Suite: 145 passed. Verified the profile still imports every name staff use.

Copilot

Copilot review overview

🟡 Changes recommended

There are CI-relevant maintainability/type-check concerns (notably the RunEngine metadata typing and union-annotation formatting) that should be addressed before merging.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

Resolve RunEngine metadata mapping type-check error · New
Handle exhausted observe_value stream as timeout
Keep shutter union annotations on one line · New

+    # TODO: loosen the type of RE.md to Mapping rather than dict.
+    return RunEngine(RedisJSONDict(open_redis_client(redis_ssl=True), ""))


+    # Whether to open and check the photon shutter during the scan
+    use_shutter: bool = False,
+    fe_shutter: Shutter
+    | None = None,  # Front-end shutter to check before opening the photon shutter
+    photon_shutter: Shutter
+    | None = None,  # Photon shutter to open/close around the acquisition


Anthony Sligar (sligara7) added 7 commits September 21, 2026 10:47

Copilot AI lite review requested due to automatic review settings September 21, 2026 15:04

Copilot started reviewing on behalf of Anthony Sligar (sligara7) September 21, 2026 15:04 View session