spc-group / haven

Bluesky tools for beamlines managed by the spectroscopy group
https://haven-spc.readthedocs.io/en/latest/
Other
2 stars 6 forks source link

Can the InstrumentRegistry have ... #101

Open prjemian opened 1 year ago

prjemian commented 1 year ago
canismarko commented 1 year ago

@prjemian, here are my thoughts. Is this a feature request or just a discussion at this point? Either one is fine just wondering how to proceed.

a .remove(item) method?

Yes, shouldn't be too difficult. Internally, there's a components list. remove(item) would look in this list for item and remove it from the list (and optionally all it's sub-components too). Is item expected to be a Device instance, or device name as a string (or both)?

a .cleanup() method (that calls .remove(item) for any item which cannot be found, other than in the .components list)?

By "cannot be found" do you mean "are not present by channel access" or "are not reachable as python objects"? First should be doable, just need to wait_for_connection() on each item, maybe? Second, I'm not sure how to test for this. Maybe the gc module allows for checking the value of the reference counter for each item? It's a bit at odds with how I use the instrument registry since none of my devices are accessible except through the registry. What is the use case for this (can talk more during office hours if its easier)?

prjemian commented 1 year ago

Is item expected to be a Device instance, or device name as a string (or both)?

Both. If the object cannot be found (in Python), then its children must be inaccessible from the object. Remove all of them.

prjemian commented 1 year ago

By name (as string) or by reference (as object), that's the question. I was thinking of by reference (the object itself).

The remove(item) method might be called by a cleanup() method which iterates over all the known objects and removes any which cannot be found (in Python). It should not remove items which cannot be found by Channel Access since a not found condition might be due to the IOC's momentary unavailability.

prjemian commented 1 year ago

The cleanup() method could even be called internally as part of some routine operation, as long as it executes quickly, with no side-effects.

prjemian commented 1 year ago

Determination of "known" might be a challenge since the registry itself maintains a reference to each object.

canismarko commented 7 months ago

I got the first one done. You can now either use .pop() or the del keyword. I used these options rather than remove() since it's similar to how python dictionaries work. Version 1.0.0 on pypi has these new features.

https://github.com/spc-group/ophyd-registry?tab=readme-ov-file#removing-devices

canismarko commented 7 months ago

Oops, forget to remove a device's child components. Fixed it in the feature branch. I'll update pypi soon.

canismarko commented 7 months ago

I think I have a solution to the cleanup() question (maybe, see below). As I was thinking about it, this sounded a lot like reference counting, and so maybe keeping weak references is an easy solution.

Now you can create the registry as Registry(keep_references=False) and OphydObjects that are no longer accessible in another scope may be automatically removed from the registry. Since it relies of garbage collection to actually remove the objects, you can use gc.collect() to force this.

I left the default as keep_references=True since one my motivations for making this package was so I didn't have to keep references to every OphydObject I make.

@prjemian would this suit your use case? If you still see value in an explicit cleanup() method, I could just make registry.cleanup() call gc.collect().

Here is some code examples: https://github.com/spc-group/ophyd-registry/tree/weakref?tab=readme-ov-file#keeping-references

a test: def test_weak_references():

and the PR: https://github.com/spc-group/ophyd-registry/pull/4

The weird part is that if I actually use the code in the example, it doesn't work. But the test I wrote, does. If I look at the reference count, for a simulated epics motor it is ~37. Not sure why there are so many references left over. So maybe this isn't possible in most circumstances.

canismarko commented 6 months ago

For v1.2.0, I merge in the weak references option described above, and added a pop_disconnected() method. It looks at all the root devices in the registry and removes/returns those that are not connected (with an optional timeout argument).

The motivation here is to mimic a device's wait_for_connection() method but on all the devices at once so you don't have to wait for the timeout for each device individually:

1) create a new registry 2) make a bunch of devices 3) call registry.pop_disconnected(timeout=3) 4) provide feedback (warning, exception, etc) for which devices weren't connected

@prjemian I think I'm ready to close this issue. Anything else you'd like included?

prjemian commented 6 months ago

This sounds right. Kind of.

My intent was to drop objects from the registry that are no longer in use. Such as, when autoregistry is used, those created in within a function or method, just with local scope. So this might apply to ophyd objects not related to EPICS.

canismarko commented 5 months ago

Got it. I think the keep_references option does that. Here's a quick test in ipython.

This first block is with regular references. See that after the function runs, the registry still has access to the motor 1.

In [45]: reg = Registry(keep_references=True, auto_register=False)
    ...: print(f"Start: {len(reg.findall('motor1', allow_none=True))}")
    ...: def inner():
    ...:     motor = reg.register(Signal(name='motor1'))
    ...:     print(f"Inner: {len(reg.findall('motor1', allow_none=True))}")
    ...: # Run the function
    ...: inner()
    ...: # Check if we can get to the motor anymore
    ...: print(f"End: {len(reg.findall('motor1', allow_none=True))}")
Start: 0
Inner: 1
End: 1

Whereas with weak references, that device is gone after losing function scope:

In [46]: reg = Registry(keep_references=False, auto_register=False)
    ...: print(f"Start: {len(reg.findall('motor1', allow_none=True))}")
    ...: def inner():
    ...:     motor = reg.register(Signal(name='motor1'))
    ...:     print(f"Inner: {len(reg.findall('motor1', allow_none=True))}")
    ...: # Run the function
    ...: inner()
    ...: # Check if we can get to the motor anymore
    ...: print(f"End: {len(reg.findall('motor1', allow_none=True))}")
Start: 0
Inner: 1
End: 0