-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Fix Segfault (maybe) #31092
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
Fix Segfault (maybe) #31092
Conversation
@@ -40,8 +40,7 @@ cdef class BlockPlacement: | |||
self._as_array = arr | |||
self._has_array = True | |||
else: | |||
# Cython memoryview interface requires ndarray to be writeable. | |||
arr = np.require(val, dtype=np.int64, requirements='W') | |||
arr = np.require(val, dtype=np.int64) |
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.
could do np.asarray here? i had to look up np.require
No luck, it's still segfaulting with this branch in the same way. If I look at the traceback in gdb, I get:
The only thing that I can make of it is that the problem happens in deallocation of the DataFrame |
A potential indirect pointer might be the second case I showed here: #21824 (comment) That example (with selecting the column instead of calling the method on the DataFrame), doesn't crash but gives an incorrect result. Maybe fixing that could indirectly fix the segfault as well, who knows (very uncertain of course :)) |
Hmm, actually that second example might be correct after all. |
BTW, I had this branch checked out and then did some Arrow work, and got a failure in an arrow test about an array not being writable (pyarrow might do some funky business with block placement, didn't investigate further) |
Do we have a non-onerous way to reproduce this? i.e. can i help troubleshoot this? |
@jbrockmendel do you get a segfault with the example? |
@jorisvandenbossche can you inspect frame 5 in your core dump to see what variable it is trying to free? |
I don't have much experience with gdb. Do you have a pointer on how I can do that? |
Sure - assuming you already have it loaded just do You can also use https://sourceware.org/gdb/current/onlinedocs/gdb/Selection.html#Selection |
On OSX+py37 I get the segfault on the |
Stepping through the call to |
disabling the |
there it is: disabling the
|
@jbrockmendel I think that's a great find. What is the state of those variables at that point in time? |
Looks like |
Cool! ;) So I printed the variables in the cython function, and you get:
So in this code: Lines 738 to 745 in a890239
it's indeed getting a -1 index out of the So it's clear that the Now for a solution, I think we first need to decide on the behavior that we want. Should the "NaN" in the index level be present in the output as well?
or
If it is the first, then we need to exclude those -1's from the In general, in groupby, we don't include NaNs as a group in the output.
So it might be more consistent to exclude the NaNs from the groups. In that case, we also have to fix the Series case though, as this also includes NaN (but without the segfault):
|
I think should exclude NaN for consistency |
Short on time so closing for now, but hope to come back to this in the future |
I have only been able to reproduce this issue once and can't verify that this works, but I think @jorisvandenbossche has had more luck reproducing so maybe can try
I noticed
_as_array
showing up in a core dump I was able to get, and this line seems a little suspect so perhaps removing the writeable requirement fixes