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
66 stars 38 forks source link

Device server config duplicate issue #274

Closed thomasmfish closed 3 weeks ago

thomasmfish commented 1 year ago

We have the config:

from microscope.cameras import atmcd
from microscope.stages import linkam
from microscope.devices import device

DEVICES = [
    device(atmcd.AndorAtmcd, 'ixon1.b24', 7777, uid='8906'),
    device(atmcd.AndorAtmcd, 'ixon2.b24', 7777, uid='8974'),
    device(linkam.LinkamCMS, 'ixon1.b24', 9000, uid='')
    ]

Which means that devices are sharing a conf. In the case of similar devices (here, two Atmcds) we end up with clashes. I suggested a fix some time ago but when I was sorting out a new version to install I found this https://github.com/thomasmfish/microscope/commit/5b9a07d25bbbb395125e50c4b53ce8da8e36801f , which I thought fixed the issue. However, this doesn't seem to have actually fixed the issue, so here's the branch with my proposed fix: https://github.com/thomasmfish/microscope/tree/alternative-device-conf-fix

thomasmfish commented 3 weeks ago

Just to let you know this is an issue that I faced again with very unclear error messages when helping someone else set up their system. Adding deepcopy(conf), as in the linked branch, fixed the issue. The deepcopy that is done later on only seems to go on layer deep, so does not make a new conf, even though the conf is included in the dictionary that is being deepcopied.

iandobbie commented 3 weeks ago

The merges from master updates make the vital commit hard to find. I think the critical commit is 0836713, which is small and obvious in what it does. The only slight drawback I can see is that we get an extra copy of every config dict, but they are small and it seems like a small price to pay for actually fixing a real problem that Diamond are having.

carandraug commented 3 weeks ago

I'm fine with making the deepcopy in device() (and if we do, then we should remove the one in serve_devices). However, I don't understand where the issue with the current code. We only modify conf in serve_devices() and we make a deepcopy there. Why is the deepcopy in serve_devices() not working? You mention "The deepcopy that is done later on only seems to go on[sic] layer deep" but the whole point of making a deepcopy is to go down all the layers deep.

carandraug commented 3 weeks ago

Here's showing with code what I think should happen (but I guess is not):

>>> from microscope.device_server import device
>>> from microscope.simulators import SimulatedCamera
>>> import copy
>>> d1 = device(SimulatedCamera, 'localhost', 7789)
>>> d2 = device(SimulatedCamera, 'localhost', 7790)
>>> d1
{'cls': <class 'microscope.simulators.SimulatedCamera'>, 'host': 'localhost', 'port': 7789, 'uid': None, 'conf': {}}
>>> d2
{'cls': <class 'microscope.simulators.SimulatedCamera'>, 'host': 'localhost', 'port': 7790, 'uid': None, 'conf': {}}
>>> d1c = copy.deepcopy(d1)
>>> d2c = copy.deepcopy(d2)
>>> d1c['conf']['index'] = 2  # only modifying d1c but nothing changes on d1, d2, or d2c
>>> d1['conf']
{}
>>> d2['conf']
{}
>>> d1c['conf']
{'index': 2}
>>> d2['conf']
{}
iandobbie commented 3 weeks ago

I understand the expected behavior, maybe there is a subtle bug elsewhere. Unusually, the CryoSIM setup has 2 cameras on one computer but with two network cards. The cards have different addresses and host names and the two cameras bind the same port but on different network interfaces. No reason I can see why this would cause an issue but it is one slight complication for this specific system.

thomasmfish commented 3 weeks ago

The system I most recently had the issue with was with all the devices on localhost. I have no idea why it's not working as expected but we were getting weird errors until I added that in...


From: Ian Dobbie @.> Sent: Tuesday, June 11, 2024 1:22:27 AM To: python-microscope/microscope @.> Cc: Fish, Thomas (DLSLtd,RAL,LSCI) @.>; Author @.> Subject: Re: [python-microscope/microscope] Device server config duplicate issue (Issue #274)

I understand the expected behavior, maybe there is a subtle bug elsewhere. Unusually, the CryoSIM setup has 2 cameras on one computer but with two network cards. The cards have different addresses and host names and the two cameras bind the same port but on different network interfaces. No reason I can see why this would cause an issue but it is one slight complication for this specific system.

— Reply to this email directly, view it on GitHubhttps://github.com/python-microscope/microscope/issues/274#issuecomment-2159540961, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AL6IKW2IDTET4BPW2VOVJDLZGY7MHAVCNFSM6AAAAABJCZ5VLWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZGU2DAOJWGE. You are receiving this because you authored the thread.Message ID: @.***>

This e-mail and any attachments may contain confidential, copyright and or privileged material, and are for the use of the intended addressee only. If you are not the intended addressee or an authorised recipient of the addressee please notify us of receipt by returning the e-mail and do not use, copy, retain, distribute or disclose the information in or attached to the e-mail. Any opinions expressed within this e-mail are those of the individual and not necessarily of Diamond Light Source Ltd. Diamond Light Source Ltd. cannot guarantee that this e-mail or any attachments are free from viruses and we cannot accept liability for any damage which you may sustain as a result of software viruses which may be transmitted in or with the message. Diamond Light Source Limited (company no. 4375679). Registered in England and Wales with its registered office at Diamond House, Harwell Science and Innovation Campus, Didcot, Oxfordshire, OX11 0DE, United Kingdom.

carandraug commented 3 weeks ago

I'm wary about fixes we don't understand and can't reproduce because they come with a note saying that they're needed for mysterious reasons and prevents us from making changes in the future.

Anyway, I think I've figured out what's happening. copy.deepcopy makes a deepcopy of the original object. However, if two objects inside it are references to each other, the new copy keeps that relationship. See:

>>> import copy
>>> k = {"k": 0}
>>> d_original = {"a": k, "b": k}
>>> d_copy = copy.deepcopy(d_original)
>>> d_original
{'a': {'k': 0}, 'b': {'k': 0}}
>>> d_copy
{'a': {'k': 0}, 'b': {'k': 0}}
>>> d_copy['a']['k'] = 10
>>> d_copy  # the values for the 'a' and 'b' remain references to each other
{'a': {'k': 10}, 'b': {'k': 10}}
>>> d_original  # but changing d_copy did not change d_original
{'a': {'k': 0}, 'b': {'k': 0}}

I fixed this issue in serve_devices because this where we make the change to the conf dict. If we were to fix it in device() then someone using serve_devices directly would be affected by this bug. Fixed on 38a19742 .

Also, since #212 I've been convinced that using {} (or other mutable values) as default value is a bad and dangerous idea (in a way, this issue stems from there). So I'm making the default value conf=None