BUG: introduce PyArray_SafeCast to fix issues around stringdtype views by ngoldbaum · Pull Request #26147 · numpy/numpy

ngoldbaum · 2024-03-27T20:17:09Z

This is a minimal implementation of a new function that generalizes PyArray_EquivTypes.

… views

seberg · 2024-03-27T20:40:43Z

numpy/_core/src/multiarray/multiarraymodule.c

+    if ((type1 != type2) && (type1->kind == 'T')) {
+        return 0;
+    }
+    return 1;


Need to dig slightly deeper and use this pattern:

numpy/numpy/_core/src/umath/ufunc_object.c

Lines 790 to 812 in 51fd714

if (dtypes[i] != PyArray_DESCR(op[i])) {

npy_intp view_offset;

NPY_CASTING safety = PyArray_GetCastInfo(

PyArray_DESCR(op[i]), dtypes[i], NULL, &view_offset);

if (safety < 0 && PyErr_Occurred()) {

/* A proper error during a cast check, should be rare */

return -1;

}

if (view_offset != 0) {

/* NOTE: Could possibly implement non-zero view offsets */

must_copy = 1;

}

if (force_cast_input && i < nin) {

/*

* ArrayMethod flagged to ignore casting (logical funcs

* can force cast to bool)

*/

}

else if (PyArray_MinCastSafety(safety, casting) != casting) {

return 0; /* the cast is not safe enough */

}

}

Making this not be string dtype specific!

ah nice and then I can control how view_offset gets set in string_to_string_resolve_descriptors, that seems much more idiomatic

So this idea doesn't quite work as cleanly as I'd hoped. If I set view_offset != 0 in string_to_string_resolve_descriptors, there's an assert that triggers if casting == NPY_NO_CASTING:

numpy/numpy/_core/src/multiarray/convert_datatype.c

Lines 465 to 482 in e1bf1d6

/*

* Check for less harmful non-standard returns. The following two returns

* should never happen:

* 1. No-casting must imply a view offset of 0.

* 2. Equivalent-casting + 0 view offset is (usually) the definition

* of a "no" cast. However, changing the order of fields can also

* create descriptors that are not equivalent but views.

* Note that unsafe casts can have a view offset. For example, in

* principle, casting `<i8` to `<i4` is a cast with 0 offset.

*/

if (*view_offset != 0) {

assert(casting != NPY_NO_CASTING);

}

else {

assert(casting != NPY_EQUIV_CASTING

|| (PyDataType_HASFIELDS(from) && PyDataType_HASFIELDS(to)));

}

return casting;

In the latest version I'm about to push I guard that assert with an if statement that checks for stringdtype, but that seems unsatisfying. Maybe the correct thing is for stringdtype to set e.g. NPY_EQUIV_CASTING for casts with distinct allocators, but when I tried doing that it broke reductions and I couldn't see a clear path forward on fixing that, because the check for valid reductions relies on PyArray_EquivTypes, which enfoces NPY_NO_CASTING as the minimum casting level:

numpy/numpy/_core/src/umath/ufunc_object.c

Lines 2394 to 2411 in e1bf1d6

/*

* The first operand and output should be the same array, so they should

* be identical. The second argument can be different for reductions,

* but is checked to be identical for accumulate and reduceat.

* Ideally, the type-resolver ensures that all are identical, but we do

* not enforce this here strictly. Otherwise correct handling of

* byte-order changes (or metadata) requires a lot of care; see gh-20699.

*/

if (!PyArray_EquivTypes(out_descrs[0], out_descrs[2]) || (

enforce_uniform_args && !PyArray_EquivTypes(

out_descrs[0], out_descrs[1]))) {

PyErr_Format(PyExc_TypeError,

"the resolved dtypes are not compatible with %s.%s. "

"Resolved (%R, %R, %R)",

ufunc_get_name_cstr(ufunc), method,

out_descrs[0], out_descrs[1], out_descrs[2]);

goto fail;

}

Maybe we need a new casting level between NO_CASTING and EQUIV_CASTING for this case where data can live outside the array buffer on the descriptor?

I think we can probably just remove the assert, just need to make sure to not document anywhere that no-casting implies view (I doubt we use it anywhere, but not sure). It used to, for these strings it can't since they use a whole new mechanism for storage... that seems fine.
(you could keep the assert together with checking that the dtype implements the hook for fixing the dtype in the array creation call, given that reason.)

ngoldbaum · 2024-03-28T17:19:54Z

numpy/_core/src/multiarray/multiarraymodule.c

+         * We ignore that possibility for simplicity; it really is not our bug.
+         */
+        return 0;
+    }


Is this still necessary here and in EquivTypes?

I think we can remove it, as it I am very certain it was fixed in boost python a while ago.

ngoldbaum · 2024-03-28T17:20:47Z

numpy/_core/src/multiarray/multiarraymodule.c

+        return 0;
+    }
+    /* If casting is "no casting" this dtypes are considered equivalent. */
+    return PyArray_MinCastSafety(safety, NPY_NO_CASTING) == NPY_NO_CASTING;


Note that most of this implementation is copied from PyArray_EquivTypes, to avoid calling GetCastInfo twice. If I just called EquivTypes, there would be an unnecessary second call to GetCastInfo to get the view_offset.

Fair, could add that as a code comment if you like.

charris · 2024-03-29T16:46:19Z

What is the status of this?

ngoldbaum · 2024-03-29T17:50:33Z

I think it just needs another round of review. It’s not critical for the RC but it would be good to have it in.

seberg

This is good, but the view offset needs fixing. I cannot review if other places using eqivtypes shojld be using this meaning, right now.

Its maybe subtle but only really affects strings, so with that fix happy to see it in. (Other comments could wait).

seberg · 2024-03-29T21:16:45Z

numpy/_core/src/multiarray/multiarraymodule.c

+         * We ignore that possibility for simplicity; it really is not our bug.
+         */
+        return 0;
+    }


I think we can remove it, as it I am very certain it was fixed in boost python a while ago.

seberg · 2024-03-29T21:17:50Z

numpy/_core/src/multiarray/multiarraymodule.c

+        return 0;
+    }
+    /* If casting is "no casting" this dtypes are considered equivalent. */
+    return PyArray_MinCastSafety(safety, NPY_NO_CASTING) == NPY_NO_CASTING;


Fair, could add that as a code comment if you like.

seberg · 2024-03-29T21:19:26Z

numpy/_core/src/multiarray/stringdtype/casts.c


-    *view_offset = 0;
+    // views are only legal between descriptors that share allocators (e.g. the same object)
+    *view_offset = descr0->allocator != descr1->allocator;


This is wrong. You must set it to 0 or leave it unchanged! (Intp min, I think)

seberg · 2024-03-29T21:21:48Z

numpy/_core/src/multiarray/multiarraymodule.c

+ * is true as well.
+ */
+NPY_NO_EXPORT unsigned char
+PyArray_ViewableTypes(PyArray_Descr *type1, PyArray_Descr *type2)


Maybe we can encode the equiv in the name? Or pass in the cast safety? Passing it in would make it work in the ufunc code I think.

But if not its OK.

seberg · 2024-03-29T21:32:08Z

numpy/_core/src/multiarray/multiarraymodule.c

        /* One more chance for faster exit if user specified the dtype. */
        oldtype = PyArray_DESCR(oparr);
-        if (PyArray_EquivTypes(oldtype, dtype)) {
+        unsigned char viewable = PyArray_ViewableTypes(oldtype, dtype);


The equiv cast may not matter, but then a single field struct to non-struct could return views... not today, I guess

ngoldbaum · 2024-03-29T23:00:43Z

Thanks for the prompt to refactor so I could use it in ufunc_object.c, the result is a bit more understandable and hopefully more useful.

Now instead of PyArray_ViewableTypes, I'm calling the new function PyArray_SafeCast, which is a generalization of PyArray_EquivTypes that accepts the minimum casting safety and also allows callers to see the view_offset value. I was able to use this new function in the trivial ufunc loop setup code.

ngoldbaum · 2024-03-29T23:01:31Z

Even though sebastian approved this, probably worth one more round of review before merging after the last commit.

mhvk

Looks good. Mostly just queries/requests for clarification

mhvk · 2024-03-31T15:27:35Z

numpy/_core/src/multiarray/ctors.c

           ((flags & NPY_ARRAY_WRITEABLE) &&
                   (!(arrflags & NPY_ARRAY_WRITEABLE))) ||
-           !PyArray_EquivTypes(oldtype, newtype);
+           !view_safe;


Since the actual work has now been done before defining copy, I think it should be the first item for copy, i.e.,

copy = !view_safe || ...

Alternatively, maybe better, since this now makes every case where a copy was already required slower, one can remove the last bit, and do the calculation like,

if (!copy) { /* Check a view is in fact possible */ npy_intp view_offset npy_intp is_safe = ... copy = copy && is_safe && (view_offset == 0); }

mhvk · 2024-03-31T15:34:28Z

numpy/_core/src/multiarray/multiarraymodule.c

+ * *minimum_safety* casting level. Sets the view_offset if that is set
+ * for the cast. If ignore_error is 1, errors in cast setup are ignored.
+ */
+NPY_NO_EXPORT npy_intp


The comment should perhaps add

... are ignored; otherwise the error is kept and -1 is returned

Thanks, remnant of an older version that unconditionally ignored errors.

mhvk · 2024-03-31T16:59:07Z

numpy/_core/src/multiarray/ctors.c

    arrflags = PyArray_FLAGS(arr);
+    npy_intp view_offset;
+    npy_intp is_safe = PyArray_SafeCast(oldtype, newtype, &view_offset, NPY_NO_CASTING, 1);
+    npy_intp view_safe = (is_safe && (view_offset == 0));


Probably fine as is, but in the code proper, impossible views are marked with view_offset = NPY_MIN_INTP

mhvk · 2024-03-31T16:59:52Z

numpy/_core/src/multiarray/multiarraymodule.c

-        if (PyArray_EquivTypes(oldtype, dtype)) {
+        npy_intp view_offset;
+        npy_intp is_safe = PyArray_SafeCast(oldtype, dtype, &view_offset, NPY_NO_CASTING, 1);
+        npy_intp view_safe = (is_safe && (view_offset == 0));


As above, should one check view_offset != NPY_MIN_INTP instead?

mhvk · 2024-03-31T17:02:29Z

numpy/_core/src/multiarray/multiarraymodule.h

 NPY_VISIBILITY_HIDDEN extern PyObject * npy_ma_str_cpu;
 NPY_VISIBILITY_HIDDEN extern PyObject * npy_ma_str_array_err_msg_substr;

+NPY_NO_EXPORT npy_intp


Is this the right module to define it in? I know PyArray_EquivTypes is here, but maybe casts.c makes more sense?

numpy/_core/src/umath/ufunc_object.c

numpy/_core/tests/test_stringdtype.py

ngoldbaum · 2024-04-01T20:37:04Z

All review comments should be resolved now

mhvk

Looks good! A suggestion for a tiny further improvement in-line, though probably barely worth the CI since the compiler will likely do it already.

mhvk · 2024-04-01T21:05:39Z

numpy/_core/src/multiarray/ctors.c

+    if (!copy) {
+        npy_intp view_offset;
+        npy_intp is_safe = PyArray_SafeCast(oldtype, newtype, &view_offset, NPY_NO_CASTING, 1);
+        copy = copy || !(is_safe && (view_offset != NPY_MIN_INTP));


My suggestion was imperfect: this can be copy = !(is_safe... since we know copy is false.

numpy/_core/src/umath/ufunc_object.c

ngoldbaum · 2024-04-03T14:12:29Z

Pulling this one in, thanks for the review!

BUG: introduce PyArray_ViewableTypes to fix issues around stringdtype…

a703ff3

… views

github-actions bot added the 00 - Bug label Mar 27, 2024

ngoldbaum added the 09 - Backport-Candidate PRs tagged should be backported label Mar 27, 2024

seberg reviewed Mar 27, 2024

View reviewed changes

ngoldbaum added 2 commits March 27, 2024 19:29

MNT: make PyArray_ViewableTypes use view_offset from casting setup

ae4be90

MNT: Only assert if finalize_descr isn't defined

b7f6de9

ngoldbaum commented Mar 28, 2024

View reviewed changes

seberg approved these changes Mar 29, 2024

View reviewed changes

seberg reviewed Mar 29, 2024

View reviewed changes

MAINT: refactor to pass casting safety in

a914726

ngoldbaum changed the title ~~BUG: introduce PyArray_ViewableTypes to fix issues around stringdtype views~~ BUG: introduce PyArray_SafeCast to fix issues around stringdtype views Mar 29, 2024

mhvk reviewed Mar 31, 2024

View reviewed changes

ngoldbaum added 2 commits April 1, 2024 14:31

MNT: respond to review comments

e16ad3b

MNT: check for valid views by comparing with NPY_MIN_INTP

54e3a83

mhvk approved these changes Apr 1, 2024

View reviewed changes

MNT: tiny optimization

6373b85

ngoldbaum mentioned this pull request Apr 2, 2024

BUG: ensure find-like ufuncs convert arguments to common dtypes #26198

Closed

ngoldbaum merged commit ffb23cd into numpy:main Apr 3, 2024

charris mentioned this pull request Apr 3, 2024

BUG: introduce PyArray_SafeCast to fix issues around stringdtype views #26205

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Apr 3, 2024

ngoldbaum mentioned this pull request Apr 19, 2024

BUG: use PyArray_SafeCast in array_astype #26317

Merged

charris mentioned this pull request Apr 23, 2024

BUG: use PyArray_SafeCast in array_astype #26332

Merged

ngoldbaum mentioned this pull request May 10, 2024

BUG: Passing a stringdtype array to np.where can lose string data #26420

Closed

ngoldbaum mentioned this pull request Nov 13, 2024

DEV: Audit internal uses of PyArray_EquivTypes #27751

Open

	if (dtypes[i] != PyArray_DESCR(op[i])) {
	npy_intp view_offset;
	NPY_CASTING safety = PyArray_GetCastInfo(
	PyArray_DESCR(op[i]), dtypes[i], NULL, &view_offset);
	if (safety < 0 && PyErr_Occurred()) {
	/* A proper error during a cast check, should be rare */
	return -1;
	}
	if (view_offset != 0) {
	/* NOTE: Could possibly implement non-zero view offsets */
	must_copy = 1;
	}

	if (force_cast_input && i < nin) {
	/*
	* ArrayMethod flagged to ignore casting (logical funcs
	* can force cast to bool)
	*/
	}
	else if (PyArray_MinCastSafety(safety, casting) != casting) {
	return 0; /* the cast is not safe enough */
	}
	}

	/*
	* Check for less harmful non-standard returns. The following two returns
	* should never happen:
	* 1. No-casting must imply a view offset of 0.
	* 2. Equivalent-casting + 0 view offset is (usually) the definition
	* of a "no" cast. However, changing the order of fields can also
	* create descriptors that are not equivalent but views.
	* Note that unsafe casts can have a view offset. For example, in
	* principle, casting `<i8` to `<i4` is a cast with 0 offset.
	*/
	if (*view_offset != 0) {
	assert(casting != NPY_NO_CASTING);
	}
	else {
	assert(casting != NPY_EQUIV_CASTING
	\|\| (PyDataType_HASFIELDS(from) && PyDataType_HASFIELDS(to)));
	}
	return casting;

	/*
	* The first operand and output should be the same array, so they should
	* be identical. The second argument can be different for reductions,
	* but is checked to be identical for accumulate and reduceat.
	* Ideally, the type-resolver ensures that all are identical, but we do
	* not enforce this here strictly. Otherwise correct handling of
	* byte-order changes (or metadata) requires a lot of care; see gh-20699.
	*/
	if (!PyArray_EquivTypes(out_descrs[0], out_descrs[2]) \|\| (
	enforce_uniform_args && !PyArray_EquivTypes(
	out_descrs[0], out_descrs[1]))) {
	PyErr_Format(PyExc_TypeError,
	"the resolved dtypes are not compatible with %s.%s. "
	"Resolved (%R, %R, %R)",
	ufunc_get_name_cstr(ufunc), method,
	out_descrs[0], out_descrs[1], out_descrs[2]);
	goto fail;
	}

Uh oh!

Conversation

ngoldbaum commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Mar 29, 2024

Uh oh!

ngoldbaum commented Mar 29, 2024

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Mar 29, 2024

Uh oh!

ngoldbaum commented Mar 29, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ngoldbaum commented Apr 1, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngoldbaum commented Apr 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ngoldbaum commented Mar 27, 2024 •

edited

Loading