867 devices in blueapi context

[Tom-Willemsen]

Fixes #867

This refactors our hyperion plans to no longer refer to things like i03.backlight() directly, and instead to pull devices out of a blueapi context.

The main approach at present is that each class defines a composite of devices it needs (as a dataclass), in it's setup method.

as an example, something like:

@dataclasses.dataclass
class SomePlanComposite():
    backlight: Backlight
    eiger: Eiger

In the plan's create_devices, this composite is then instantiated with the appropriate devices from the blueapi context. This composite instance is then passed as one of the arguments to the plan, and relevant devices (or the entire composite) passed down into sub-plans as necessary.

I've tried to design this so that it should be relatively easy to move to blueapi's inject mechanism when we do DiamondLightSource/mx-bluesky#339 (might need a thin shim), as now all of the dependencies of each plan are passed around as explicit arguments rather than implicit and scattered throughout sub-plans.

I've tried to keep the API type-safe for developers where possible, as before, which is why I chose to use a dataclasses in the above format rather than passing around e.g. a Dict[str, Device]. This also lets us check we got a device of the expected type out of the context.

Some API decisions which I'm not necessarily sure about:

• The handling of wait_for_connection=False is a bit hacky, it's currently "known about" in two places, one when you setup the context (which honors the wait_for_connection setting) and one when the device composite is actually created (at which point you're starting a plan and therefore always need the devices to be connected).
  • I think this will likely fall out of BlueAPI: Allow plans to be called with blueapi mx-bluesky#339 once that issue is done, as blueapi will have to be responsible for making sure that any devices it injects are actually connected at the point they're used
  • Unless we want to make the plan itself responsible for this? But that doesn't seem right.
  • Happy to hear any other alternative approaches
• Post-init in device_instantiation (for example - loading aperture positions). At the point when we're building the context we don't know which plan we'll be running but still need to set up the devices. Currently I've moved loading aperture positions into a __post_init__ on the dataclasses described above, so that this logic happens when the relevant device is about to be used.
  • This will likely change again when we do BlueAPI: Allow plans to be called with blueapi mx-bluesky#339 as blueapi will then somehow have to be told about any post-device-creation initialisation that needs to be done.
  • But again, happy to hear alternative approaches on this...

To test

• Ensure that individual plans do not reference i03.<anything> any more.
  • Unit tests might still reference i03, in particular mocks set up using things like mock_backlight = i03.backlight(fake_with_ophyd_sim=True). This seems ok to me as it's just for simulated devices.
• No new s03 test failures. Some tests fail on main and still fail on this branch (e.g. rotation scans, fgs).
• Will need testing on beamline(s). This is a change that touches all plans and isn't possible to test fully from a dev machine as far as I know.

[callumforrester]

Unsure if it's a multi-step process, but I think the eventual aim should be to remove the experiment registry entirely and just replace it with a hierarchy of plans. I.e. there is a top level plan called fast_grid_scan_experiment which takes the correct parameters, sets up the subscriptions via yield from subscribe(...), and calls the actual fast_grid_scan plan.

The philosophy here is that blueapi can be a nice, stable, standardised core that's the same for everyone, and per-beamline customization is kept in the plans as far as is reasonable. This is a very core-ivory-tower point of view though and hasn't yet been tested in anger, so I welcome differing opinions @DominicOram and @Tom-Willemsen. We deliberately left this part of the design loose in anticipation of feedback.

[Tom-Willemsen]

Unsure if it's a multi-step process, but I think the eventual aim should be to remove the experiment registry entirely and just replace it with a hierarchy of plans. I.e. there is a top level plan called fast_grid_scan_experiment which takes the correct parameters, sets up the subscriptions via yield from subscribe(...), and calls the actual fast_grid_scan plan.

The philosophy here is that blueapi can be a nice, stable, standardised core that's the same for everyone, and per-beamline customization is kept in the plans as far as is reasonable. This is a very core-ivory-tower point of view though and hasn't yet been tested in anger, so I welcome differing opinions @DominicOram and @Tom-Willemsen. We deliberately left this part of the design loose in anticipation of feedback.

I agree with what you've written, and I'm hoping that this PR doesn't conflict too much with that view. The only additional coupling to experiment_registry in this PR is that the object returned by setup (in practice, always a composite) is passed as the first argument to the plan. In my mind the way to move this out of the experiment registry level would be something along the lines of:

from blueapi import inject_composite

@dataclasses.dataclass
class MyPlanComposite:
    backlight: Backlight
    eiger: Eiger
    ...

def my_experiment_plan(
        composite: MyPlanComposite = inject_composite(MyPlanComposite),
        ...
    ):
    yield from prepare(composite)
    yield from do_it(composite)
    yield from clean_up(composite)

where inject_composite is similar to the existing blueapi inject mechanism, but instead of injecting a single device at a time, knows how to iterate through the members of the referenced composite and inject a "populated" composite. Essentially moving the logic from device_composite_from_context introduced in this PR into blueapi itself. Exact syntax can be argued/refined...

The main thing I was trying to avoid was having to write something very drawn out like:

from blueapi import inject

def my_experiment_plan(
        backlight: Backlight = inject("backlight"),
        eiger: Eiger = inject("eiger"),
        ...<20 more devices>...
    ):
    yield from prepare(eiger, backlight, <20 more devices>)
    yield from do_it(eiger, backlight, <20 more devices>)
    yield from clean_up(eiger, backlight, <20 more devices>)

in every single plan (and passing through lots of devices into subplans too).

I think the option to inject single device(s) should remain though, for simpler plans that only need a limited number of devices.

---

This PR is indeed just a first step though - moving away from hard-coded i03.something() to something generic across beamlines. There are further issues open in this repo to move to blueapi's management components, which would then mean using something more like blueapi's inject mechanism or similar.

---

Does that sound sensible?

[DominicOram]

Unsure if it's a multi-step process, but I think the eventual aim should be to remove the experiment registry entirely and just replace it with a hierarchy of plans. I.e. there is a top level plan called fast_grid_scan_experiment which takes the correct parameters, sets up the subscriptions via yield from subscribe(...), and calls the actual fast_grid_scan plan.

The philosophy here is that blueapi can be a nice, stable, standardised core that's the same for everyone, and per-beamline customization is kept in the plans as far as is reasonable. This is a very core-ivory-tower point of view though and hasn't yet been tested in anger, so I welcome differing opinions @DominicOram and @Tom-Willemsen. We deliberately left this part of the design loose in anticipation of feedback.

Yes, this was my understanding of the end goal. This is a step towards that to try and work incrementally to where we want to be. I think the next issue is probably DiamondLightSource/mx-bluesky#339 then I think a full removal of experiment_registry and clean up

[callumforrester]

@DominicOram and @Tom-Willemsen, that all sounds good to me! Will let you know if I have any other thoughts.

[DominicOram]

Great, thank you. All looks a lot tidier

[codecov]

Codecov Report

Merging #871 (39868c0) into main (10b6522) will increase coverage by 0.31%.
The diff coverage is 92.54%.

❗ Current head 39868c0 differs from pull request most recent head 592cef7. Consider uploading reports for the commit 592cef7 to get more accurate results

@@            Coverage Diff             @@
##             main     DiamondLightSource/hyperion#871      +/-   ##
==========================================
+ Coverage   93.03%   93.34%   +0.31%     
==========================================
  Files          51       52       +1     
  Lines        2456     2526      +70     
==========================================
+ Hits         2285     2358      +73     
+ Misses        171      168       -3     

Files	Coverage Δ
src/hyperion/device_setup_plans/setup_oav.py	98.46% <100.00%> (ø)
...c/hyperion/experiment_plans/experiment_registry.py	100.00% <ø> (ø)
src/hyperion/parameters/beamline_parameters.py	97.14% <100.00%> (-0.73%)	⬇️
src/hyperion/utils/aperturescatterguard.py	100.00% <100.00%> (ø)
src/hyperion/__main__.py	90.00% <93.75%> (-0.61%)	⬇️
...eriment_plans/grid_detect_then_xray_centre_plan.py	99.00% <97.61%> (-1.00%)	⬇️
...perion/experiment_plans/oav_grid_detection_plan.py	98.76% <92.85%> (-1.24%)	⬇️
...rion/experiment_plans/optimise_attenuation_plan.py	93.79% <95.23%> (-0.56%)	⬇️
...hyperion/experiment_plans/pin_tip_centring_plan.py	95.83% <92.30%> (-1.14%)	⬇️
...rc/hyperion/experiment_plans/rotation_scan_plan.py	99.11% <97.14%> (-0.89%)	⬇️
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[DominicOram]

I think we can also now remove both get_beamline_prefixesand associated tests as we're doing this in dodal

[DominicOram]

Some comments in code but mostly there

[DominicOram]

Great, thank you Tom