Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: Architecture for inject #874

Open
DominicOram opened this issue Oct 29, 2024 · 7 comments
Open

Discussion: Architecture for inject #874

DominicOram opened this issue Oct 29, 2024 · 7 comments
Labels
question Further information is requested

Comments

@DominicOram
Copy link
Contributor

DominicOram commented Oct 29, 2024

As a user of BlueAPI I would like to not have to specify my devices everytime I call a particular plan e.g.

def initialise_detector(det: Eiger):
    ...

Should be able to be called from BlueAPI with no arguments specified as BlueAPI should already have all the devices and be able to use its existing Eiger.

There are a number of ways we could do this:

  1. What we currently have:
def initialise_detector(det:Eiger = ixx.eiger()):
    pass

As discussed on #854 this has the issue that it does work on import.

  1. What we used to have:
def initialise_detector(det:Eiger = inject("eiger_name")):
    pass

This had the issue that it was hard to fix in pydantic 2.

  1. A decorator approach:
@inject(det="eiger_name")
def initialise_detector(det:Eiger):
    pass
  1. Passing the factory itself to inject without calling it:
def initialise_detector(det:Eiger = inject(ixx.eiger)):
    pass
  1. Remove the magic entirely:
def initialise_detector(det:Eiger|str = "eiger_name"):
    if isinstance(det, str):
         det = fetch_device(CONTEXT, det)
    pass

Acceptance Criteria

  • Options are discussed by stakeholders
  • A prototype implementation is written
@DominicOram
Copy link
Contributor Author

My personal preference is 3. I think it is more readable as "this is just a blueAPI thing I can ignore generally" and it is beamline independent.

@DiamondJoseph
Copy link
Contributor

DiamondJoseph commented Nov 5, 2024

I prefer 1; or some version of it that linting is happy with e.g.

eiger = ixx.eiger()

def initialise_detector(det:Eiger = eiger):
    ...

With ophyd-async calling connect only when required, the only [required] side effect is creating light weight Signals without backends. Any device that needs to e.g. read from the filesystem should do that on first connect.

I'd much rather have 3 or 4 than 2 or 5.

RE: 5 I really really want to avoid re-inventing the Finder: once we re-introduce the idea that you can get anything anywhere in your plan, the signature of every plan is degenerate, and it's a race to the bottom of not knowing what was available/required at the start of your plan and not able to pass any overrides so you simultaneously have N plans for the same scan with each of N devices, and none of them have the device as an argument...

RE: 3 it would give us a hook to ensure the device is connected too, at the start of the scan. But it also means there is an inconsistency between running the scan with blueapi and from a terminal.

@DominicOram
Copy link
Contributor Author

With ophyd-async calling connect only when required, the only [required] side effect is creating light weight Signals without backends. Any device that needs to e.g. read from the filesystem should do that on first connect.

I still think doing this on import is bad:

  • Devices are going to be written by a number of people, including potentially scientists. I think it's unrealistic to enforce no work on initialisation (not just touching filesystem, it's more generally anything that we would ideally want to mock out) and to expect this to continue to be enforced over ~20 years rather than falling into the old pattern of just not doing testing
  • The consequence of factories now not being connected by default is a degradation of the jupyter notebook way of working. It is expected by a scientist that calling a factory will give them a device that is connected and is a gotcha to then have to call connect on it.

@callumforrester
Copy link
Contributor

From discussion with @DominicOram and @DiamondJoseph:
We agree that (potential) import side effects are bad, so discourage 1. We want to encourage injecting all devices into plans rather than retrieving them halfway through, so discourage 5.

There is some disagreement over just how terrible a thing having tens of defaulted arguments for complex plans would be. It's messy, but explicit e.g.

def my_plan(x: Motor = inject(ixx.x), y: Motor = inject(ixx.y), z: Motor = inject(ixx.z), ...) -> MsgGenerator:
    ...

@DiamondJoseph and I had previously envisioned inject only being used to specify per-beamline defaults, but @DominicOram is interested in using it to make plans reusable between beamlines, making 4. difficult as it ties a plan to a beamline.

The current, boilerplate-heavy solution would be:

def plan_for_beamline_1(eiger: Eiger = inject(beamline_1.eiger)) -> MsgGenerator:
    yield from base_plan(eiger=eiger)

def plan_for_beamline_2(eiger: Eiger = inject(beamline_2.eiger)) -> MsgGenerator:
    yield from base_plan(eiger=eiger)

def base_plan(eiger: Eiger) -> MsgGenerator:
    ...

We discussed using composites to satisfy this e.g.

class ExperimentalComposite(BaseModel):
    x: Motor = inject(ixx.x)
    y: Motor = inject(ixx.y)
    eiger: Eiger = inject(ixx.eiger)

def my_plan(composite: ExperimentalComposite), z: Motor = inject(ixx.z)-> MsgGernerator:
    ...

@callumforrester
Copy link
Contributor

  1. definitely does couple the devices and plan logic much more closely. I'm currently favouring 2. or 3.

@callumforrester
Copy link
Contributor

We are favouring 3. because:

  • We want to keep plan logic and device instantiation separate
  • Scientists can ignore a decorator more easily than a default argument
  • A decorator can also be used to signify that something is a plan that blueapi should export, however we should be careful not to reinvent the existing @bluesky.plan decorator.

We could have something like this, where we have a shared set of constants for device names...

# Devices
@device_factory()
def eiger() -> Eiger:
    return Eiger(name=MY_EXPERIMENT_DEFINITION.main_detector)

# Plan
@export(inject={"eiger": MY_EXPERIMENT_DEFINITION.main_detector})  # This should shout at you if there is no parameter called "eiger" on import
def my_plan(eiger: Eiger) -> MsgGenerator:
    ...

@callumforrester
Copy link
Contributor

Thinking about how a composite device would work:

class ExperimentalComposite(BaseModel):
    x: Motor
    y: Motor
    eiger: Eiger

@export(inject={"composite": {"x": "x", "y": "y", "eiger": "eiger"}})
def my_plan(composite: ExperimentalComposite))-> MsgGenerator:
    ...

...can we think of a nicer way?

Do we want something like this? Composite is defined as a device.

@export(inject={"x": "x", "y": "y", "eiger": "eiger"})
class ExperimentalComposite(BaseModel):
    x: Motor
    y: Motor
    eiger: Eiger

@export()
def my_plan(composite: ExperimentalComposite)) -> MsgGenerator:
    ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants