python-microscope / microscope

Python library for control of microscope devices, supporting hardware triggers and distribution of devices over the network for performance and flexibility.
https://www.python-microscope.org
GNU General Public License v3.0
69 stars 41 forks source link

avoiding `initialize` #129

Open carandraug opened 4 years ago

carandraug commented 4 years ago

Ideally, objects are in an usable state after construction. However, in Device we have both __init__ and initialize. However, I don't see where this is required and deviceserver calls initialize right after __init__ so can the code of initialize be moved into __init__?

The devices that have any code in initialize are the Cobolt, Obis, and Sapphire lasers, and the cameras. I have already checked the lasers and there's no reason why having the initialize method at all. The cameras are all floating devices and their initialize is a bit more convolved so I'm not sure.

If we can avoid having an extra step, that's one less source of issues. It also means we don't have to check at each step if the device has already been initialized, and also we don't need to defined the behaviour for something that has been through __init__ but through __initialize__.

mickp commented 4 years ago

I would be wary of reyling on doing hardware device initialization solely within __init__.

Sometimes, it is necessary to reinitialize a device to put it into a known state. Currently, a client can do that by calling initialize, and the existing settings dict can be reapplied. If device initialization were only within __init__, we'd have to destroy the device instance and create a new one, which would lose any existing settings info and require a way to bind the new object to any existing Pyro connections.

Also, currently, if the deviceserver attempts to connect to a device before it is ready, we can keep the object and the client can retry initialization later. This would not be possible if we didn't have initialize.

There is also the case where there are interdependent devices, and initialization of one device can not be completed until its dependencies have been initialized. How might we handle this if initialization is solely in __init__, if we need to intantiate the device object in order to determine its dependencies?

I think __init__ should largely concern itself with setting up the object. It may then attempt to call initialize, but we should be able to call this elsewhere, too.

carandraug commented 4 years ago

Sometimes, it is necessary to reinitialize a device to put it into a known state. Currently, a client can do that by calling initialize, and the existing settings dict can be reapplied.

I don't think we are ever reinitializing a device in that manner. What is the situation? We could get the existing settings dict and apply them on a newly created object, no need to keep the same only for that.

If device initialization were only within init, we'd have to destroy the device instance and create a new one, which would lose any existing settings info and require a way to bind the new object to any existing Pyro connections.

Is this not the same from when the device crashes and the device server constructs a new one? We already have to handle that.

Also, currently, if the deviceserver attempts to connect to a device before it is ready, we can keep the object and the client can retry initialization later. This would not be possible if we didn't have initialize.

I think this is the only instance where we call initialize a second time instead of constructing the device again. However, what do we gain from it? Would it not work just as well if it just kept calling __init__ until it works? It might take a bit longer but the step that takes most time would be the initialization anyway so I don't think it matters.

There is also the case where there are interdependent devices, and initialization of one device can not be completed until its dependencies have been initialized. How might we handle this if initialization is solely in init, if we need to intantiate the device object in order to determine its dependencies?

Where is this a problem at the moment? We currently have separate __init__ and initialize but we don't handle this case. I think requiring them to have them specified in the correct order should be enough.

mickp commented 4 years ago

I don't think we are ever reinitializing a device in that manner.

We do, from cockpit. For example, when a camera is power cycled, we can often prevent having to restart cockpit by calling the camera's initialize from the shell.

Is this not the same from when the device crashes and the device server constructs a new one

No. For that mechanism to be engaged, the process has to die. This is not (always) the case if a connection is dropped then re-established between controller-PC and hardware.

I think this is the only instance where we call initialize a second time

That's the only instance where microscope calls initialize a second time. Initialize is also exported to cockpit, which may call it several times.

Where is this a problem at the moment?

It's handled in cockpit. Take a look at how cockpit.depot initializes devices. depot.initalizedevices calls SomeDevice.initialize which typically points to (or calls) initialize on the remote device. I think removing initialize would make it considerably more difficult to ever move something like cockpit's configurational awareness down into microscope.

One other point to note is that the PYME skeleton class we were given to ensure we implemented similar methods also used separate __init__ and Init. PYME also uses a model that encapsulates hardware initialization outside of __init__.

What are the benefits of removing initialize?

juliomateoslangerak commented 4 years ago

I agree that it is better to keep different 'levels' of initialization: init, initialize, enable. In the cases where this is redundant you can always call initialize inside init.