-
-
Notifications
You must be signed in to change notification settings - Fork 8k
FIX: be more cautious about checking widget size #30484
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to have an _isdeleted; should we be using that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we check the error message to ensure we’re not swallowing other potential RuntimeErrors?
Also, are we sure this is the only place that may access the C++ object?
We seem to have an _isdeleted; should we be using that?
Yes and than also handles Tim's comment.
This can fail if the c++ side of the object has already been cleaned up but the Python side is still lingering (due to refs in callbacks). closes matplotlib#29618
4821cd6 to
4b0a26a
Compare
I confirmed this also fixes the reproduce I found.
Test failures appear to be related to a new release of coverage:
https://pypi.org/project/coverage/#history
https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst#version-7106--2025年08月29日
EDIT: Was not coverage, was pytest-rerunfailures
...484-on-v3.10.x Backport PR #30484 on branch v3.10.x (FIX: be more cautious about checking widget size)
This can fail if the c++ side of the object has already been cleaned up but the Python side is still lingering (due to refs in callbacks).
closes #29618
I do not think it is worth constructing a test to generate this case, but in the case where I saw it in the wild this fixed the tests locally for me.
PR checklist