Add deprecation warning to inject

[callumforrester]

Fixes #845
Supersedes #839

Instructions to reviewer on how to test:

1. Confirm tests pass
2. Optional: write a plan that uses inject and confirm it raises a warning when run.

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]

@DominicOram is there anything else from your comments in #841 that you feel would be appropriate to document here?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.19%. Comparing base (ecc1a20) to head (cd50d73).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #854   +/-   ##
=======================================
  Coverage   95.19%   95.19%           
=======================================
  Files         120      120           
  Lines        4976     4979    +3     
=======================================
+ Hits         4737     4740    +3     
  Misses        239      239           

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

[DiamondJoseph]

I don't want to encourage this, I want to always have the device as a default argument.
This prevents any metrics/tracing/logging/documenting on the arguments of the plan, leads to N different plans that are just using a different device, makes it easier for the scope of the device to be managed incorrectly and cause issues (conditionally instantiating my device as mock/connecting my device?).
I think the Finder in GDA was a mistake that made every momentary decision to use it slightly easier and every subsequent attempt to extract what the hell was going on a lot harder.

I think it's risking making debugging a lot harder for a small benefit in verbosity, so I would like more attention paid to the above and comments about the benefits of doing it that way.

I'm prepared to be told I'm getting in the way of writing plans for this standpoint. I really don't like it.

[DiamondJoseph]

It also means we can call ensure_connected at the top of the plan and fail fast if any device won't be available, rather than after completing the first iteration of the fastest axis: and we can call connect on all of the devices in parallel rather than on each one as we encounter it.

[callumforrester]

To be honest I broadly agree. Dependency injection is a good thing and, in my opinion, having lots of defaulted parameters is not a particularly terrible thing. Maybe we take this out for now and add it later if demand calls for it?

[DominicOram]

@DominicOram is there anything else from your comments in #841 that you feel would be appropriate to document here?

Sorry, been in and out of the office. Maybe we should have a chat about this when I'm back. I'm concerned that by encouraging the use of the factory function in the default args we're:

• Causing the device to be initialized on import of the plan (even though it's not connecting)
• Tying the plan to a specific beamline, you could pass in other devices I guess but it's not obvious and you've already started initialising a different beamline's device

[DiamondJoseph]

With aims of making plans more generic and more available, I think it encourages having modules like mx-bluesky with i04.py [with i04 defaults injected] and plans.py without: blueapi can import both, plans get written into i04.py first then genericised and moved into plans.py when ready.
And moves us away from e.g. i22-bluesky individual beamline module.

[callumforrester]

One pattern to avoid side-effect imports could be:

def my_plan(det: Factory[Readable] = i22.saxs) -> MsgGenerator:
    yield from bp.count([det()])

As far as tying the plan to the beamline goes, there is a tradeoff between that and having too many parameters that you have to fill out every time. You always still have the option of making the plan generic via

def my_plan(det: Readable) -> MsgGenerator: ...

But this helps you in the cases that really are beamline-specific. My thinking on plans is that there will be a layer that forms a "beamline API" (see this comment). So I can imagine wrapping generic plans in beamline specific plans e.g.

from saxs_waxs import linkam_experiment as generic_linkam_experiment  # Multi-beamline plan

# I22 specific version with fewer parameters
def linkam_experiment(start: float, stop: float, stages: int = 1, saxs: Factory[Pilatus] = i22.saxs, waxs: Factory[Pilatus] = i22.waxs) -> MsgGenerator:
    yield from  generic_linkam_experiment(start=start, stop=stop, stages=stages, min_temp=-160, detectors=[saxs, waxs])

[DiamondJoseph]

def my_plan(det: Factory[Readable] = i22.saxs):

Then blueapi's context now passes references to the DFF instead of the device? It seems very clunky to ask people to write plans that look like that.

[callumforrester]

I agree it's not ideal, was just exploring ways of avoiding import side effects. I wonder if @coretl has any thoughts, since he's the main proponent of the use case of writing a plan offline and migrating it to blueapi with minimal effort.

[coretl]

Is there anything wrong with initializing devices as default args in beamline plans?

from saxs_waxs import linkam_experiment as generic_linkam_experiment  # Multi-beamline plan

# I22 specific version with fewer parameters
def linkam_experiment(start: float, stop: float, stages: int = 1, saxs = i22.saxs(), waxs = i22.waxs()) -> MsgGenerator:
    yield from  generic_linkam_experiment(start=start, stop=stop, stages=stages, min_temp=-160, detectors=[saxs, waxs])

Then when they are made generic they will lose their beamline defaults and be wrapped with a bl specific plan

[olliesilvester]

Is there anything wrong with initializing devices as default args in beamline plans?

It's been causing some issues with unit tests - importing the plan instantiates the device with its default arguments, leading to typing errors when the test tries to create a fake device for this plan.

@dperl-dls has done a (probably temporary) workaround to address this for our tests

def rebuild_oa_device_as_mocked_if_necessary(factory: Callable[[], OADevice], **kwargs):
    device = factory(**kwargs)
    if not device._previous_connect_was_mock:  # noqa
        clear_device(device.name)
    device = factory(**kwargs)
    return device