Compatibility with ophyd-async v0.3a4 #525

callumforrester · 2024-05-13T12:48:06Z

Update all compatibility issues with ophyd-async v0.3a4

Handle renaming of sim to mock
Use ophyd-async's new soft signals, replacing dodal's home-grown ones

Instructions to reviewer on how to test:

Confirm unit tests pass

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly

olliesilvester

Looks good, but I'm mainly trusting the unit tests! I'll make a separate issue regarding my comment

olliesilvester · 2024-05-13T14:13:56Z

src/dodal/devices/oav/pin_image_recognition/__init__.py

            NDArray[np.uint32], "triggered_top_edge", self.name
        )
-        self.triggered_bottom_edge = create_soft_signal_r(
+        self.triggered_bottom_edge, _ = soft_signal_r_and_setter(


As discussed, we can make a separate issue to make sure these signals use their setters rather than the backends in order to set values. Also, there may be scope to have a regular soft_signal_r function in ophyd-async

evalott100 · 2024-05-13T14:57:51Z

src/dodal/devices/webcam.py

+        self.filename = soft_signal_rw(str, "filename", "webcam")
+        self.directory = soft_signal_rw(str, "directory", "webcam")
+        self.last_saved_path = soft_signal_rw(str, "last_saved_path", "webcam")


Suggested change

self.filename = soft_signal_rw(str, "filename", "webcam")

self.directory = soft_signal_rw(str, "directory", "webcam")

self.last_saved_path = soft_signal_rw(str, "last_saved_path", "webcam")

self.filename = soft_signal_rw(str, name="filename")

self.directory = soft_signal_rw(str, name="directory")

self.last_saved_path = soft_signal_rw(str, name="last_saved_path")

soft_signal_rw has different arguments to create_soft_signal_rw.

We don't need to provide the source prefix to soft_signal_rw

Concerning that that didn't make any tests fail...

Slightly, I'm not overly surprised that the initial_value isn't covered in tests. I don't think that matters for quite a few signals. We should probably cover the names better though.

It's also concerning that the type checker doesn't catch soft_signal_rw(int, initial_value="string") as a problem.

Hmm, I did this in part to use #497 because the initial values weren't working properly in my tests for the Undulator, and I decided to upgrade before tackling the problem. Is it possible that initial_value never worked?

I think they were fixed in bluesky/ophyd-async#266

DominicOram · 2024-05-13T16:24:53Z

I think this Fixes #482 and Fixes #497

DominicOram

Thanks. Mostly echoing @evalott100's comment but in some additional places too

DominicOram · 2024-05-13T16:31:19Z

src/dodal/devices/tetramm.py

-        self.range = ad_rw(TetrammRange, prefix + "Range")
-        self.sample_time = ad_r(float, prefix + "SampleTime")
+        self.range = epics_signal_rw(TetrammRange, prefix + "Range")
+        self.sample_time = epics_signal_r(float, prefix + "SampleTime")

-        self.values_per_reading = ad_rw(int, prefix + "ValuesPerRead")
-        self.averaging_time = ad_rw(float, prefix + "AveragingTime")
-        self.to_average = ad_r(int, prefix + "NumAverage")
-        self.averaged = ad_r(int, prefix + "NumAveraged")
+        self.values_per_reading = epics_signal_rw(int, prefix + "ValuesPerRead")
+        self.averaging_time = epics_signal_rw(float, prefix + "AveragingTime")
+        self.to_average = epics_signal_r(int, prefix + "NumAverage")
+        self.averaged = epics_signal_r(int, prefix + "NumAveraged")

-        self.acquire = ad_rw(bool, prefix + "Acquire")
+        self.acquire = epics_signal_rw(bool, prefix + "Acquire")

        # this PV is special, for some reason it doesn't have a _RBV suffix...
        self.overflows = epics_signal_r(int, prefix + "RingOverflows")

-        self.num_channels = ad_rw(TetrammChannels, prefix + "NumChannels")
-        self.resolution = ad_rw(TetrammResolution, prefix + "Resolution")
-        self.trigger_mode = ad_rw(TetrammTrigger, prefix + "TriggerMode")
-        self.bias = ad_rw(bool, prefix + "BiasState")
-        self.bias_volts = ad_rw(float, prefix + "BiasVoltage")
-        self.geometry = ad_rw(TetrammGeometry, prefix + "Geometry")
+        self.num_channels = epics_signal_rw(TetrammChannels, prefix + "NumChannels")
+        self.resolution = epics_signal_rw(TetrammResolution, prefix + "Resolution")
+        self.trigger_mode = epics_signal_rw(TetrammTrigger, prefix + "TriggerMode")
+        self.bias = epics_signal_rw(bool, prefix + "BiasState")
+        self.bias_volts = epics_signal_rw(float, prefix + "BiasVoltage")
+        self.geometry = epics_signal_rw(TetrammGeometry, prefix + "Geometry")


Must: I think the equivalent of ad_rw is epics_signal_rw_rbv see https://github.com/bluesky/ophyd-async/pull/274/files

Hmm, have done this but the PVs seem to be wrong now, will investigate

DominicOram · 2024-05-13T16:35:40Z

src/dodal/devices/oav/pin_image_recognition/__init__.py

+        self.preprocess_operation = soft_signal_rw(int, "preprocess", self.name)
+        self.preprocess_ksize = soft_signal_rw(int, "preprocess_ksize", self.name)
+        self.preprocess_iterations = soft_signal_rw(
            int, "preprocess_iterations", self.name
        )
-        self.canny_upper_threshold = create_soft_signal_rw(
-            int, "canny_upper", self.name
-        )
-        self.canny_lower_threshold = create_soft_signal_rw(
-            int, "canny_lower", self.name
-        )
-        self.close_ksize = create_soft_signal_rw(int, "close_ksize", self.name)
-        self.close_iterations = create_soft_signal_rw(
-            int, "close_iterations", self.name
-        )
-        self.scan_direction = create_soft_signal_rw(int, "scan_direction", self.name)
-        self.min_tip_height = create_soft_signal_rw(int, "min_tip_height", self.name)
-        self.validity_timeout = create_soft_signal_rw(
-            float, "validity_timeout", self.name
-        )
+        self.canny_upper_threshold = soft_signal_rw(int, "canny_upper", self.name)
+        self.canny_lower_threshold = soft_signal_rw(int, "canny_lower", self.name)
+        self.close_ksize = soft_signal_rw(int, "close_ksize", self.name)
+        self.close_iterations = soft_signal_rw(int, "close_iterations", self.name)
+        self.scan_direction = soft_signal_rw(int, "scan_direction", self.name)
+        self.min_tip_height = soft_signal_rw(int, "min_tip_height", self.name)
+        self.validity_timeout = soft_signal_rw(float, "validity_timeout", self.name)


Must: As @evalott100 has said below, the arguments here have changed and we're now actually making signals with an initial_value of e.g. "canny_upper".

Should: For bonus points could we use the initial values as in connect? I attempted to do this before but hit bluesky/ophyd-async#266, which shouldn't be a problem now

DominicOram · 2024-05-13T16:38:11Z

src/dodal/devices/zocalo/zocalo_results.py

+        self.results, _ = soft_signal_r_and_setter(
+            list[XrcResult], "results", self.name
+        )
+        self.centres_of_mass, _ = soft_signal_r_and_setter(
            NDArray[np.uint64], "centres_of_mass", self.name
        )
-        self.bbox_sizes = create_soft_signal_r(
+        self.bbox_sizes, _ = soft_signal_r_and_setter(
            NDArray[np.uint64], "bbox_sizes", self.name
        )
-        self.ispyb_dcid = create_soft_signal_r(int, "ispyb_dcid", self.name)
-        self.ispyb_dcgid = create_soft_signal_r(int, "ispyb_dcgid", self.name)
+        self.ispyb_dcid, _ = soft_signal_r_and_setter(int, "ispyb_dcid", self.name)
+        self.ispyb_dcgid, _ = soft_signal_r_and_setter(int, "ispyb_dcgid", self.name)


Must: Again, the signature is different

This really ought to be caught by a test

DominicOram

Thanks, couple more commenst in the code. I think the OAV is broken for i22's system tests on main but otherwise python tests/system_tests/test_i22_system.py should pass

DominicOram · 2024-05-14T12:06:24Z

src/dodal/devices/tetramm.py

+        self.bias = epics_signal_rw_rbv(bool, prefix + "BiasState")
+        self.bias_volts = epics_signal_rw_rbv(float, prefix + "BiasVoltage")
+        self.geometry = epics_signal_rw_rbv(TetrammGeometry, prefix + "Geometry")
+        self.nd_attributes_file = epics_signal_rw_rbv(str, prefix + "NDAttributesFile")


Must: nd_attributes_file was just a epics_signal_rw before so should not move to a epics_signal_rw_rbv.

DominicOram · 2024-05-14T12:14:21Z

src/dodal/devices/tetramm.py

+        self.range = epics_signal_rw_rbv(TetrammRange, prefix + "Range")
+        self.sample_time = epics_signal_r(float, prefix + "SampleTime")

-        self.values_per_reading = ad_rw(int, prefix + "ValuesPerRead")
-        self.averaging_time = ad_rw(float, prefix + "AveragingTime")
-        self.to_average = ad_r(int, prefix + "NumAverage")
-        self.averaged = ad_r(int, prefix + "NumAveraged")
+        self.values_per_reading = epics_signal_rw_rbv(int, prefix + "ValuesPerRead")
+        self.averaging_time = epics_signal_rw_rbv(float, prefix + "AveragingTime")
+        self.to_average = epics_signal_r(int, prefix + "NumAverage")
+        self.averaged = epics_signal_r(int, prefix + "NumAveraged")

-        self.acquire = ad_rw(bool, prefix + "Acquire")
+        self.acquire = epics_signal_rw_rbv(bool, prefix + "Acquire")

        # this PV is special, for some reason it doesn't have a _RBV suffix...
        self.overflows = epics_signal_r(int, prefix + "RingOverflows")

-        self.num_channels = ad_rw(TetrammChannels, prefix + "NumChannels")
-        self.resolution = ad_rw(TetrammResolution, prefix + "Resolution")
-        self.trigger_mode = ad_rw(TetrammTrigger, prefix + "TriggerMode")
-        self.bias = ad_rw(bool, prefix + "BiasState")
-        self.bias_volts = ad_rw(float, prefix + "BiasVoltage")
-        self.geometry = ad_rw(TetrammGeometry, prefix + "Geometry")
-        self.nd_attributes_file = epics_signal_rw(str, prefix + "NDAttributesFile")
+        self.num_channels = epics_signal_rw_rbv(TetrammChannels, prefix + "NumChannels")
+        self.resolution = epics_signal_rw_rbv(TetrammResolution, prefix + "Resolution")
+        self.trigger_mode = epics_signal_rw_rbv(TetrammTrigger, prefix + "TriggerMode")
+        self.bias = epics_signal_rw_rbv(bool, prefix + "BiasState")
+        self.bias_volts = epics_signal_rw_rbv(float, prefix + "BiasVoltage")
+        self.geometry = epics_signal_rw_rbv(TetrammGeometry, prefix + "Geometry")
+        self.nd_attributes_file = epics_signal_rw_rbv(str, prefix + "NDAttributesFile")


Must: Sorry, I should have caught this before, epics_signal_r is not the same as ad_r as ad_r appends _RBV on to the PV. This means you get:

(.venv) [ffv81422@ws455 dodal]$ python tests/system_tests/test_i22_system.py Making all i22 devices Traceback (most recent call last): File "/scratch/ffv81422/hyperion/dodal/tests/system_tests/test_i22_system.py", line 22, in <module> make_all_devices(i22) File "/scratch/ffv81422/hyperion/dodal/src/dodal/utils.py", line 128, in make_all_devices devices: dict[str, AnyDevice] = invoke_factories(factories, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/scratch/ffv81422/hyperion/dodal/src/dodal/utils.py", line 153, in invoke_factories devices[dependent_name] = factories[dependent_name](**params, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/scratch/ffv81422/hyperion/dodal/src/dodal/beamlines/i22.py", line 84, in it return device_instantiation( ^^^^^^^^^^^^^^^^^^^^^ File "/scratch/ffv81422/hyperion/dodal/src/dodal/utils.py", line 101, in wrapper return func(*args, **kwds) ^^^^^^^^^^^^^^^^^^^ File "/scratch/ffv81422/hyperion/dodal/src/dodal/beamlines/beamline_utils.py", line 112, in device_instantiation _wait_for_connection(device_instance, mock=fake) File "/scratch/ffv81422/hyperion/dodal/src/dodal/beamlines/beamline_utils.py", line 55, in _wait_for_connection call_in_bluesky_event_loop( File "/scratch/ffv81422/hyperion/hyperion/.venv/lib/python3.11/site-packages/bluesky/run_engine.py", line 2782, in call_in_bluesky_event_loop return fut.result(timeout=timeout) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/dls_sw/apps/python/miniforge/4.10.0-0/envs/python3.11/lib/python3.11/concurrent/futures/_base.py", line 456, in result return self.__get_result() ^^^^^^^^^^^^^^^^^^^ File "/dls_sw/apps/python/miniforge/4.10.0-0/envs/python3.11/lib/python3.11/concurrent/futures/_base.py", line 401, in __get_result raise self._exception File "/scratch/ffv81422/hyperion/hyperion/.venv/lib/python3.11/site-packages/ophyd_async/core/utils.py", line 99, in wait_for_connection raise NotConnected(exceptions) ophyd_async.core.utils.NotConnected: coros: NotConnected: drv: NotConnected: sample_time: NotConnected: ca://BL22I-EA-TTRM-02:DRV:SampleTime to_average: NotConnected: ca://BL22I-EA-TTRM-02:DRV:NumAverage averaged: NotConnected: ca://BL22I-EA-TTRM-02:DRV:NumAveraged

Because they should be reading SampleTime_RBV etc.

callumforrester · 2024-05-14T15:16:45Z

@DominicOram the OAV bug was fixed in bluesky/ophyd-async#289, have applied the tetramm fixes and now the system test is happy:

(venv) vid18871@pc0101:/scratch/vid18871/projects/dodal# python tests/system_tests/test_i22_system.py 
Making all i22 devices
Successfully connected

Stale

DominicOram

Great, thank you. Going to merge as this is breaking other things

DominicOram · 2024-05-14T18:45:43Z

FYI, I believe this has slowed down are tests quite a bit #533

Compatibility with ophyd-async v0.3a4

c0e5b3a

callumforrester requested review from DominicOram, d-perl and keithralphs May 13, 2024 12:50

olliesilvester self-requested a review May 13, 2024 14:12

olliesilvester approved these changes May 13, 2024

View reviewed changes

callumforrester mentioned this pull request May 13, 2024

Make soft signals use their setters rather than the backends #526

Closed

olliesilvester mentioned this pull request May 13, 2024

Compatibility with ophyd async 0.3a4 DiamondLightSource/hyperion#1375

Merged

evalott100 assigned evalott100 and unassigned evalott100 May 13, 2024

evalott100 previously requested changes May 13, 2024

View reviewed changes

This was linked to issues May 13, 2024

Use new ophyd_async soft signal helpers #482

Closed

Implement the ophyd-async sim to mock changes in dodal #497

Closed

DominicOram requested changes May 13, 2024

View reviewed changes

callumforrester mentioned this pull request May 14, 2024

Test Soft Signals Have Expected Names and Values #527

Open

callumforrester added 4 commits May 14, 2024 11:47

Fix soft_signal_rw signature

b493a6d

Fix signature of soft_signal_r_and_setter calls

42d2baa

Fix initial values for pin image recognition

9630a0d

Use correct signal type for tetramm

d058794

callumforrester requested review from DominicOram, olliesilvester and evalott100 May 14, 2024 11:04

Fix broken import

d1dea19

DominicOram requested changes May 14, 2024

View reviewed changes

callumforrester added 2 commits May 14, 2024 16:05

Fix wrong PV on tetramm

0f54b62

Fix readback signals in tetramm

4f088fd

callumforrester force-pushed the ophyd-async-v0.3a4-compat branch from 45f08c0 to 4f088fd Compare May 14, 2024 15:15

callumforrester requested a review from DominicOram May 14, 2024 15:17

DominicOram approved these changes May 14, 2024

View reviewed changes

DominicOram merged commit 141e7c6 into main May 14, 2024
11 checks passed

DominicOram deleted the ophyd-async-v0.3a4-compat branch May 14, 2024 18:33

DominicOram mentioned this pull request May 14, 2024

Improve test speed #533

Closed

This was referenced May 15, 2024

473 create a device for xpress3 areadetector #524

Merged

Add NeXus-friendly soft signals to Undulator #511

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility with ophyd-async v0.3a4 #525

Compatibility with ophyd-async v0.3a4 #525

callumforrester commented May 13, 2024 •

edited

Loading

olliesilvester left a comment

olliesilvester May 13, 2024

evalott100 May 13, 2024

callumforrester May 13, 2024

DominicOram May 13, 2024

callumforrester May 14, 2024

DominicOram May 14, 2024

DominicOram commented May 13, 2024

DominicOram left a comment

DominicOram May 13, 2024

callumforrester May 14, 2024

DominicOram May 13, 2024

DominicOram May 13, 2024

DominicOram May 13, 2024

d-perl May 14, 2024

callumforrester May 14, 2024

DominicOram left a comment

DominicOram May 14, 2024

DominicOram May 14, 2024

callumforrester commented May 14, 2024 •

edited

Loading

DominicOram left a comment •

edited

Loading

DominicOram commented May 14, 2024

Compatibility with ophyd-async v0.3a4 #525

Compatibility with ophyd-async v0.3a4 #525

Conversation

callumforrester commented May 13, 2024 • edited Loading

Instructions to reviewer on how to test:

Checks for reviewer

olliesilvester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram commented May 13, 2024

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callumforrester commented May 14, 2024 • edited Loading

DominicOram left a comment • edited Loading

Choose a reason for hiding this comment

DominicOram commented May 14, 2024

callumforrester commented May 13, 2024 •

edited

Loading

callumforrester commented May 14, 2024 •

edited

Loading

DominicOram left a comment •

edited

Loading