TEST: dropna() doesn't fall into infinite loop #50751
Conversation
|
Hi @MarcoGorelli. Could you please take a look at the test. |
There was a problem hiding this comment.
Looks good to me - this a bit smaller than in the OP, but still enough to reproduce the bug on 1.5.2, and fast enough to not clog up the test suite on main
cc @phofl as you'd commented on the issue - any objections?
| @@ -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): | |||
There was a problem hiding this comment.
ah sorry, missed the GH reference - could you add a link to the issue #50708 as a comment please?
MarcoGorelli
added
Needs Tests
|
|
||
| frame = get_frame(1000000) | ||
| # check that dropna() doesn't fall into an infinite loop | ||
| frame.dropna() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Thank you, @MarcoGorelli. I will rewrite the test to avoid ci Timeout.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I think this was just an infinite loop iirc, can check later
There was a problem hiding this comment.
yeah it still hangs forever in 1.5 with sys.setrecursionlimit for me
There was a problem hiding this comment.
@mroeschke any objections to the current solution?
When I tried sys.setrecursionlimit it didn't work for me
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
should be good (pre-commit isort failure is unrelated, and solved in #51048 - I just hit the "update branch" button to merge upstream/main and restart it just to be sure) |
|
Thank you, @MarcoGorelli for reviewing the PR. |
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
Hi @MarcoGorelli. I merged upstream/main into my branch. Could you please have a look? |
|
thanks - looks like the test is failing now it might be worth doing a dive into setting what caused the timeout in the first place - I could take a look next week |
|
Looks like this PR has stalled so closing to clear the queue |
|
thank you, @mroeschke, I should have closed this pr myself earlier. |
.dtypesin a subclass constructor with large frames causes infinite loop #50708I have checked that the problem was reproducible in 1.5.2 and is not reproducible on main. The added test
test_constructor_large_size_framechecks if an infinite loop occurs.