Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling assertion errors in MMCore #84

Open
tlambert03 opened this issue Aug 20, 2023 · 9 comments
Open

Handling assertion errors in MMCore #84

tlambert03 opened this issue Aug 20, 2023 · 9 comments

Comments

@tlambert03
Copy link
Contributor

tlambert03 commented Aug 20, 2023

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

marktsuchida commented Aug 28, 2023

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
Copy link
Contributor Author

ok, all makes sense. thanks for the background!

@marktsuchida
Copy link
Member

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

@marktsuchida
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor Author

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

@marktsuchida
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants