Call ensure_connected on all ophyd_async devices #865

DiamondJoseph · 2024-10-25T07:56:33Z

Fixes #864

Instructions to reviewer on how to test:

Run dodal connect i22 while on the network and able to get those PVs
Ensure that devices are connected
Change the PV prefixes of some devices on i22 to known invalid values
Ensure that dodal connect i22 does not connect to those devices

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}

callumforrester · 2024-11-11T18:22:01Z

This is the correct solution but I think we need to think a bit more about UX in the implementation.

The problem is that we are now blurring the boundaries between instantiation and connection and muddying the error messages. There are 3 paths to the display output depending on device configuration:

flowchart TD
    A[dodal connect] --> B(device_instantiation)
    B --> C{wait_for_connection?}
    C -->|True| D{Connection Error?}
    C -->|False| E[ensure_connected with RE]
    D -->|True| F[Display connection error from dodal]
    D -->|False| G[Display connection success]
    E --> H{Connection Error?}
    H -->|False| G
    H -->|True| I[Display connection error from RE]

Presumably we want to aim for one of these two:

flowchart TD
    A[dodal connect] --> B(device_instantiation)
    B --> C{Instantiation error?}
    C -->|False| D[Display instantiation success]
    D --> E[Attempt connection]
    C -->|True| F[Display instantiation error]
    E --> H{Connection error?}
    H -->|True| I[Display connection error]
    H -->|False| J[Display connection success]

flowchart TD
    A[dodal connect] --> B(device_instantiation)
    B --> C{Instantiation error?}
    C -->|False| E[Attempt connection]
    C -->|True| J
    E --> H{Connection error?}
    H -->|True| I[Display connection error]
    H -->|False| J[Display connection success]

Could do with opinions on which solution we want, also @coretl any opinions on changes needed for ophyd-async? In either case I think I'd want an easier way to introspection connection errors.

coretl · 2024-11-11T21:06:18Z

Here's the code for wait_for_connection: https://github.com/bluesky/ophyd-async/blob/f8fae433e220f8b16774f76928c62e4b71247454/src/ophyd_async/core/_utils.py#L118-L137

Basically asyncio.gather(*[device.connect() for device in devices], return_exceptions=True), then accumulate the errors together. If you want to reassociate the errors with the device I suggest you do the gather yourself, then you can do what you want with the returned Exceptions.

codecov · 2024-11-12T11:30:45Z

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.61%. Comparing base (f896461) to head (4cd1375).

Files with missing lines	Patch %	Lines
src/dodal/cli.py	90.00%	1 Missing ⚠️
src/dodal/utils.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #865      +/-   ##
==========================================
- Coverage   95.63%   95.61%   -0.03%     
==========================================
  Files         128      128              
  Lines        5271     5289      +18     
==========================================
+ Hits         5041     5057      +16     
- Misses        230      232       +2

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

coretl · 2024-11-12T14:19:21Z

If you want to reassociate the errors with the device I suggest you do the gather yourself, then you can do what you want with the returned Exceptions.

Alternatively I can make NotConnected._errors public and you can use that to inspect the error that comes back

callumforrester · 2024-11-13T07:41:38Z

@coretl I think either way I'll make cleaning up the error messages a separate ticket.

Alternatively I can make NotConnected._errors public and you can use that to inspect the error that comes back

That sounds very useful in general, raised an issue: bluesky/ophyd-async#647

callumforrester · 2024-11-13T09:27:07Z

The tests should be fixed by: bluesky/ophyd-async#646

DominicOram

I think comparing with the old version we're missing some errors, the output is also not as pretty. Old:

(.venv) [ffv81422@ws455 dodal]$ dodal connect i03
Attempting connection to i03 (using dodal.beamlines.i03)
25 devices connected:
	aperture_scatterguard
	attenuator
	backlight
	cryo_stream
	detector_motion
	eiger
	flux
	lower_gonio
	mirror_voltages
	oav
	panda_fast_grid_scan
	qbpm
	robot
	s4_slit_gaps
	sample_shutter
	smargon
	synchrotron
	thawer
	undulator
	vfm
	webcam
	xbpm_feedback
	xspress3mini
	zebra
	zebra_fast_grid_scan
Traceback (most recent call last):
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/bin/dodal", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/dodal/src/dodal/cli.py", line 71, in connect
    raise NotConnected(exceptions)
ophyd_async.core._utils.NotConnected: 
zocalo: TypeError: Expected Array1D[dtype], got numpy.ndarray[typing.Any, numpy.dtype[numpy.uint64]]
panda: NotConnected:
    coros: NotConnected: pva://BL03I-EA-PANDA-01:PVI
undulator_dcm: TypeError: Expected Array1D[dtype], got numpy.ndarray[typing.Any, numpy.dtype[numpy.uint64]]
pin_tip_detection: TypeError: Expected Array1D[dtype], got numpy.ndarray[typing.Any, numpy.dtype[numpy.uint32]]
dcm: TypeError: Expected Array1D[dtype], got numpy.ndarray[typing.Any, numpy.dtype[numpy.uint64]]

vs. new:

(.venv) [ffv81422@ws455 mx-bluesky]$ dodal connect i03
Attempting connection to i03 (using dodal.beamlines.i03)
ERROR:bluesky:Run aborted
Traceback (most recent call last):
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/bluesky/run_engine.py", line 1605, in _run
    msg = self._plan_stack[-1].send(resp)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/ophyd_async/plan_stubs/_ensure_connected.py", line 29, in ensure_connected
    raise connect_task.exception()
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/ophyd_async/core/_utils.py", line 145, in wait_for_connection
    raise NotConnected.with_other_exceptions_logged(exceptions)
ophyd_async.core._utils.NotConnected: 
panda: NotConnected: pva://BL03I-EA-PANDA-01:PVI

Traceback (most recent call last):
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/bin/dodal", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/dodal/src/dodal/cli.py", line 67, in connect
    _connect_devices(RE, devices, sim_backend)
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/dodal/src/dodal/cli.py", line 94, in _connect_devices
    RE(ensure_connected(*ophyd_async_devices.values(), mock=sim_backend))
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/bluesky/run_engine.py", line 973, in __call__
    plan_return = self._resume_task(init_func=_build_task)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/bluesky/run_engine.py", line 1109, in _resume_task
    raise exc
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/bluesky/run_engine.py", line 1751, in _run
    raise err
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/bluesky/run_engine.py", line 1605, in _run
    msg = self._plan_stack[-1].send(resp)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/ophyd_async/plan_stubs/_ensure_connected.py", line 29, in ensure_connected
    raise connect_task.exception()
  File "/scratch/ffv81422/mx_bluesky_hyperion_merge/mx-bluesky/.venv/lib/python3.11/site-packages/ophyd_async/core/_utils.py", line 145, in wait_for_connection
    raise NotConnected.with_other_exceptions_logged(exceptions)
ophyd_async.core._utils.NotConnected: 
panda: NotConnected: pva://BL03I-EA-PANDA-01:PVI

Specific issues:

Old style gave me a list of all the devices that were happy, this was good as a sanity check
New style is not telling me about zocalo, dcm or pin_tip_detection errors

Could be because we're not swallowing the errors and then checking subsequent devices?

Also, May be scope creep but I wonder if it's worth just swallowing the stack trace for the connect? It's not helpful to a user

DiamondJoseph mentioned this pull request Oct 25, 2024

Dodal connect on i22 is not actually connecting #864

Open

DiamondJoseph force-pushed the ensure_connect_on_cli branch from 7278a22 to eded5b3 Compare October 25, 2024 08:21

DiamondJoseph mentioned this pull request Oct 25, 2024

Revert device_factory changes #869

Merged

callumforrester force-pushed the ensure_connect_on_cli branch from d7d1e7a to 03ad2f0 Compare November 11, 2024 17:26

callumforrester mentioned this pull request Nov 12, 2024

Reinstate DeviceInitializationController #881

Open

4 tasks

callumforrester mentioned this pull request Nov 13, 2024

Make errors in NotConnected introspectable bluesky/ophyd-async#647

Closed

DiamondJoseph and others added 4 commits November 13, 2024 09:27

Call ensure_connected on all ophyd_async devices

987817e

Start on tests for disconnection case

ec4ed9b

Fix tests in latest version of ophyd-async

56b0ed9

Refactor test cases and add missing case

a28a0b9

callumforrester force-pushed the ensure_connect_on_cli branch from 7384006 to a28a0b9 Compare November 13, 2024 09:27

callumforrester requested a review from DominicOram November 13, 2024 09:27

callumforrester added 2 commits November 13, 2024 09:32

Revert accidental change to pyproject.toml

b550e22

Make all devices lazy in CLI

4cd1375

callumforrester force-pushed the ensure_connect_on_cli branch from 6c57567 to 4cd1375 Compare November 13, 2024 16:17

DominicOram requested changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call ensure_connected on all ophyd_async devices #865

Call ensure_connected on all ophyd_async devices #865

DiamondJoseph commented Oct 25, 2024

callumforrester commented Nov 11, 2024

coretl commented Nov 11, 2024

codecov bot commented Nov 12, 2024 •

edited

Loading

coretl commented Nov 12, 2024

callumforrester commented Nov 13, 2024

callumforrester commented Nov 13, 2024

DominicOram left a comment •

edited

Loading

Call ensure_connected on all ophyd_async devices #865

Are you sure you want to change the base?

Call ensure_connected on all ophyd_async devices #865

Conversation

DiamondJoseph commented Oct 25, 2024

Instructions to reviewer on how to test:

Checks for reviewer

callumforrester commented Nov 11, 2024

coretl commented Nov 11, 2024

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

coretl commented Nov 12, 2024

callumforrester commented Nov 13, 2024

callumforrester commented Nov 13, 2024

DominicOram left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 12, 2024 •

edited

Loading

DominicOram left a comment •

edited

Loading