Skip to content

stop "conf"s all being the same dict#212

Merged
iandobbie merged 1 commit into
python-microscope:masterfrom
thomasmfish:B24-dual-AndorAtmcd-fix
Jun 16, 2021
Merged

stop "conf"s all being the same dict#212
iandobbie merged 1 commit into
python-microscope:masterfrom
thomasmfish:B24-dual-AndorAtmcd-fix

Conversation

@thomasmfish

Copy link
Copy Markdown
Contributor

When multiple devices are being created without a conf set, all dev["conf"]s are pointing to the same dictionary, meaning that they all have the same _index. This was making it impossible to load both AndorAtmcd cameras being loaded at once on the cryoSIM at B24, Diamond Light Source as it hit this within the class AndorAtmcd:

    def initialize(self):
        """Initialize the library and hardware and create Setting objects."""
        _logger.info("Initializing ...")
        num_cams = GetAvailableCameras()
        if self._index >= num_cams:
            msg = "Requested camera %d, but only found %d cameras" % (
                self._index,
                num_cams,
            )
            raise microscope.InitialiseError(msg)
        self._handle = GetCameraHandle(self._index)

@iandobbie iandobbie merged commit ef280c3 into python-microscope:master Jun 16, 2021
@carandraug

Copy link
Copy Markdown
Collaborator

The proposed fix changes the default from an empty dict to a None. This also changes the function signature so the type annotation needs to be fixed accordingly, i.e., from Mapping[str, Any] to Optional[Mapping[str, Any]]

However, I don't think this is the right fix. The default really is an empty dict, and the use of None here is a workaround. The root problem is that in microscope.device_server.serve_devices the dictionary of arguments is being modified and that behaviour is unexpected.

Suppose that instead of wanting the default empty dict, we do have two devices with the same list of arguments, so we do:

conf = {'arg1': some_value}
DEVICES = [
    device(SomeFloatingDevice, .., conf=conf, uid='B'),
    device(SomeFloatingDevice, .., conf=conf, uid='A'),
]

Because this does not trigger if conf is None: conf = {}, both devices will end up with the same index value. The issue here remains.

I believe the right fix is to have make a copy of the conf dict (possibly in serve_devices I'm not sure).

@thomasmfish

Copy link
Copy Markdown
Contributor Author

@carandraug presumably any original dict is thrown away after device is created anyway, so I've made a different fix at #217 (unfortunately untested for now)

carandraug added a commit that referenced this pull request Oct 12, 2021
…#211)

This commit has serve_devices make a deepcopy of the devices argument
so that in case we need to make changes to it, we don't modify the
stuff in the caller.  This commit reverts ef280c3 which was
another fix for this issue.

The way we support floating devices requires that we inject the index
value on the device configuration.  However, we don't want that to
propagate to the caller which may be reusing that configuration (and
it kinda does it when device configuration is not specified see
discussion on #212).
thomasmfish added a commit to thomasmfish/microscope that referenced this pull request Jun 1, 2022
thomasmfish pushed a commit to thomasmfish/microscope that referenced this pull request Jun 1, 2022
…python-microscope#211)

This commit has serve_devices make a deepcopy of the devices argument
so that in case we need to make changes to it, we don't modify the
stuff in the caller.  This commit reverts ef280c3 which was
another fix for this issue.

The way we support floating devices requires that we inject the index
value on the device configuration.  However, we don't want that to
propagate to the caller which may be reusing that configuration (and
it kinda does it when device configuration is not specified see
discussion on python-microscope#212).
carandraug pushed a commit that referenced this pull request Jun 12, 2024
Using a mutable object such as {} as default can lead to surprises
behaviour (see pylint notes for dangerous-default-value / W0102 at
https://pylint.pycqa.org/en/stable/user_guide/messages/warning/dangerous-default-value.html ).
Indeed, this triggered the issues #211, #212, and #274.  So just make
the default None and convert to an empty dict.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants