Mass migration of pyepics scripts to ophyd-async/bluesky - #76
Mass migration of pyepics scripts to ophyd-async/bluesky#76Jakub Wlodek (jwlodek) wants to merge 17 commits into
Conversation
Jakub Wlodek (jwlodek)
commented
Aug 12, 2026
- Update gitignore
- Add motors module remove unused files
- Switch back to pixi from uv. Fix phantom ophyd object with new ophyd-async version. Add classes for Filters and Monochromater. Add plan for changing energies, with auto-tuning of the second crystal pitch
…async version. Add classes for Filters and Monochromater. Add plan for changing energies, with auto-tuning of the second crystal pitch
Anthony Sligar (sligara7)
left a comment
There was a problem hiding this comment.
Thanks Jakub — the change-energy plan is great: the coarse→fine camera-feedback auto-tune of the crystal-2 pitch is exactly the compensation the mono needs for the encoder/rail imperfections, and the approach-from-below for backlash plus waiting on the shutter position status (not the command status) are both real improvements over the pyepics lib. Inline comments below; the two biggest are the ophyd-async version story (0.21 here vs 0.19.4 in hex-ob and the beamline profile env) and making sure the phantom fixes that were just validated against the simulated beamline in hex-ob PR #12 survive the API migration. CI reds understood as the in-flight pixi move.
| self.driver = driver | ||
|
|
||
| async def arm(self): | ||
| async def start_acquiring(self): |
There was a problem hiding this comment.
This migration starts from the pre-hex-ob version of the phantom, so two fixes that were found against the simulated beamline (hex-ob PR #12) don't appear here:
- the external-trigger deadlock chain fix — busy-record arm handling in the acquire path (without it, an externally-triggered tomo flyscan deadlocks; it reproduced identically in sim and would at the beamline);
- the download-watch race fix in the RAM-download flow.
It's possible the 0.21 acquire-logic contract absorbs part of (1) — but that needs to be shown rather than assumed. Suggest porting the hex-ob lib/phantom.py (fixes included) forward to the 0.21 API, rather than migrating the pre-fix version, so the sim-validated behavior is the thing that survives.
| await asyncio.gather(*coros) | ||
|
|
||
|
|
||
| def change_energy( |
There was a problem hiding this comment.
energy: float = 0.0 is a footgun — calling with the default hits np.arcsin(1.977/0.0). Suggest making energy required, and validating energy > 1.977 (arcsin domain for Si(111)) with a clear error before any motor moves.
|
|
||
| # Create a PeakStats object to monitor the fluorescence screen camera signal | ||
| # and find the position of the crystal 2 pitch that produces a peak. | ||
| ps = PeakStats(dclm.xtal2_pitch.name, f"{fs_camera.name}_stats1_mean") |
There was a problem hiding this comment.
f"{fs_camera.name}_stats1_mean" is the ophyd-v1 naming convention — ophyd-async data keys are dash-separated (<name>-stats1-mean), so as written PeakStats never sees the signal, ps.com stays None, and the bps.mv(dclm.xtal2_pitch, ps.com) below fails cryptically.
Two asks: derive the key from the device rather than hardcoding the string, and guard the ps.com is None case (no peak found — e.g. no beam, screen out) with a clear error before any move.
| await wait_for_value(self.in_position, True, timeout=10) | ||
|
|
||
|
|
||
| class Slits(StandardReadable, EpicsDevice): |
There was a problem hiding this comment.
The slit motors are created after super().__init__ with bare suffix strings ("I}Mtr" etc.) — do they actually inherit the device prefix? If child prefix propagation only happens via PvSuffix annotations, these connect to literal PV names like I}Mtr. A test that connects one axis against the mock backend would settle it either way.
| ) | ||
| self.xtal1_pitch = AsyncEpicsMotor(prefix + "C1P}Mtr") | ||
| self.xtal2_pitch = AsyncEpicsMotor(prefix + "C2P}Mtr") | ||
| self.flourescence_screen = AsyncEpicsMotor(prefix + "FS}Mtr") |
There was a problem hiding this comment.
Typo that's about to become public API surface: flourescence_screen → fluorescence_screen (also referenced from the plan below). Cheap to fix now, breaking to fix later.
| return BeamMode.WHITE | ||
| return BeamMode.MONOCHROMATIC | ||
|
|
||
| def _get_energy(self, pitch_angle: float) -> float: |
There was a problem hiding this comment.
The Si(111) constants (35.2544 offset, 1.977 = hc/2d) are duplicated here and in change_energy. Worth hoisting to module-level named constants so the energy readback and the plan can't drift apart.
There was a problem hiding this comment.
Pull request overview
This pull request migrates tooling to Pixi, updates ophyd-async integrations, and adds beamline devices plus tomography/XRD functionality.
Changes:
- Replaces uv-based workflows with Pixi/Hatch packaging and CI.
- Adds motors, detectors, filters, monochromator, flyers, and energy-control plans.
- Expands tomography, calibration, and device tests.
Reviewed changes
Copilot reviewed 24 out of 28 changed files in this pull request and generated 27 comments.
Show a summary per file
| File | Final review findings |
|---|---|
tests/tomography/test_alignment.py |
Moderate: test name collision drops parameterized cases; no-flat assertion is ineffective. Nit: unused import causes lint failure. |
tests/test_utils.py |
No final comments. |
tests/test_photon_delivery_system.py |
No final comments. |
tests/detectors/test_phantom.py |
No final comments. |
tests/conftest.py |
No final comments. |
src/hextools/xrd/calibration.py |
Critical: calibration plan performs no calibration. |
src/hextools/xrd/__init__.py |
No final comments. |
src/hextools/utils.py |
Critical: producer exceptions can leave the consumer blocked indefinitely. |
src/hextools/tomography/alignment.py |
Critical: alignment execution, return values, ellipse fitting, and metadata handling are incomplete or failing; moderate run-management issue. |
src/hextools/profiles/collection.py |
Critical: Kinetix 2–4 reuse Kinetix 1’s prefix. |
src/hextools/plans/tomography.py |
Critical: encoder signal is passed instead of its value; moderate: shutter parameters are unused. |
src/hextools/plans/__init__.py |
No final comments. |
src/hextools/photon_delivery_system.py |
Critical: child PV prefixes, no-camera screen positioning, and auto-tune cleanup are incorrect; moderate energy validation issue; nit: misspelled public attribute. |
src/hextools/motors.py |
Critical: encoder conversion and child motor prefixes are incorrect for optics and sample-tower motors. |
src/hextools/machine.py |
Critical: NSLS2StorageRing does not initialize its base device. |
src/hextools/flyers.py |
Critical: calculate_scan_time rejects every valid argument combination. |
src/hextools/detectors/utils.py |
No final comments. |
src/hextools/detectors/phantom.py |
No final comments. |
src/hextools/detectors/germ.py |
Moderate: acquisition logic remains under the obsolete attribute and will not be invoked. |
src/hextools/_version.pyi |
No final comments. |
pyproject.toml |
Moderate: ophyd-async is unconstrained; required nslsii and redis_json_dict dependencies are undeclared. |
pixi.toml |
No final comments. |
.github/workflows/publish-pypi.yml |
Critical: release workflow uses a mutable setup-pixi tag. |
.github/workflows/pixi-update.yml |
No final comments. |
.github/workflows/ci.yml |
No final comments. |
.github/dependabot.yml |
No final comments. |
Suppressed comments (12)
src/hextools/detectors/utils.py:1
- This new module has no module docstring. The repository selects Ruff's
Drules and runs them in CI, so lint fails on this file; add the module-level documentation before relying on these public utilities.
from ophyd_async.epics.adkinetix import KinetixReadoutMode
src/hextools/flyers.py:131
- When a period is supplied equal to the exposure time, this condition accepts it even though
overheadis added per acquisition; for example, 1.0 s exposure/period with 0.005 s overhead is treated as 1.0 s rather than 1.005 s. That underestimates the time-based flyscan duration and can make the motor/Panda schedule overrun; compare the period directly withexposure_time + overhead.
start_position: float,
src/hextools/machine.py:1
- This new module has no module docstring and
NSLS2StorageRinghas no class docstring. Both are required by the repository's configured RuffDrules, which CI runs over this file, so the lint job will fail.
from ophyd_async.core import StandardReadable
src/hextools/photon_delivery_system.py:263
- Any state that is not the fully-out white configuration is reported as monochromatic, including a mixed state where one component is out and the other is in or moving.
change_energytrusts this signal, so it can start an energy change while the monochromator is not actually in a valid monochromatic configuration.
if xtal1_out and beam_stop_out:
return BeamMode.WHITE
return BeamMode.MONOCHROMATIC
src/hextools/plans/tomography.py:1
- This new module has no module docstring. Ruff's configured
Drules are run by the CI pre-commit hook, so this file is rejected before tests run; add a module docstring (and keep the documented parameter list in sync with the implementation).
from bluesky import plan_stubs as bps
src/hextools/profiles/collection.py:155
kinetix2uses the same EPICS prefix askinetix1, so the two objects control and read the same detector while exposing different names. Resource configuration and acquisition writes will collide; use the Kinetix:2 IOC prefix for this device.
"XF:27ID1-ES{Kinetix:1}", ADWriterFactory.hdf(path_provider), name="kinetix2"
src/hextools/profiles/collection.py:158
kinetix3repeats the Kinetix:1 EPICS prefix used bykinetix1, so this named detector is another object controlling the same hardware. Use the Kinetix:3 IOC prefix here.
"XF:27ID1-ES{Kinetix:1}", ADWriterFactory.hdf(path_provider), name="kinetix3"
src/hextools/profiles/collection.py:161
kinetix4repeats the Kinetix:1 EPICS prefix used bykinetix1, so this named detector is another object controlling the same hardware. Use the Kinetix:4 IOC prefix here.
"XF:27ID1-ES{Kinetix:1}", ADWriterFactory.hdf(path_provider), name="kinetix4"
src/hextools/tomography/alignment.py:408
- The flatfield UID is assigned but never put into the projection run metadata. The alignment test expects the second start document to contain
flat_uid, but this code discards the value, so consumers cannot associate the projection run with its flatfield.
flat_uid = yield from bps.trigger_and_read(dets, name="flatfield")
yield from bps.mvr(sample_stage_x, -base_x_offset)
src/hextools/tomography/alignment.py:1
- This module has no module docstring and several public functions also have none, while the repository's Ruff configuration selects the pydocstyle
Drules and CI runs Ruff over all Python files. The lint job will reject this new module; add the required module/public API documentation or an intentional per-file exemption.
from enum import StrEnum
src/hextools/tomography/alignment.py:13
- This imports
RegionPropertiesfrom the privateskimage.measure._regionpropsmodule without an exemption. The repository explicitly enables RuffPLC2701and CI runs it, so lint will fail here; use a public import if available or add a narrowly scoped exemption/type alternative.
from skimage.measure._regionprops import RegionProperties
src/hextools/xrd/calibration.py:13
xrd_calibrationis a public function without a docstring, and the repository's Ruff configuration selectsD103for public functions. The CI lint hook will reject this new module; document its inputs, scan behavior, and return value along with completing the implementation.
def xrd_calibration(
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "algotom", | ||
| "ipython", | ||
| "numpy", | ||
| "ophyd-async[ca, pva]", | ||
| "rich>=15.0.0", |
| self.driver = GeRMDetectorIO(prefix, name=name) | ||
| self._trigger_logic = GeRMTriggerLogic(self.driver) | ||
| self._arm_logic = GeRMArmLogic(self.driver) | ||
| self._arm_logic = GeRMAcquireLogic(self.driver) |
| fastest_possible_move_time = travel_distance / max_motor_velocity | ||
| total_acq_time = num_images * (exposure_time + acq_time_overhead) | ||
|
|
||
| return max(fastest_possible_move_time, total_acq_time) |
| await asyncio.gather(*(asyncio.ensure_future(forward(a)) for a in aiterables)) | ||
| await queue.put(sentinel) |
| if start_position is None: | ||
| start_position = yield from bps.rd(motor) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>