Skip to content

BUG: Propagate Python exceptions from c_is_list_like (#33721) #33723

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

Merged
merged 4 commits into from
Apr 27, 2020
Merged

BUG: Propagate Python exceptions from c_is_list_like (#33721) #33723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da1f74a
BUG: Propagate Python exceptions from c_is_list_like (#33721)
shwina Apr 22, 2020
a773ba5
Remove whatsnew entry
shwina Apr 23, 2020
25a0afe
Add test for recursion error thrown from is_list_like()
shwina Apr 24, 2020
50812d8
Merge branch 'master' into propagate-py-exception-lib-pyx
shwina Apr 27, 2020
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
2 changes: 1 addition & 1 deletion pandas/_libs/lib.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
cdef bint c_is_list_like(object, bint)
cdef bint c_is_list_like(object, bint) except -1
Copy link
Member

Choose a reason for hiding this comment

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

Hmm don't think a bint can ever return -1. The only thing I think you could except here is NULL but would really have to refactor to do that

Is there a particular issue you are trying to solve?

Copy link
Contributor Author

@shwina shwina Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for reviewing.

The ignored exception led to a segfault in https://github.com/rapidsai/cudf that I'm unable to reproduce at the moment since we've since refactored our code a bit -- apologies.

However, we can demonstrate the issue by raising artificially within c_is_list_like -- I believe it's valid to assume that Python exceptions can originate from within this function.

  1. Without the except -1:
cdef inline bint c_is_list_like(object obj, bint allow_sets):
    raise ZeroDivisionError()
    return (
        isinstance(obj, abc.Iterable)
        # we do not count strings/unicode/bytes as list-like
        and not isinstance(obj, (str, bytes))
        # exclude zero-dimensional numpy arrays, effectively scalars
        and not (util.is_array(obj) and obj.ndim == 0)
        # exclude sets if allow_sets is False
        and not (allow_sets is False and isinstance(obj, abc.Set))
    )
>>> import pandas as pd
>>> pd.api.types.is_list_like([])
ZeroDivisionError
Exception ignored in: 'pandas._libs.lib.c_is_list_like'
ZeroDivisionError:
False
  1. With the except -1:
cdef inline bint c_is_list_like(object obj, bint allow_sets) except -1:
    raise ZeroDivisionError()
    return (
        isinstance(obj, abc.Iterable)
        # we do not count strings/unicode/bytes as list-like
        and not isinstance(obj, (str, bytes))
        # exclude zero-dimensional numpy arrays, effectively scalars
        and not (util.is_array(obj) and obj.ndim == 0)
        # exclude sets if allow_sets is False
        and not (allow_sets is False and isinstance(obj, abc.Set))
    )
>>> import pandas as pd
>>>pd.api.types.is_list_like([])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pandas/_libs/lib.pyx", line 985, in pandas._libs.lib.is_list_like
    return c_is_list_like(obj, allow_sets)
  File "pandas/_libs/lib.pyx", line 989, in pandas._libs.lib.c_is_list_like
    raise ZeroDivisionError()
ZeroDivisionError

(-1 seems to be a valid value for bint)

Copy link
Member

Choose a reason for hiding this comment

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

The ignored exception led to a segfault

Any idea what value was being passed? Its hard to imagine what could cause this to raise unless you specifically rigged it to make isinstance checks fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha - so after digging up some old code, it looks the source of the problem was a RecursionError, which happened to be thrown within isinstance here.

Copy link
Contributor Author

@shwina shwina Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Minimal repro (segfaults for me):

In [1]: import pandas as pd

In [2]: def foo():
   ...:     pd.api.types.is_list_like("test")
   ...:     foo()

Copy link
Contributor

Choose a reason for hiding this comment

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

-1 is appropriate here as it signals cython that there maybe is an exception to check.

2 changes: 1 addition & 1 deletion pandas/_libs/lib.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,7 +988,7 @@ def is_list_like(obj: object, allow_sets: bool = True) -> bool:
return c_is_list_like(obj, allow_sets)


cdef inline bint c_is_list_like(object obj, bint allow_sets):
cdef inline bint c_is_list_like(object obj, bint allow_sets) except -1:
return (
isinstance(obj, abc.Iterable)
# we do not count strings/unicode/bytes as list-like
Expand Down
11 changes: 11 additions & 0 deletions pandas/tests/dtypes/test_inference.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,17 @@ def test_is_list_like_disallow_sets(maybe_list_like):
assert inference.is_list_like(obj, allow_sets=False) == expected


def test_is_list_like_recursion():
# GH 33721
# interpreter would crash with with SIGABRT
def foo():
inference.is_list_like([])
foo()

with pytest.raises(RecursionError):
foo()


def test_is_sequence():
is_seq = inference.is_sequence
assert is_seq((1, 2))
Expand Down