Fixes for ophyd async types mega merge #858

rtuck99 · 2024-10-22T13:39:32Z

Fixes breakages in dodal for bluesky/ophyd-async#594

~~Note this fix depends on #814~~
Note this fix now incorporates #814
See also mx-bluesky PR DiamondLightSource/mx-bluesky#593

Instructions to reviewer on how to test:

Unit tests pass
Check the following changes:
- Usage of StrictEnum/SubsetEnum is appropriate
- Conversion of signals to numpy array types is appropriate

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
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

codecov · 2024-10-23T16:05:50Z

Codecov Report

Attention: Patch coverage is 97.57576% with 4 lines in your changes missing coverage. Please review.

Project coverage is 95.60%. Comparing base (8640900) to head (5ce9e43).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/dodal/devices/fluorescence_detector_motion.py	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
- Coverage   95.61%   95.60%   -0.02%     
==========================================
  Files         125      125              
  Lines        5246     5229      -17     
==========================================
- Hits         5016     4999      -17     
  Misses        230      230

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stan-dot · 2024-10-28T11:03:57Z

src/dodal/common/beamlines/beamline_utils.py

+    uri = kwargs.get("uri")
+    uri_or_prefix = uri or prefix
+    kwargs_with_prefix_or_uri = dict(kwargs)


please better naming or typing, that uri_or_prefix: str for clarity

DominicOram

Great, thanks, a few comments in code. Additionally, I'm getting some errors on dodal connect i03:

Traceback (most recent call last):
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/ophyd_async/core/_device.py", line 146, in connect
    await self._connect_task
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/ophyd_async/core/_signal.py", line 66, in connect
    await self.backend.connect(timeout)
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/ophyd_async/epics/signal/_aioca.py", line 264, in connect
    self.converter = make_converter(self.datatype, self.initial_values)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/ophyd_async/epics/signal/_aioca.py", line 209, in make_converter
    raise TypeError(
TypeError: BL03I-EA-XSP3-01:ARR1:ArrayData with inferred datatype Array1D[np.float64] cannot be coerced to numpy.ndarray[typing.Any, numpy.dtype[numpy.float64]]

thawer: TypeError: Cannot set the parent of <ophyd_async.core._signal.SignalRW object at 0x7fe77afded90> to be <dodal.devices.thawer.ThawingTimer object at 0x7fe77b0146d0>: it is already a child of <dodal.devices.thawer.Thawer object at 0x7fe77aff5990>
panda: TypeError: HDFPanda.__init__() got an unexpected keyword argument 'uri'

I think the other errors from dodal connect i03 are related to real hardware being off. I'm also seeing errors on dodal connect i22 which I think are related to this. Basically, it would be good to go through all the beamlines and try dodal connect with this. Annoyingly dodal connect --all is broken (#875) so this will be a painful job.

I haven't dug into the changes for #814 (comment) as I will need to review in conjunction with DiamondLightSource/mx-bluesky#475

DominicOram · 2024-10-30T11:03:11Z

src/dodal/devices/attenuator.py

@@ -24,23 +24,23 @@ class Attenuator(StandardReadable, Movable):
    read from the device it is also fractional"""

    def __init__(self, prefix: str, name: str = ""):
-        self._calculated_filter_states: DeviceVector[SignalR[int]] = DeviceVector(
+        self.calculated_filter_states: DeviceVector[SignalR[int]] = DeviceVector(


Should: Can we put all the signals in here back to private? I think we don't want people accidentally using them rather than the set of this Device

DominicOram · 2024-10-30T11:14:14Z

src/dodal/devices/aperturescatterguard.py

@@ -27,7 +27,7 @@ class AperturePosition(BaseModel):
    aperture_z: float
    scatterguard_x: float
    scatterguard_y: float
-    radius: float | None = Field(json_schema_extra={"units": "µm"}, default=None)
+    radius: float = Field(json_schema_extra={"units": "µm"}, default=0.0)


Should: We need to be a bit careful here as in the robot load position no aperture is selected and so asking what the aperture radius is is meaningless. I think using 0 to signify it is fine, or we could maybe use float('nan') but we should probably write it in some docstrings here and be aware of it in the downstream code

DominicOram · 2024-10-30T12:02:02Z

src/dodal/devices/focusing_mirror.py

+        self.actual_v = epics_signal_r(int, prefix + "R")
+        self.setpoint_v = epics_signal_rw(int, prefix + "D")
+        self.demand_accepted = epics_signal_r(MirrorVoltageDemand, prefix + "DSEV")


Should: As above, I think keep these private to avoid people setting then directly

DominicOram · 2024-10-30T12:09:05Z

src/dodal/devices/oav/oav_to_redis_forwarder.py

            {
                Source.ROI.value: OAVSource(f"{prefix}MJPG:", "roi"),
                Source.FULL_SCREEN.value: OAVSource(f"{prefix}XTAL:", "fullscreen"),
            }
        )
-        self.selected_source = soft_signal_rw(Source)
+        self.selected_source = soft_signal_rw(int)


Should: I think we should probably make this a string enum then. It will tidy up the log messages etc. too

Unfortunately the integer is required for the use of DeviceVector

DominicOram · 2024-10-30T12:22:01Z

src/dodal/devices/oav/utils.py

        timeout = yield from bps.rd(ophyd_pin_tip_detection.validity_timeout)
        raise PinNotFoundException(f"No pin found after {timeout} seconds")

-    return found_tip  # type: ignore
+    return Pixel((int(found_tip[0]), int(found_tip[1])))  # type: ignore


Nit: I don't think we need the type ignore now?

DominicOram · 2024-10-30T12:22:56Z

src/dodal/devices/zebra_controlled_shutter.py

@@ -45,7 +44,7 @@ async def set(self, value: ZebraShutterState):
            raise UserWarning(
                f"Tried to set shutter to {value.value} but the shutter is in auto mode."
            )
-        await self._manual_position_setpoint.set(value)
+        await self.manual_position_setpoint.set(value)


Should: I think we should keep this private to encourage the set to be used

DominicOram · 2024-10-30T12:44:52Z

tests/devices/i22/test_dcm.py

+@pytest.mark.skip(
+    reason="https://github.com/bluesky/ophyd-async/pull/594 assert_configuration() on numpy arrays is "
+    "broken"
+)


Should: Is this still true? If so it needs a link to an open issue to fix/workaround

Relm-Arrowny · 2024-10-30T14:18:43Z

src/dodal/devices/i10/i10_apple2.py

@@ -196,17 +198,17 @@ def __init__(
        """
        super().__init__(name=name)
        with self.add_children_as_readables():


Suggested change

with self.add_children_as_readables():

I think we can remove this now.
I also have a question, what is the best way to have read behave like it use to? For example in this case, when read is call it will read the readable in both id_ref and pgm_ref as well as i10_apple2.

I have removed it. I think that what you want is probably not going to be supported by ophyd-async because it would make it difficult for users to reason about the expected behaviour of devices if their read behaviour differs from their publicly expressed device/signal hierarchy.

I believe @dperl-dls is going to write a style guide about the best way to structure devices and I think it will probably advise not to have overlapping composite devices.

DominicOram

Thanks, one minor comment but I still need to review the Zocalo changes in the context of DiamondLightSource/mx-bluesky#553

DominicOram · 2024-11-04T13:46:00Z

src/dodal/devices/zebra_controlled_shutter.py

@@ -26,7 +25,7 @@ class ZebraShutter(StandardReadable, Movable):
    Internally in the zebra there are two AND gates, one for manual control and one for
    automatic control. A soft input (aliased to control_mode) will switch between
    which of these AND gates to use. For the manual gate the shutter is then controlled
-    by a different soft input (aliased to _manual_position_setpoint). Both these AND
+    by a different soft input (aliased to manual_position_setpoint). Both these AND


Nit: This doesn't match the signal name now

…private again.

…s where the children were all instances of Reference

rtuck99 force-pushed the mx-bluesky_475_multi_centre_plan branch from 65bf6a3 to 303869f Compare October 22, 2024 13:41

rtuck99 force-pushed the fixes_for_ophyd_async_mega_merge branch 2 times, most recently from 1b5cfd6 to 9b8109d Compare October 23, 2024 15:14

rtuck99 mentioned this pull request Oct 24, 2024

Fixes for ophyd async types mega merge DiamondLightSource/mx-bluesky#593

Merged

DominicOram mentioned this pull request Oct 24, 2024

Catch device creation exceptions in test_device_instantiation #851

Closed

4 tasks

rtuck99 mentioned this pull request Oct 25, 2024

I22 connection tests did not fail when they weren't mocking #850

Closed

rtuck99 force-pushed the fixes_for_ophyd_async_mega_merge branch from def6e22 to 3732d8f Compare October 25, 2024 12:14

stan-dot added bug Something isn't working python Pull requests that update Python code tests labels Oct 28, 2024

stan-dot reviewed Oct 28, 2024

View reviewed changes

rtuck99 force-pushed the fixes_for_ophyd_async_mega_merge branch from 3732d8f to e02138f Compare October 29, 2024 14:41

rtuck99 changed the base branch from mx-bluesky_475_multi_centre_plan to main October 29, 2024 14:43

rtuck99 marked this pull request as ready for review October 30, 2024 11:59

rtuck99 requested review from DominicOram, noemifrisina, dperl-dls, olliesilvester, Relm-Arrowny and callumforrester October 30, 2024 12:01

rtuck99 changed the title ~~DNM Fixes for ophyd async types mega merge~~ Fixes for ophyd async types mega merge Oct 30, 2024

rtuck99 mentioned this pull request Oct 30, 2024

(DiamondLightSource/mx-bluesky#475) Method to allow access to full zocalo results #814

Closed

DominicOram requested changes Oct 30, 2024

View reviewed changes

rtuck99 mentioned this pull request Oct 30, 2024

(#475) Xray-centre and collect on multiple centres DiamondLightSource/mx-bluesky#553

Open

Relm-Arrowny reviewed Oct 30, 2024

View reviewed changes

rtuck99 requested a review from DominicOram October 31, 2024 10:40

DominicOram reviewed Nov 4, 2024

View reviewed changes

rtuck99 force-pushed the fixes_for_ophyd_async_mega_merge branch from 85d8ac0 to 2ed8908 Compare November 4, 2024 16:42

DominicOram mentioned this pull request Nov 4, 2024

Fixes for broken tests caused by declarative epics ophyd-async changes #880

Merged

4 tasks

rtuck99 added 26 commits November 5, 2024 13:18

Fix PMAC to use new Reference wrapper

3a689b2

Fix Thawer to use new Reference wrapper

4ccd3a7

Fix OAV to redis forwarder signal types

a78a9d1

Allow I10 devices to accept float64 from bluesky scan() plan

55422f1

Revert panda uri changes back to prefix

0a21b4c

Misc I10 fixes for ophyd-async type changes

fdb844c

Revert uri/prefix mangling in device_instantiation()

c3ea992

Fix I10 unit tests by flattening devices

04607d2

Make type-checking happy

97b541b

bump ophyd-async hash

6400f4c

Remove nonsensical fgs counter mock set

2f5d152

Bump dodal commit hash

ae06801

Bump ophyd-async hash

b9141bf

Fix connection issue due to typing on xspress3

0fc6d49

Document the aperturescatterguard radius. Make the attenuator fields …

b514a42

…private again.

Make mirror voltage attribs private again

6f2dd4b

Remove unneeded lint ignore

950c64a

Make zebra shutter manual_position_setpoint private again

fe7b63e

Update comment with reference to the correct ophyd-async issue

3cbe079

Remove superfluous calls to add_children_as_readables() in i10 device…

26ba9f5

…s where the children were all instances of Reference

Pin dodal to >=0.8.0a1

542d6d9

Fix typo in zebra_controlled_shutter pydoc. Pin ophyd-async to v0.8.0a2

bf939c2

Fixes for OAV changes

7913741

Make type-checking happy

9f7df7b

Fix OAV changes to work with ophyd-async typing changes

e7066dd

Make type-checking happy

5ce9e43

rtuck99 force-pushed the fixes_for_ophyd_async_mega_merge branch from c5baf3f to 5ce9e43 Compare November 5, 2024 13:18

rtuck99 merged commit 2e58d4e into main Nov 5, 2024
19 checks passed

rtuck99 deleted the fixes_for_ophyd_async_mega_merge branch November 5, 2024 13:23

callumforrester mentioned this pull request Nov 11, 2024

Reinstate DeviceInitializationController #881

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for ophyd async types mega merge #858

Fixes for ophyd async types mega merge #858

rtuck99 commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 23, 2024 •

edited

Loading

stan-dot Oct 28, 2024

DominicOram left a comment

DominicOram Oct 30, 2024

DominicOram Oct 30, 2024

DominicOram Oct 30, 2024

DominicOram Oct 30, 2024

rtuck99 Oct 31, 2024

DominicOram Oct 30, 2024

DominicOram Oct 30, 2024

DominicOram Oct 30, 2024

Relm-Arrowny Oct 30, 2024

rtuck99 Oct 31, 2024

DominicOram left a comment

DominicOram Nov 4, 2024

Fixes for ophyd async types mega merge #858

Fixes for ophyd async types mega merge #858

Conversation

rtuck99 commented Oct 22, 2024 • edited Loading

Instructions to reviewer on how to test:

Checks for reviewer

codecov bot commented Oct 23, 2024 • edited Loading

Codecov Report

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

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

rtuck99 commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 23, 2024 •

edited

Loading