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

[DiamondJoseph]

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

Do you have an example of a plan that's causing issues? There was discussion around passing *args, **kwargs to the device factory function and the ability to pass args was removed- this would only have helped if your test function called device(args) before importing plans.py, but importing a module during a test is something that can be done: and with intentions of dodal linting imports to ensure that devices never import from plans, something that should ensure the correct order?

[callumforrester]

@coretl A more general answer: https://docs.astral.sh/ruff/rules/function-call-in-default-argument/#why-is-this-bad

[DiamondJoseph]

@olliesilvester just had a chat with me, think we can resolve what's going on much easier if it's just whether the device is mock or not that needs to be set, not extra init arguments:

A device is not "mock" or "real" until connect() has been called on the device.
I've made an issue because we already think that the device should not eagerly connect by default, default arguments of plans should always definitely have connect_immediately=False to prevent connecting being a side effect of importing plans.

e.g.

@device_factory()
def saxs()
   return PilatusDetector()


def plan(det: Readable = saxs(connect_immediately=False)):
    yield from ensure_connected(saxs)
    yield from {}


def test_that_only_gets_mock_saxs():
    # Same instance of device in the default arg, and only now connects and only now is mock or not
    saxs(connect_immediately=True, mock=True)
    yield from plan()

[coretl]

@coretl A more general answer: https://docs.astral.sh/ruff/rules/function-call-in-default-argument/#why-is-this-bad

If the use of a singleton is intentional, assign the result call to a module-level variable, and use that variable in the default argument:

Ok fine...

from saxs_waxs import linkam_experiment as generic_linkam_experiment  # Multi-beamline plan

SAXS = i22.saxs()
WAXS = i22.waxs()

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

[DiamondJoseph]

Example from i22 using a default_devices.py

https://github.com/DiamondLightSource/i22-bluesky/pull/99/files#diff-88a0805a81dca16d95038e63da5ca21e5ad4976539050ca654c3bfa006f4e6b0

[d-perl]

default arguments of plans should always definitely have connect_immediately=False to prevent connecting being a side effect of importing plans

I really don't think you should have to add code to prevent there being side effects on import

[DiamondJoseph]

I don't know of a way to know that the call to ixx.device() is in the defaults of a method or not, so if a device can be connected on creation, even if I make it less likely to be the default, I'd prefer to be explicit.

The alternative is that we can't ever connect a device on creation and always have to call ensure_connected in a plan or device.connect()

[d-perl]

yes, maybe that would be better, if we really have to have them as default args (which I would still strongly prefer we find a different solution to)

[DiamondJoseph]

I've opted for the latter: blueapi will call connect on all devices concurrently on startup, any users on the command line should ensure their devices are connected before use by awaiting device.connect(), and any plans with defaults should be written with yield from ensure_connected(*devices) to make the case simpler for both of the above.