Fix and allow streamable server cameras #143
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Currently, Instamatic does not allow cameras running on server to stream images at all. There are two layers of security that prevent this at different levels. From my discussion with @stefsmeets I concluded that the reason behind that is mostly historical/practical. The initial development of Instamatic was done on a relatively slow camera that did not allow streaming, and since then a lot of work was done on fast local cameras that did not require the client-server architecture.
The main goal of this PR is to allow cameras running on server, such as the new instamatic-TEM-emulator, to stream their data via shared memory. In vacuum, this could be achieved by removing only two lines of code, as noted in #140 discussion. However, this introduced another problem - what if due to practical reasons someone actually wanted their camera to be non-streamable? To address this case, this PR also introduces a new camera config parameter,
config.camera.streamable
that, when set to either True or False, overwrites the default class variable for given camera.Other than allowing streamable server cameras, this PR introduces a few code base improvements. Firstly, it aligns the code conventions used in the camera client code to ones introduced to microscope files by @viljarjf in #99. Furthermore, it allows cameras to call methods that do not exist in their namespace (but may still be accessible on the server) to reflect an analogous change introduced in #108. It slightly improves some docstrings and type hints.
Development note
Having documented all relevant changes, I decided to share some of my notes on writing this thing, because in the process of adding 3 lines for 3 hours I learned that apparently the Instamatic client-server architecture for cameras is heavily reliant on black magic. Buckle up and grab some popcorn!
instamatic.camera.camera_base:CameraBase
is a relatively recent class, introduced to Instamatic by @viljarjf in #91. Acting as the base class of all cameras, it includes a very helpful methodload_defaults
that populates the camera namespace with the camera config. Therefore, it is incredibly easy to add new instance variable and even overwrite camera class variables - just addkey: value
tocamera.yaml
and poof! Suddenly yourctrl.cam
object has access to anyvalue
desired!On paper, it works great! However, it acts really strange when observed via the lens of
CamClient
i.e. when using a server.CamClient
overloads__getattr__
in a very ingenious way. The old implementation first determinesself._dct
(namespace of local camera class) andself._attr_dct
, attribute space of the remote camera class. If the local class hasattr_name
in its namespace,__getattr__
will call server implementation ofattr_name
but wrap it in localattr_name
so that the Exceptions are easy to understand. Ifattr_name
does not exist in local namespace but does remotely, it will be called without wrapping:So here is when things get very funny:
streamable
is a class attribute of every camera class. Therefore,streamable
exists in the local class namespace as logged inself._dct
. Therefore, whenevercamera.streamable
is called,__getattr__
first looks it up in the local class and runswrapped = self._dct[attr_name]
which evaluates to...True
. Since no other checks are run after that, the bottom wrapping code runs as normal and, by a reason I still fully fail to understand, by addingstreamable = False
to the config and callingcamera.streamable
, instead of getting aFalse
, you get aFalse
wrapped inTrue
that must be called to getFalse
! Nota bene this behavior can't be reproduced in an interactive session, as this should be impossible.Attempts to fix this behavior was very annoying because of recursion and the odd nature of
self._dct
andself._attr_dct
. Ultimately, I managed to fix it by changing the first part of__getattr__
to the following:The top addition of conditional
if attr_name in object.__getattribute__(self, '_attr_dct')
checks whether an attribute is present in both local class and on server, in which case it takes its value from the latter. This makes behavior of class variable normal again, in particularself.streamable = True
now. You will also see that I prevent theAttributeError
from being raised ifattr_name
is not found, in line with #108. This change is necessary for external server camera to work! AFAIK in #91,get_camera_dimensions()
method was moved from individual cameras to theCameraBase
- but the scope ofCameraBase
is not read byCamClient
which believes that this method is thus undefined and falsely raisesAttributeError
whenever you try to display any image from server camera usingFakeVideoStream
. So this issue is also fixed here.Major changes
streamable
variable;Minor changes
CamClient
: When calling a method via getattr, try evaluating instead of raising KeyError if it is not found in the interface. This is in line with Tecnai bigfixes, Microscope client documentation and EAFP #108 microscope logic and potentially allows more cameras with "incomplete" local interface template class e.g. the new Instamatic-TEM-emulator to be streamable. Should a method actually be absent, existingCamClient
should handle the error.Code maintanence
Client
was removed;