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

PR: Fix crash and error dialog when triggering OverflowError (too large int) when editing arrays and dataframes #6118

Merged
merged 10 commits into from
Jan 4, 2018

Conversation

CAM-Gerlach
Copy link
Member

Fixes #6114 .

Spyder array and dataframe editor now fail gracefully when a value too large for the given (int) datatype is entered in a cell, showing a warning message rather than crashing or displaying an error dialog, respectively (the former due to a Python3-incompatible e.message syntax of retrieving the error string in that case). Also, a bit of minor associated cleanup.

TODO: Add a simple test for each, ETA Tomorrow...err, later today (Wednesday). Assuming that happens in time, it could be included in 3.2.6, though no pressure on my end.

Also, one really weird bug I wasn't yet able to track down: Before the Array Editor change, Spyder would crash completely ("Python has stopped working...") due to the exception handling bug, except when SPYDER_DEBUG=3. In that case, it would just print the error traceback to the shell it was launched in and continue on. However, conversely, once this fix was implemented it now works fine in all scenarios, except when SPYDER_DEBUG=3 and one hits enter/return to confirm the whole Array editor dialog, rather than just clicking off of the current cell to save just it. In the former case, the warning dialog is triggered as it should, but after hitting okay, the Variable Explorer window c;loses and then a "Python has stopped working..." crash is triggered. I was able to get the following traceback printed to console:

QObject::installEventFilter(): Cannot filter events for objects in a different thread.
Traceback (most recent call last):
  File "C:\Users\C. A. M. Gerlach\Documents\dev\SpyderDev\spyder\spyder\widgets\variableexplorer\arrayeditor.py", line 393, in commitAndCloseEditor
    self.closeEditor.emit(editor, QAbstractItemDelegate.NoHint)
RuntimeError: wrapped C/C++ object of type QLineEdit has been deleted

When I tried catching and passing that error, it would still crash, presumably somewhere else maybe in C++ that I couldn't see. Again, this only happened in SPYDER_DEBUG=3, when hitting enter right from the edit box, and with an OverflowError of course. If you have any insight, please let me know; otherwise just ignore this and I'll look into it more tomorrow...er, today.

@CAM-Gerlach CAM-Gerlach added this to the v3.2.6 milestone Jan 3, 2018
@CAM-Gerlach CAM-Gerlach requested a review from ccordoba12 January 3, 2018 10:56
@pep8speaks
Copy link

pep8speaks commented Jan 4, 2018

Hello @CAM-Gerlach! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 04, 2018 at 16:21 Hours UTC

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Jan 4, 2018

All builds on Travis, but not AppVeyor or CircleCI, are failing all four tests, apparently due to self.test_array[0] = val not raising an error (OverflowError) when it should, despite confirming that val is the same as I'm seeing on my machine, i.e. 2 ** 34. I'm baffled as to why that might be; it appears both are running identical or nearly so numpy and python versions, and the difference is the same across Python 3.6, 3.5 and 2.7; only different CIs running the test suite. Any ideas?

FYI, AppVeyor is only failing (one test) this cycle due to using print() statements in my code to test Travis CI; everything else passed before and after. Its Travis that's the issue, and I can't figure out why. Its just not failing when its supposed to.

@jitseniesen
Copy link
Member

There is also no OverflowError being raised on my Linux box when assigning 2 ** 34 to an int32 array. Here is a plain (i.e. outside Spyder) IPython session:

Python 3.6.3 |Anaconda custom (64-bit)| (default, Oct 13 2017, 12:02:49)
Type 'copyright', 'credits' or 'license' for more information
IPython 6.0.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import numpy as np

In [2]: arr = np.array([1,2,3], dtype=np.int32)

In [3]: arr[0] = 2 ** 34

In [4]: arr
Out[4]: array([0, 2, 3], dtype=int32)

In [5]: arr[0] = 2 ** 66
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)
<ipython-input-5-3cf8afbda221> in <module>()
----> 1 arr[0] = 2 ** 66

OverflowError: Python int too large to convert to C long

@jitseniesen
Copy link
Member

numpy/numpy#6289 seems to confirm this

@CAM-Gerlach
Copy link
Member Author

@jitseniesen Sorry to bother you again @jitseniesen , but any idea on this one? Its only test_dataframeeditor_edit_overflow that's failing, and it does so on every Travis build consistently; the OverflowError is triggered as it should be as the unit test of that passes, and the overall qtbot navigation seems to work also since test_arrayeditor_edit_overflow passes too. If it isn't immediately obvious, maybe test it with qtbot.wait(1000)'s and see what it does, if you can? I really need to get my old Linux machine up and running, but I'm not even sure if its bootable at this point.


@flaky(max_runs=3)
@pytest.mark.skipif(os.environ.get('CI', None) is not None and
system() == "Linux",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use sys.platform.startswith('linux') here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's what I used on the other one previously, but couldn't remember the exact syntax. Just curious, what's the advantage over platform.system()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just our custom throughout our entire codebase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks.

@ccordoba12
Copy link
Member

@CAM-Gerlach, I left a minor comment, then I'd say this is ready.

@CAM-Gerlach
Copy link
Member Author

Updated as requested, plus more minor cleanup. CIs are still running, but I'm going to try to get a little sleep before work so wanted to let you know before. Thanks!

@CAM-Gerlach CAM-Gerlach changed the title PR: Fix program crash and error dialog respectively when triggering OverflowError (too large int) editing array and dataframe respectively PR: Fix program crash and error dialog respectively when triggering OverflowError (too large int) editing array and dataframe Jan 4, 2018
@CAM-Gerlach
Copy link
Member Author

@ccordoba12 Checks passed, so should be ready to go.

@ccordoba12 ccordoba12 changed the title PR: Fix program crash and error dialog respectively when triggering OverflowError (too large int) editing array and dataframe PR: Fix crash and error dialog when triggering OverflowError (too large int) when editing arrays and dataframes Jan 4, 2018
ccordoba12 added a commit that referenced this pull request Jan 4, 2018
@CAM-Gerlach CAM-Gerlach deleted the array-maxint-crash branch January 10, 2018 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants