labscript-suite / labscript-utils

Shared modules used by the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦. Includes a graphical exception handler, debug tools, configuration management, cross platform filepath conversions, unit conversions and custom GUI widgets.
http://labscriptsuite.org
Other
2 stars 45 forks source link

labscript_utils.excepthook uses tk #104

Open ispielma opened 6 months ago

ispielma commented 6 months ago

Is there a reason that import labscript_utils.excepthook uses the tk GUI system while the rest of labscript uses qt? It seems like a needless use of system resources to import two different GUI libraries.

philipstarkey commented 6 months ago

The original reason (from 10+ years ago) was that Tk was shipped by default with whatever Python distribution we were telling people to use and Qt was not. So if the exception was due to a Qt issue, we'd still get a graphical exception popup.

But I have no idea of that's still true (someone should investigate if Tk comes with Conda and/or default Python before actioning). And we have requirements files now. So the initial justification may not make sense anymore.

That said, the excepthook opens the GUI in a new subprocess (so that the parent process crashing doesn't hide the exception window). So either way, it'll have to import a GUI library from scratch regardless of which one it is. And Tk may well be faster than Qt.

ispielma commented 6 months ago

Thanks @philipstarkey. That makes a lot of sense. Tk is still part of the python standard library so it should (almost always) be present. The reason I was tracking this down was in the context of lyse where these windows pop up for processes which left their errors in the lyse text box anyway. This can lead to many basically useless windows to close. I was thinking that if qt were controlling it there would be a better way to inhibit them from opening.

chrisjbillington commented 6 months ago

Tk still comes with Python by default, though Ubuntu and Debian-based Linuxes strip it out, annoyingly, and make it its own package. Not sure if that carries over to venvs, or if you get tk if you create a venv in Ubuntu (it would be silly if you didn't). But yeah, "lowest common denominator" requirements was the idea.

The reason I was tracking this down was in the context of lyse where these windows pop up for processes which left their errors in the lyse text box anyway.

The custom excepthook is called only for uncaught exceptions, so the way to suppress it would be to catch exceptions in the context where you're going to print them anyway, print a traceback to stderr, and then not re-raise the exception.

Errors in user code in lyse analysis routines is a silly reason to pop up windows, and it looks like we catch them and do the above, but if there is another context where it's sort-of "user" code instead of application code breaking, we should catch exceptions in those contexts as well to suppress the popups.

Would be hesitant to suppress them for anything that happens in a lyse subprocess though, since the communication back to the parent process is one of the things that could go wrong (and that mechanism for that is not exactly low in the "stack", so can't really be relied on if other things are breaking).