Skip to content

TEST: dropna() doesn't fall into infinite loop #50751

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

TEST: dropna() doesn't fall into infinite loop #50751

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56498f9
TEST: dropna() doesn't fall into infinite loop
natmokval Jan 14, 2023
55e596a
Merge branch 'main' into 50708-dtypes-with-large-frames-causes-infini…
natmokval Jan 29, 2023
6374d43
add a link to the issue
natmokval Jan 30, 2023
d11a3d8
add decorator to handle timeout
natmokval Jan 30, 2023
fa8acb0
Merge branch 'main' into 50708-dtypes-with-large-frames-causes-infini…
MarcoGorelli Jan 30, 2023
aed97cc
Merge branch 'main' into 50708-dtypes-with-large-frames-causes-infini…
natmokval Mar 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions pandas/tests/frame/test_constructors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2457,6 +2457,32 @@ def test_constructor_list_str_na(self, string_dtype):
expected = DataFrame({"A": ["1.0", "2.0", None]}, dtype=object)
tm.assert_frame_equal(result, expected)

def test_constructor_large_size_frame(self):
Copy link
Member

Choose a reason for hiding this comment

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

ah sorry, missed the GH reference - could you add a link to the issue #50708 as a comment please?

class LargeFrame(DataFrame):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
for col in self.columns:
if self.dtypes[col] == "O":
self[col] = pd.to_numeric(self[col], errors="ignore")

@property
def _constructor(self):
return type(self)

def get_frame(N):
return LargeFrame(
data=np.vstack(
[
np.where(np.random.rand(N) > 0.36, np.random.rand(N), np.nan)
for _ in range(2)
]
).T
)

frame = get_frame(1000000)
# check that dropna() doesn't fall into an infinite loop
frame.dropna()
Copy link
Member

@phofl phofl Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a way of breaking out of the infinite loop? This might cause weird ci Timeouts if we run into this bug again in the future

Copy link
Member

Choose a reason for hiding this comment

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

good point - in the notebook where I did git bisect I did

class TimeoutError(Exception):
    pass

def timeout(seconds=10, error_message=os.strerror(errno.ETIME)):
    def decorator(func):
        def _handle_timeout(signum, frame):
            raise TimeoutError(error_message)

        @functools.wraps(func)
        def wrapper(*args, **kwargs):
            signal.signal(signal.SIGALRM, _handle_timeout)
            signal.alarm(seconds)
            try:
                result = func(*args, **kwargs)
            finally:
                signal.alarm(0)
            return result

        return wrapper

    return decorator

@timeout(5)
def long_running_function(n):
    get_frame(n).dropna()

, taken from https://stackoverflow.com/a/2282656/4451315

Perhaps this test could be rewritten to use that, with a comment indicating why (and linking to the stackoverflow answer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @MarcoGorelli. I will rewrite the test to avoid ci Timeout.

Copy link
Member

Choose a reason for hiding this comment

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

If the previous code was failing with an recursion error, I would recommend setting sys.setrecursionlimit instead (I've done this with other tests that used to recursively fail

        rec_limit = sys.getrecursionlimit()
        try:
            sys.setrecursionlimit(100)
            code_that_fails()
        finally:
            sys.setrecursionlimit(rec_limit)

Copy link
Member

Choose a reason for hiding this comment

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

I think this was just an infinite loop iirc, can check later

Copy link
Member

Choose a reason for hiding this comment

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

yeah it still hangs forever in 1.5 with sys.setrecursionlimit for me

Copy link
Member

Choose a reason for hiding this comment

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

@mroeschke any objections to the current solution?

When I tried sys.setrecursionlimit it didn't work for me

Copy link
Member

@mroeschke mroeschke Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am still a little weary about adding a test like this despite the timeout protection. Is it known what caused the infinite loop before?

Copy link
Member

Choose a reason for hiding this comment

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

sure, good point

From https://www.kaggle.com/code/marcogorelli/pandas-regression-example?scriptVersionId=117662725, I'm seeing #49551 as the commit that fixed it, but I don't really see why that would be the case


@pytest.mark.parametrize("copy", [False, True])
def test_dict_nocopy(
self,
Expand Down