Skip to content
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

Jump to bottom

API: Check for integer overflows when creating scalar form python int #16140

Merged
merged 5 commits into from
Jul 15, 2024
Merged

API: Check for integer overflows when creating scalar form python int #16140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2174f56
API: Change Scalar to raise for invalid Python values
seberg May 31, 2024
14b9a09
TST: Add test to check scalar conversion behavior for python ints
seberg Jul 1, 2024
2e1eb4a
MAINT: Use `int8(-1)` as default categorical as `np.uint8(-1)` fails
seberg May 31, 2024
174f2b3
TST: Fixup test mistake
seberg Jul 2, 2024
39d240f
Use packaging.version for numpy version check as suggested
seberg Jul 12, 2024
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
4 changes: 3 additions & 1 deletion python/cudf/cudf/core/column/categorical.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@
)


_DEFAULT_CATEGORICAL_VALUE = -1
# Using np.int8(-1) to allow silent wrap-around when casting to uint
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing we should disallow this cast if this happens, but not for this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not following up. NumPy allows np.int8(-1) to be cast to silently to other integers pretty easily including unsigned, which is what we rely on here.

If NumPy wants to change that, the _DEFAULT_CATEGORICAL_VALUE might have to be some mapping/function based on the actual dtype used, but for now this works.

# it may make sense to make this dtype specific or a function.
_DEFAULT_CATEGORICAL_VALUE = np.int8(-1)


class CategoricalAccessor(ColumnMethods):
Expand Down
17 changes: 17 additions & 0 deletions python/cudf/cudf/tests/test_scalar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import pandas as pd
import pyarrow as pa
import pytest
from packaging import version

import rmm

Expand Down Expand Up @@ -253,6 +254,22 @@ def test_generic_null_scalar_construction_fails(value):
cudf.Scalar(value)


@pytest.mark.parametrize(
"value, dtype", [(1000, "uint8"), (2**30, "int16"), (-1, "uint16")]
)
@pytest.mark.filterwarnings("ignore::DeprecationWarning")
def test_scalar_out_of_bounds_pyint_fails(value, dtype):
# Test that we align with NumPy on scalar creation behavior from
# Python integers.
if version.parse(np.__version__) >= version.parse("2.0"):
with pytest.raises(OverflowError):
cudf.Scalar(value, dtype)
else:
# NumPy allowed this, but it gives a DeprecationWarning on newer
# versions (which cudf did not used to do).
assert cudf.Scalar(value, dtype).value == np.dtype(dtype).type(value)


@pytest.mark.parametrize(
"dtype", NUMERIC_TYPES + DATETIME_TYPES + TIMEDELTA_TYPES + ["object"]
)
Expand Down
5 changes: 4 additions & 1 deletion python/cudf/cudf/tests/test_unaops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ def generate_valid_scalar_unaop_combos():
@pytest.mark.parametrize("slr,dtype,op", generate_valid_scalar_unaop_combos())
def test_scalar_unary_operations(slr, dtype, op):
slr_host = np.array([slr])[0].astype(cudf.dtype(dtype))
slr_device = cudf.Scalar(slr, dtype=dtype)
# The scalar may be out of bounds, so go via array force-cast
# NOTE: This is a change in behavior
slr = np.array(slr).astype(dtype)[()]
slr_device = cudf.Scalar(slr)

expect = op(slr_host)
got = op(slr_device)
Expand Down
14 changes: 8 additions & 6 deletions python/cudf/cudf/utils/dtypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,16 +253,18 @@ def to_cudf_compatible_scalar(val, dtype=None):
elif isinstance(val, datetime.timedelta):
val = np.timedelta64(val)

val = _maybe_convert_to_default_type(
cudf.api.types.pandas_dtype(type(val))
).type(val)

if dtype is not None:
if isinstance(val, str) and np.dtype(dtype).kind == "M":
dtype = np.dtype(dtype)
if isinstance(val, str) and dtype.kind == "M":
# pd.Timestamp can handle str, but not np.str_
val = pd.Timestamp(str(val)).to_datetime64().astype(dtype)
else:
val = val.astype(dtype)
# At least datetimes cannot be converted to scalar via dtype.type:
val = np.array(val, dtype)[()]
else:
val = _maybe_convert_to_default_type(
cudf.api.types.pandas_dtype(type(val))
).type(val)

if val.dtype.type is np.datetime64:
time_unit, _ = np.datetime_data(val.dtype)
Expand Down
Loading