Fix and allow streamable server cameras #143
Merged
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.streamablethat, 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:CameraBaseis 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_defaultsthat 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: valuetocamera.yamland poof! Suddenly yourctrl.camobject has access to anyvaluedesired!On paper, it works great! However, it acts really strange when observed via the lens of
CamClienti.e. when using a server.CamClientoverloads__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_namein its namespace,__getattr__will call server implementation ofattr_namebut wrap it in localattr_nameso that the Exceptions are easy to understand. Ifattr_namedoes not exist in local namespace but does remotely, it will be called without wrapping:So here is when things get very funny:
streamableis a class attribute of every camera class. Therefore,streamableexists in the local class namespace as logged inself._dct. Therefore, whenevercamera.streamableis 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 = Falseto the config and callingcamera.streamable, instead of getting aFalse, you get aFalsewrapped inTruethat 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._dctandself._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 = Truenow. You will also see that I prevent theAttributeErrorfrom being raised ifattr_nameis 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 ofCameraBaseis not read byCamClientwhich believes that this method is thus undefined and falsely raisesAttributeErrorwhenever you try to display any image from server camera usingFakeVideoStream. So this issue is also fixed here.Major changes
streamablevariable;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, existingCamClientshould handle the error.Code maintanence
Clientwas removed;