Fix segfault with printing dataframe
State: closed

GH 46848

- [x] closes #46848
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Labels:
name: Output-Formatting
description: __repr__ of pandas objects, to_string
name: Regression
description: Functionality that used to work in a prior pandas version

Comments:


I have a fix that prevents the segfault, but I am not sure how a regression test could be made as the failure depends on the width of the terminal screen. I deem this not a workable regression test, but I am at a loss how to trigger the issue otherwise.
As far as I can determine the crash only happens when printing a dataframe requires the use of dots to indicate missing values when the screen is not wide enough.
Apparently there is some logic to determine this situation, and then take another code path. This other code path then takes care of the injection of the dots, but also triggers the code path that produces the segfault.
Ideally we can make a regression test that provokes the code path that produces the segfault without the print logic.
Does the segfault arise in one of the lines of the diff of this PR? Or is it only when this result is used later on?
The segfault is caused on the original line 186.
The code generated by cython tries to increment the refcount of the object on the rhs of the assignment expression. However that is a NULL pointer.
Question is where that NULL value comes from. 
The extra check for None prevents this call to increase the refcount.

can you add a test which replicates
> can you add a test which replicates

@roberthdevries can you also add a release note to doc/source/whatsnew/v1.4.3.rst and resolve conflicts with main


> I have a fix that prevents the segfault, but I am not sure how a regression test could be made as the failure depends on the width of the terminal screen.

use MRE from https://github.com/pandas-dev/pandas/issues/46848#issuecomment-1138672874
@jbrockmendel thoughts on catching the uninitialised numpy array in setitem instead? A change to setitem was the cause to the regression. either way I guess there will a performance regression. 

according to https://github.com/numpy/numpy/issues/7854 it is a Cython issue. maybe we should also not fix.
> thoughts on catching the uninitialised numpy array in setitem instead?

its not clear to me how we check for this.  is there something in like arr.flags that would have what we need? 
> > thoughts on catching the uninitialised numpy array in setitem instead?
> 
> its not clear to me how we check for this. is there something in like arr.flags that would have what we need?

I was thinking along the lines that maybe more performant to do a check once on setitem rather than on every subsequent take operation. but thinking some more should probably take this suggestion of the table and be consistent with the fix for the similar issue for DataFrame construction #13717.
> according to [numpy/numpy#7854](https://github.com/numpy/numpy/issues/7854) it is a Cython issue. maybe we should also not fix.

in the issue, the OP is using 

```python
for col in df.columns:
    df[col] = np.empty_like(df[col])

print(df)
```
which results in the segfault.

instead
```python
for col in df.columns:
    df.loc[:, col] = np.empty_like(df[col])

print(df)
```
works fine.

#46267 is reporting a significant slowdown for repeated assigned via \_\_setitem__

this sort of supports the "won't fix" (in pandas) argument as the workaround is more performant.

@roberthdevries can you run the asvs to see if the current proposed fix affects performance. 
The current proposed fix should greatly slow down things.  You probably would need to push the code closer to C to fix it, though (unless cython is "fixed").
(That is should not be particularly difficult, but not a typical code pattern here I guess.)

Maybe we should prioritize NumPy avoiding this.  There are some cases where NumPy probably cannot, but I guess that those cases do not really affect normal cython use cases.  Of course, that will only fix the issue on newer NumPy versions.
moving off 1.4.3 milestone.
> @roberthdevries can you run the asvs to see if the current proposed fix affects performance.

I have run the asvs, but the output is huge and I cannot find anything obvious in it that would indicate a slowdown. But  maybe you could help me by pointing in the direction where I should look, or tell me what you are looking for.
there's a benchmark using `take_nd` called `parallel_take1d` so I assume not relevant. And there's a `time_take` in `asv_bench/benchmarks/indexing.py` which is for a Series. So it maybe that we don't have any benchmarks specific to the changed code path.

So if we don't see any other slowdowns, maybe that's good or maybe should add an explicit benchmark to be sure
My nightly run did not report any significant performance degradations:
```
[99.81%] ··· Setting up timedelta:14                                                                                                                                  ok
[99.81%] ··· timedelta.DatetimeAccessor.time_dt_accessor                                                                                                         178±2ns
[99.83%] ··· timedelta.DatetimeAccessor.time_timedelta_days                                                                                                     1.16±0ms
[99.86%] ··· timedelta.DatetimeAccessor.time_timedelta_microseconds                                                                                             1.17±0ms
[99.88%] ··· timedelta.DatetimeAccessor.time_timedelta_nanoseconds                                                                                              1.16±0ms
[99.90%] ··· timedelta.DatetimeAccessor.time_timedelta_seconds                                                                                               1.14±0.01ms
[99.93%] ··· Setting up tslibs.timedelta:55                                                                                                                           ok
[99.93%] ··· tslibs.timedelta.TimedeltaProperties.time_timedelta_days                                                                                            167±2ns
[99.95%] ··· tslibs.timedelta.TimedeltaProperties.time_timedelta_microseconds                                                                                    165±1ns
[99.98%] ··· tslibs.timedelta.TimedeltaProperties.time_timedelta_nanoseconds                                                                                   167±0.6ns
[100.00%] ··· tslibs.timedelta.TimedeltaProperties.time_timedelta_seconds                                                                                         166±1ns
(base) robert@thinkpad-rhdv:~/projects/pandas/asv_bench$
```

@jbrockmendel wdyt here?

The underlying issue this is fixing has been around since at least pandas-0.25.3 (the oldest I checked with using your code sample in https://github.com/pandas-dev/pandas/issues/46848#issuecomment-1138672874)

There have been issues with np.empty-like before but this current issue has only surfaced for the DataFrame printing due to the \_\_setitem__ change in 1.4.

The fix here is probably the right place (if fixing in pandas) but the take code, I assume, is performance critical, although benchmarks haven't turned anything up. (do you know if the 2d take path is heavily used?)


> The fix here is probably the right place (if fixing in pandas) but the take code, 

Agreed this is the best option currently available.  Maybe @scoder can weigh in on future possibilities.

> I assume, is performance critical, although benchmarks haven't turned anything up. (do you know if the 2d take path is heavily used?)

It's object-dtype, so I'm inclined to just not worry about it.
Btw. I have opened a cython PR that should fix this: https://github.com/cython/cython/pull/4859

Even if we say that the NumPy fix is the right path, that might be the best way forward to avoid lingering issues until we can rely on a "corrected" NumPy.
This pull request is stale because it has been open for thirty days with no activity. Please [update](https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#updating-your-pull-request) and respond to this comment if you're still interested in working on this.
> Btw. I have opened a cython PR that should fix this: [cython/cython#4859](https://github.com/cython/cython/pull/4859)

That's great. Thanks @seberg

I've not yet tested the fix, but now that that fix is in a released Cython we maybe able to close the issue with just merging the test from this PR.

Probably best not to add the release note, just in case we have issues at wheel build time, but can certainly backport the test to 1.4.x
Yeah, now that cython has a new release the fix is to pin that version and only add the test if anything.  I guess there might be a few other places with hacks in place that can now be removed.
Thanks @roberthdevries for the PR. closing in favor of #47979

tested with @jbrockmendel minimal example and cython 0.29.32