micro-manager / pymmcore

Python bindings for MMCore, Micro-Manager's device control layer
https://pypi.org/project/pymmcore/
GNU Lesser General Public License v2.1
32 stars 8 forks source link

Handling assertion errors in MMCore #84

Open tlambert03 opened 10 months ago

tlambert03 commented 10 months ago

There are a few places where assert statements are used in MMCore and DeviceBase.h, such as here.

If these lines fail, the python runtime will crash out back to terminal.

Example:

from contextlib import suppress

from pymmcore import CMMCore, StateDevice

core = CMMCore()
mm_path = "/Users/talley/Library/Application Support/pymmcore-plus/mm/Micro-Manager-2.0.2-20230810"
core.setDeviceAdapterSearchPaths([mm_path])
core.loadDevice("StateDev", "DemoCamera", "DWheel")
core.loadDevice("Camera", "DemoCamera", "DCam")

# ============  No problem here, just runtime error  we go on
with suppress(RuntimeError):
    core.getState("NotADevice")

# ============  No problem here: Camera is a device, but not a state device
with suppress(RuntimeError):
    core.getState("Camera")  # Also just 

# ============  Unless we uncomment core.initializeDevice, we abort 
# StateDev IS a state device... but `CreateIntegerProperty(MM::g_Keyword_State`
# hasn't been called by the adapter yet

# core.initializeDevice("StateDev")  # <- the fix
assert core.getDeviceType("StateDev") is StateDevice  # (it is)
with suppress(Exception):  # <- doesn't help
    core.getState("StateDev")  # Abort: GetPosition, file DeviceBase.h, line 2287.

@marktsuchida, curious to hear your thoughts. Is a full crash something we could prevent here? would it be at the level of mmCoreAndDevices (by swapping those asserts for something else), at the level of SWIG (by catching them somehow and re-reraising), or all the way at the level of pymmcore-plus or something, by (laboriously) trying to make sure you don't do something like call getState on an uninitialized stateDevice?

marktsuchida commented 10 months ago

In general C++ assert() should be used for checking internal assumptions/consistency and, in the case of functions with narrow contracts, precondition violations (all of these are programming errors, not run-time errors). Device adapters are allowed to make certain assumptions about how the Core will call them (unfortunately not very clearly documented), so violations of those assumptions could be checked by assert() (or not checked at all).

I would say that in this specific case, calling getState() on an uninitialized device is illegal. The only legal things to do to an uninitialized device is to initialize, unload, or get/set pre-init properties (and their metadata) (and maybe a few more things). So for DeviceBase to crash in this case is not a DeviceBase bug: this call from core to device is illegal.

We could (and probably should), however, change the Core to throw a nice exception when an attempt is made to use an uninitialized device in ways that are not supported. If I were redesigning I would use a separate object for device-configurator vs active-device, but in practice this would have to be done by adding conditional checks on pretty much every device method (in the MMCore DeviceInstance classes).

In the past we often just recommended using a configuration file and not mess with uninitialized devices, but I realize that that is probably too limiting when building more elaborate systems on top of MMCore.

BTW did you hit any of the assert()s in MMCore? Would be good to know because that would likely be a bug.

tlambert03 commented 10 months ago

thanks for your help!

The only legal things to do to an uninitialized device is to initialize, unload, or get/set pre-init properties (and their metadata) (and maybe a few more things). So for DeviceBase to crash in this case is not a DeviceBase bug: this call from core to device is illegal.

yep, that makes sense.

We could (and probably should), however, change the Core to throw a nice exception when an attempt is made to use an uninitialized device in ways that are not supported.

agree. a related question i've been wondering about recently (perhaps I've missed something obvious), is whether there is a way to know (from core) whether a device has been initialized. I see that most of the device adapters maintain some internal state to know whether they've been initialized or not, but it's hard to know from core. is that correct? or did i miss it? if so, it's of course harder to avoid attempting to use an uninitialized device (if the initialization state of every device needs to be saved outside of core)

In the past we often just recommended using a configuration file and not mess with uninitialized devices, but I realize that that is probably too limiting when building more elaborate systems on top of MMCore.

yeah, definitely understand that. I came across this while building a python config wizard. I do understand that it's a fringe case.

more generally, I think it is a good goal for pymmcore to never (ever) crash out to the terminal with an abort trap. That's just not what a python user expects to happen in the case of an exception. Happy to help get there! With your helpful info, I'll take a closer look and see if I can come up with more useful examples and perhaps a proposal

marktsuchida commented 10 months ago

whether there is a way to know (from core) whether a device has been initialized.

Not currently. We need to add a boolean flag to DeviceInstance to manage (and query) this. Currently the only safe thing to do is to treat loaded-but-uninitialized devices as very dangerous objects that should not be kept around outside of a limited context. Not good.

I see that most of the device adapters maintain some internal state to know whether they've been initialized or not, but it's hard to know from core.

Yes, but in practice I would not count on any sequence other than load-init-unload or load-unload (Shutdown() being implicit during unload) working, because nobody tests device adapters with multiple calls to Initialize(). The only requirement is that Shutdown() must not fail even if the device is uninitialized (or failed to initialize -- some devices count on Shutdown() to undo partial initialization, unfortunately). All this we should encode in MMCore so that other sequences are prevented.

more generally, I think it is a good goal for pymmcore to never (ever) crash out to the terminal with an abort trap. That's just not what a python user expects to happen in the case of an exception.

Couldn't agree more! I would categorize this as unfinished work. At least this one should be relatively easy to fix without affecting existing code.

tlambert03 commented 10 months ago

ok, all makes sense. thanks for the background!

marktsuchida commented 9 months ago

Fixed in micro-manager/mmCoreAndDevices#376 (MMCore 10.5.0) but keeping this open until pymmcore is updated.

marktsuchida commented 9 months ago

It turns out that throwing an exception on these checks was too ambitious and caused issues with MMStudio's Hardware Configuration Wizard, so we had to partially revert this. See micro-manager/mmCoreAndDevices#385 (MMCore 10.6.0) for details.

The hope is to re-enable the exceptions once the HCW (and parts of MMCore) have been updated to work correctly.

marktsuchida commented 8 months ago

@tlambert03 One thing we could easily do is to add a (C++) method to CMMCore that enables/disables these strict initialization state checks. If disabled by default it won't affect existing MMStudio code and would probably also help in fixing such code. The question is what to do for pymmcore: leave disabled, enable by default, or enable in pymmcore-plus? Maybe start disabled for the next release to give some time to test?

tlambert03 commented 8 months ago

i like that idea. it's kinda like strict mode for javascript :)

marktsuchida commented 8 months ago

Function to query init state has been added in micro-manager/mmCoreAndDevices#395.