v2: DeviceCollector does not connect variables that existed before being entered.

[rosesyrett]

In general, it seems a good idea for the DeviceCollector to be able to do something like this:

with DeviceCollector():
    motor1 = Motor('OLD-PV')

#processing happens here...

with DeviceCollector():
    motor1 = Motor('NEW-PV') #actually, motor1 needs to be reassigned

But at the moment, the DeviceCollector simply checks on the existing objects before it's been entered, and compares them to the objects on exit by name. This means any replaced objects, such as in the example above, won't be connected. In the above, trying to get and put to motor1 will get an object that has not had connect run, as the enter/exit definitions of DeviceCollector will not have recognized motor1 as a new device which needs connecting.

There is a fairly straightforward way of getting around this, that @coretl has mentioned to me. At the moment, the device collector checks for objects by name on exit:

    def __enter__(self):
        # Stash the names that were defined before we were called
        self._names_on_enter = set(self._caller_locals())
        return self
...
    def __exit__(self, type_, value, traceback):
        self._objects_on_exit = self._caller_locals()
        return call_in_bluesky_event_loop(self._on_exit())
...
    async def _on_exit(self):
        # Name and kick off connect for devices
        tasks: Dict[asyncio.Task, str] = {}
        for name, obj in self._objects_on_exit.items():
            if name not in self._names_on_enter and isinstance(obj, Device):
                ...
                if self._connect:
                    task = asyncio.create_task(obj.connect(self._prefix, self._sim))
                    tasks[task] = name
        ...

Instead, we can check for the id of the objects, and compare that as opposed to the name. This will allow re-defining motors and other ophyd v2 devices in device collectors, and will mean they get connected/named as expected. This would look like so:

    def __enter__(self):
        # Stash the names that were defined before we were called
        self._ids_on_enter = {k: id(v) for k, v in self._caller_locals().items())
        return self
...
    async def _on_exit(self):
        # Name and kick off connect for devices
        tasks: Dict[asyncio.Task, str] = {}
        for name, obj in self._objects_on_exit.items():
            is_new = name not in self._ids_on_enter or id(obj) != self._ids_on_enter[name]
            if is_new and isinstance(obj, Device):
                ...
                if self._connect:
                    task = asyncio.create_task(obj.connect(self._prefix, self._sim))
                    tasks[task] = name
        ...

What do people think of this? Is this what users would expect DeviceCollector to do?

[coretl]

@tacaswell @danielballan @prjemian @mrakitin any thoughts on this?

[prjemian]

I like the notion of an instrument registry that is being developed by @canismarko for our spectroscopy beam lines.

[canismarko]

As @prjemian mentioned, we developed an InstrumentRegistry class for Haven inspired by Pint's ureg.

https://haven-spc.readthedocs.io/en/latest/topic_guides/instrument_registry.html

A default registry object is created at import time and can be used either directly on Device objects:

motor1 = Motor(...)
registry.register(motor1)

or as a class decorator, where the device is registered regardless of the context it's created in:

@registry.register
class MyMotor(Motor):
    ...
    
motor1 = MyMotor(...)

I use the decorator form more since most of my devices are subclasses of ophyd.Device.

You could in theory use multiple registries, but I haven't had a need for that yet.

It currently checks for duplicates by name similar to this DeviceCollector, but that could change easily.

[coretl]

There are 2 problems that DeviceCollector aims to solve:

1. For each dev = Device(), make sure dev.name is set to "dev" if not already explicitly set
2. Calling await asyncio.gather(d.connect() for d in all_devices)

The first of these exists in both ophyd.v1 and v2, the second is only in v2 as it can do a concurrent connection of many devices at once.

I think the registry could be made to solve the first (if you declare MyMotor(name="motor") instead of motor=MyMotor() then you could globals().update({d.name: d for d in registry.findall())). It could also handle the second case, although it would have to be careful to not call connect on devices with parents, as the parent is responsible for connecting its children at present.

How do you solve 1 at present?

[tacaswell]

I would not trust stashing the id as they are only unique for objects that all exist at the same time. If an object is garbage collected it's ID is available for (immediante) re-use (there is a bluesy issue where we verified this the hard way with some of the logic in plan mutator....). Just keep a reference to any devices that exist on entry (so you are sure you keep them alive) and then you can do an is test to verify it is the same object or not. I think if we are going to have DeviceCollector then we need to support this.

I'm very very skeptical of a global devices registry at the ophyd library level. I think "you must keep track of your devices" is way simpler to explain (and enables facility and beamline specific registries if people want them) rather than "we keep a global registry that is almost certainly not what you wanted but is close so please spend an inordinate amount of effort engineering around un-needed global state". I acknowledge my presentation of the alternatives is biased.

(if you declare MyMotor(name="motor") instead of motor=MyMotor() then you could globals().update({d.name: d for d in registry.findall())).

I think this is getting too far away from Python syntax

[coretl]

I would not trust stashing the id as they are only unique for objects that all exist at the same time. If an object is garbage collected it's ID is available for (immediante) re-use (there is a bluesy issue where we verified this the hard way with some of the logic in plan mutator....). Just keep a reference to any devices that exist on entry (so you are sure you keep them alive) and then you can do an is test to verify it is the same object or not. I think if we are going to have DeviceCollector then we need to support this.

Excellent, let's do that:

    def __enter__(self):
        # Stash the names that were defined before we were called
        self._objects_on_enter = self._caller_locals()
        return self
...
    async def _on_exit(self):
        # Name and kick off connect for devices
        tasks: Dict[asyncio.Task, str] = {}
        for name, obj in self._objects_on_exit.items():
            is_new = name not in self._objects_on_enter or obj is not self._objects_on_enter[name]
            if is_new and isinstance(obj, Device):
                ...
                if self._connect:
                    task = asyncio.create_task(obj.connect(self._prefix, self._sim))
                    tasks[task] = name
        ...

@RAYemelyanova please can you make the changes with associated tests?

[coretl]

I'm very very skeptical of a global devices registry at the ophyd library level. I think "you must keep track of your devices" is way simpler to explain (and enables facility and beamline specific registries if people want them) rather than "we keep a global registry that is almost certainly not what you wanted but is close so please spend an inordinate amount of effort engineering around un-needed global state". I acknowledge my presentation of the alternatives is biased.

Agreed, let's make DeviceCollector useful but completely optional, so sites can build their own registries that do the functionality of naming and connection their own way.