Conversation
|
@jeanconn the error you reported is one that's bedeviled me for years. I've spent way too many hours googling, conversing with ChatGPT, etc., trying to fix it. How did you arrive at this particular solution? |
|
Hi @jzuhone this fix largely came out of from fighting with Claude which convinced me that
And it convinced me that the most critical fix is probably adding that extra singleShot to do a deferred update. But let me know if you think it actually helps. I'm still trying to figure out if by solving one problem it has created others. |
|
Also still trying to figure out if this can be trimmed down into a more isolated patch. |
|
It looks like the minimal change of gets this to work when installed in my ska3-hope environment, so maybe the extra parent/child handling to avoid premature garbage collection isn't really needed anyway. |
|
I also don't know if something about ska3-hope (with matplotlib 3.10.8 and qt 5.15.15) has made the "RuntimeError: wrapped C/C++ object of type FigureCanvasQTAgg has been deleted" more reliable? |
|
@jeanconn I tried these changes on my current Ska installation on my laptop, with: python 3.12.8 I used it on the CEA model, which has reliably crashed on me while attempting various plot modifications. It didn't crash this time, and everything else seemed to work, so I think this patch is great. |
|
@jeanconn I don't think the single-shot is enough, because I've tried that before. This bug is pretty intermittent. I would support your whole patch here. |
|
Oh awesome. Thanks for the functional testing @jzuhone! If it looks like there's some value here we'll get this out of draft status and move this along. |
jeanconn
changed the title
jeanconn
changed the title
There was a problem hiding this comment.
I just:
- ran the tests
- verified that master crashes with the error in the description, and the branch succeeds
- went over the code changes
The changes seem reasonable to me. I would point out that having something inherit from a layout class and then have other data members is very unusual, and probably should not be done. There is still another class that inherits from QVBoxLayout (PlotsBox) but we are not refactoring the whole thing now. I understand.
This seems like a matplotlib issue to me.
There are a few changes that smell like this type of error. But I think the source of the problem could be the lines in FitStatWindow:
self.fig = Figure()
canvas = FigureCanvas(self.fig)
self.canvas = canvas
where the local variable canvas goes out of scope and is garbage collected. This should not happen, because the parent of the canvas is correctly passed in the constructor, so the parent QWidget should keep a reference to it until itself is destroyed
I looked into the source of FigureCanvasQT (its a base of FigureCanvas), and these are the relevant lines:
class FigureCanvasQT(FigureCanvasBase, QtWidgets.QWidget):
def __init__(self, figure=None):
super().__init__(figure=figure)
See? super().__init__(figure=figure) only calls the __init__ method from FigureCanvasBase, not QWidget. That is the cause of the issue.
As an example, run the following
class A:
def __init__(self, *args, **kwargs):
print(f"A args={args} kwargs={kwargs}")
class B:
def __init__(self, *args, **kwargs):
print(f"B args={args} kwargs={kwargs}")
class C(A, B):
def __init__(self, figure):
super().__init__(figure=figure)
c = C(figure=None)
Another possible cause for issues like this (but not this one)
Another change in the same spirit, but I don't think it caused the original issue is this line in the constructor of app.MainLeftPanel:
container = QtWidgets.QWidget()
which creates a widget that is then collected when the constructor exits. I expect that the owner of the widget changes in this line:
self.scroll.setWidget(container)
at least in the C++ side it does that. Who knows if the python stuff is properly implemented.
|
Awesome. Thanks for the deep dive @javierggt . Do you think we should open a new issue and put your research in there if we decide to implement more fixes or suggest upstream fixes? Or just try to remember to come back to this PR if we want to do more? |
|
In summary, I believe the actual fix was making sure the canvas was a data member of the |
|
I was thinking we could report this upstream. I don't have a strong opinion on where to keep it here. |
3 participants
Description
More fixes for xija_gui_fit to stop RuntimeError FigureCanvasQTAgg has been deleted.
This was required for me for xija_gui_fit to run at all when installed in ska3/hope on OSX - and seems to improve reliability for ska3/latest installs.
Without these fixes I was just getting this when I tried to run xija_gui_fit from the installed environment.
Strangely, it worked fine running straight from the repo with
python -m xija.gui_fit.appbut didn't work after being installed. So that was just weird. But with these Qt changes the code changed to reliably running from either the repo or "as installed" with xija_gui_fit for me on OSX with a recent hope candidate.This issue seems to be a recurrence of #126 which we believed fixed with #127 .
I don't entirely understand the timing and Qt.
Fixes #126 again
Interface impacts
Testing
Unit tests
Independent check of unit tests by Javier
Functional tests
For functional testing, I've made test conda packages and tested this on:
My test was just to run "xija_gui_fit aca", add at least one plot, and run a fit. On each platform, I got a crash without this PR before xija_gui_fit really started and it worked OK as far as I could tell with the PR. On kady linux I note that my matplotlibrc is set to use TkAgg, so if I wanted to run xija_gui_fit without " newbackend, required_framework, current_framework))
ImportError: Cannot load backend 'TkAgg' which requires the 'tk' interactive framework, as 'qt' is currently running" I just specified the qt backend by env var with "env MPLBACKEND=qt5agg xija_gui_fit aca".
John Zuhone also noted additional functional testing in the comments.