BUG: fix skiprows callable infinite loop #45586

bram2000 · 2022-01-24T10:40:19Z

closes BUG: read_excel skiprows callable can result in infinite loop #45585
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

Not sure what to do about the whatsnew entry. Please advise (I've read the contributing doc but could not find it mentioned).

bram2000 · 2022-01-24T11:20:29Z

I'm not sure why that "Test experimental data manager" test fails; it doesn't appear to be related to my code.

EDIT: Also I cannot run this test locally; it requires numba which is not compatible with my M1 CPU.

phofl

Small comments

please add a whatsnew

pandas/tests/io/excel/test_readers.py

bram2000 · 2022-01-24T12:52:43Z

Small comments

please add a whatsnew

Is the whatsnew process documented anywhere? I couldn't figure out what to do!

doc/source/whatsnew/v1.5.0.rst

pandas/tests/io/excel/test_readers.py

rhshadrach

lgtm

doc/source/whatsnew/v1.5.0.rst

phofl · 2022-01-27T21:18:35Z

doc/source/whatsnew/v1.5.0.rst

@@ -227,6 +227,7 @@ MultiIndex
 I/O
 ^^^
 - Bug in :meth:`DataFrame.to_stata` where no error is raised if the :class:`DataFrame` contains ``-np.inf`` (:issue:`45350`)
+- Bug in :meth:`read_excel` with `engine=python` results in an infinite loop with certain `skiprows` callables (:issue:`45585`)


What does engine=python mean? I do not think that python is valid. Please also add ticks around the string, e.g. engine="python"

I'm not sure it was suggested here #45586 (comment)

I've just removed it.

Please let the reviewer resolve conversations

I think I get what jeff meant. read_csv with engine="python" and read_excel.

Please also add a test for read_csv

jreback · 2022-01-28T00:29:18Z

@phofl @rhshadrach ok here?

phofl · 2022-01-28T10:08:17Z

Tests are failing.

adjustment of whatsnew
adding a test for read csv

are open points

bram2000 · 2022-01-28T10:45:48Z

The merge of main seems to have broken something, I can no longer run my test...

> python -m pytest pandas/tests/io/excel/test_readers.py -vv -s
ImportError while loading conftest '/Users/bram/Code/nca/pandas/pandas/conftest.py'.
pandas/conftest.py:608: in <module>
    params=[
pandas/conftest.py:611: in <listcomp>
    if not isinstance(indices_dict[key], MultiIndex) and indices_dict[key].is_unique
pandas/_libs/properties.pyx:37: in pandas._libs.properties.CachedProperty.__get__
    val = self.fget(obj)
pandas/core/indexes/base.py:2297: in is_unique
    return self._engine.is_unique
pandas/_libs/properties.pyx:37: in pandas._libs.properties.CachedProperty.__get__
    val = self.fget(obj)
pandas/core/indexes/base.py:873: in _engine
    return libindex.ExtensionEngine(target_values)
E   AttributeError: module 'pandas._libs.index' has no attribute 'ExtensionEngine'```

phofl · 2022-01-28T10:53:18Z

You have to build pandas again
python setup.py build_ext --inplace -j 4

pep8speaks · 2022-01-28T11:14:15Z

Hello @bram2000! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-01-31 10:21:49 UTC

bram2000 · 2022-01-28T11:17:09Z

I've added a similar test for csv... it passes but it doesn't appear to go through my new code anyway. Does it still make sense to mention read_csv in the whatsnew?

phofl · 2022-01-28T11:35:55Z

No, don't have to mention in this case

pre-commit is failing

bram2000 · 2022-01-28T11:48:30Z

precommit is failing with the following, but I'm not sure why as I've copied the form of the other notes in the file.

rst ``code`` is two backticks...................................................................................Failed
- hook id: rst-backticks
- exit code: 1

doc/source/whatsnew/v1.5.0.rst:287:- Bug in :meth:`read_excel` results in an infinite loop with certain `skiprows` callables (:issue:`45585`)

phofl · 2022-01-28T12:02:49Z

:func:read_excel please

pre-commit is failing because of
skiprows -> skiprows

Edit: Two backticks before and after skiprows

rhshadrach · 2022-01-28T20:37:25Z

I've added a similar test for csv... it passes but it doesn't appear to go through my new code anyway. Does it still make sense to mention read_csv in the whatsnew?

You have to add engine="python". When I do this, I'm seeing _next_line being hit.

@phofl

Edit: Two backticks before and after skiprows

I struggled with this before. In GitHub (and maybe elsewhere?), you can use two different number of backticks to make backticks show up, e.g. `backtick` or ``backtick``

jreback · 2022-01-31T00:06:55Z

@bram2000 can you merge master and address comments

bram2000 · 2022-01-31T10:23:14Z

I've merged master.

Even if i manually add engine="python" to the read_excel call it still doesn't hit my debugger in the affected while loop. The test iterates through the four engine types anyway, so don't think it's necessary to explicitly set engine.

rhshadrach · 2022-01-31T21:12:28Z

it still doesn't hit my debugger in the affected while loop

Ah, I see - yes, I'm seeing the same. However doesn't the same bug exist in L691 down below?

bram2000 · 2022-02-01T11:07:02Z

it still doesn't hit my debugger in the affected while loop

Ah, I see - yes, I'm seeing the same. However doesn't the same bug exist in L691 down below?

No it doesn't seem to. I've tried various combinations of skiprow functions and I can't get it to fail.

rhshadrach · 2022-02-01T23:02:13Z

@bram2000 - thanks for checking! Sorry for my confusion, it all makes sense now.

rhshadrach · 2022-02-05T20:51:43Z

@jreback @phofl - good here?

rhshadrach · 2022-02-05T20:53:23Z

@bram2000 - failure look unrelated. Can you merge main.

mroeschke · 2022-02-06T22:41:16Z

Thanks @bram2000

* BUG: fix skiprows callable infinite loop * BUG: seperate new test * add whatsnew entry * make note user-centric * add engine * move note to IO section * modify comment * add test for csv * lint * formatting * fix whatsnew * test not valid for pyxlsb Co-authored-by: Jeff Reback <[email protected]>

BUG: fix skiprows callable infinite loop

3f8f7c6

phofl requested changes Jan 24, 2022

View reviewed changes

pandas/tests/io/excel/test_readers.py Outdated Show resolved Hide resolved

BUG: seperate new test

e563d93

add whatsnew entry

0e09f5a

jreback requested changes Jan 24, 2022

View reviewed changes

doc/source/whatsnew/v1.5.0.rst Outdated Show resolved Hide resolved

pandas/tests/io/excel/test_readers.py Show resolved Hide resolved

jreback added Bug IO CSV read_csv, to_csv IO Excel read_excel, to_excel labels Jan 24, 2022

bram2000 added 2 commits January 24, 2022 16:48

make note user-centric

30cfec6

add engine

831f69c

rhshadrach approved these changes Jan 27, 2022

View reviewed changes

rhshadrach requested changes Jan 27, 2022

View reviewed changes

doc/source/whatsnew/v1.5.0.rst Outdated Show resolved Hide resolved

move note to IO section

ac235d4

phofl reviewed Jan 27, 2022

View reviewed changes

modify comment

9df6e01

jreback added this to the 1.5 milestone Jan 28, 2022

Merge branch 'main' into skiprows-function-fix

c1acd08

add test for csv

85d5ee1

lint

a1f9187

formatting

3e41e7d

bram2000 added 2 commits January 28, 2022 13:19

fix whatsnew

196b055

test not valid for pyxlsb

3abc40a

Merge branch 'main' into skiprows-function-fix

ecd25b9

rhshadrach approved these changes Feb 1, 2022

View reviewed changes

jreback approved these changes Feb 5, 2022

View reviewed changes

phofl approved these changes Feb 6, 2022

View reviewed changes

mroeschke merged commit de6b11d into pandas-dev:main Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fix skiprows callable infinite loop #45586

BUG: fix skiprows callable infinite loop #45586

bram2000 commented Jan 24, 2022 •

edited

Loading

bram2000 commented Jan 24, 2022 •

edited

Loading

phofl left a comment

bram2000 commented Jan 24, 2022

rhshadrach left a comment

phofl Jan 27, 2022

bram2000 Jan 27, 2022

phofl Jan 27, 2022

jreback commented Jan 28, 2022

phofl commented Jan 28, 2022

bram2000 commented Jan 28, 2022 •

edited

Loading

phofl commented Jan 28, 2022

pep8speaks commented Jan 28, 2022 •

edited

Loading

bram2000 commented Jan 28, 2022

phofl commented Jan 28, 2022

bram2000 commented Jan 28, 2022

phofl commented Jan 28, 2022 •

edited

Loading

rhshadrach commented Jan 28, 2022 •

edited

Loading

jreback commented Jan 31, 2022

bram2000 commented Jan 31, 2022

rhshadrach commented Jan 31, 2022 •

edited

Loading

bram2000 commented Feb 1, 2022

rhshadrach commented Feb 1, 2022

rhshadrach commented Feb 5, 2022

rhshadrach commented Feb 5, 2022

mroeschke commented Feb 6, 2022

BUG: fix skiprows callable infinite loop #45586

BUG: fix skiprows callable infinite loop #45586

Conversation

bram2000 commented Jan 24, 2022 • edited Loading

bram2000 commented Jan 24, 2022 • edited Loading

phofl left a comment

Choose a reason for hiding this comment

bram2000 commented Jan 24, 2022

rhshadrach left a comment

Choose a reason for hiding this comment

phofl Jan 27, 2022

Choose a reason for hiding this comment

bram2000 Jan 27, 2022

Choose a reason for hiding this comment

phofl Jan 27, 2022

Choose a reason for hiding this comment

jreback commented Jan 28, 2022

phofl commented Jan 28, 2022

bram2000 commented Jan 28, 2022 • edited Loading

phofl commented Jan 28, 2022

pep8speaks commented Jan 28, 2022 • edited Loading

Comment last updated at 2022-01-31 10:21:49 UTC

bram2000 commented Jan 28, 2022

phofl commented Jan 28, 2022

bram2000 commented Jan 28, 2022

phofl commented Jan 28, 2022 • edited Loading

rhshadrach commented Jan 28, 2022 • edited Loading

jreback commented Jan 31, 2022

bram2000 commented Jan 31, 2022

rhshadrach commented Jan 31, 2022 • edited Loading

bram2000 commented Feb 1, 2022

rhshadrach commented Feb 1, 2022

rhshadrach commented Feb 5, 2022

rhshadrach commented Feb 5, 2022

mroeschke commented Feb 6, 2022

bram2000 commented Jan 24, 2022 •

edited

Loading

bram2000 commented Jan 24, 2022 •

edited

Loading

bram2000 commented Jan 28, 2022 •

edited

Loading

pep8speaks commented Jan 28, 2022 •

edited

Loading

phofl commented Jan 28, 2022 •

edited

Loading

rhshadrach commented Jan 28, 2022 •

edited

Loading

rhshadrach commented Jan 31, 2022 •

edited

Loading