ENH: mark fixed-width string to StringDType casts as safe - #32095
Conversation
ngoldbaum
added
01 - Enhancement
56 - Needs Release Note.
MaanasArora
left a comment
MaanasArora
left a comment
There was a problem hiding this comment.
Thanks, just a few typos an AI agent found but looks good to me! Largely seems straightforward as a new function + merge. I don't know too much about RAII, but seemed fine from a brief look, and the early returns are nice :)
Approving, I think both #32097 and this are ready!
| @@ -0,0 +1,12 @@ | |||
| Casting and validation changes for fixed-width to variable-width string conversions | |||
| ----------------------------------------------------------------------------------- | |||
| * Casts from fixed-width `numpy.bytes_` and `nummpy.str_` arrays to | |||
There was a problem hiding this comment.
| * Casts from fixed-width `numpy.bytes_` and `nummpy.str_` arrays to | |
| * Casts from fixed-width `numpy.bytes_` and `numpy.str_` arrays to |
| * Casts from fixed-width `numpy.bytes_` and `nummpy.str_` arrays to | ||
| `numpy.dtypes.StringDType` are now considered `"safe"` rather than | ||
| `"same-kind"`. | ||
| * These cast from `numpy.bytes_` to `numpy.dtypes.StringDType` now validates the |
There was a problem hiding this comment.
| * These cast from `numpy.bytes_` to `numpy.dtypes.StringDType` now validates the | |
| * These casts from `numpy.bytes_` to `numpy.dtypes.StringDType` now validate the |
…StringDType casts raises UnicodeDecodeError
e80d324 to
73b95d1
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looks very nice! I have one question/suggestion about the error handling, the other comment is just that, a comment and not-directly-relevant query.
| PyObject *decoded = PyUnicode_Decode(bad, bad_size, "utf-8", | ||
| "strict"); | ||
| PyMem_RawFree(bad); | ||
| if (decoded != NULL) { |
There was a problem hiding this comment.
Shouldn't one always have decoded == NULL? I.e., does the existence of this branch mean the above succeeded? Isn't that weird enough to give a RuntimeError instead, asking to report?
There was a problem hiding this comment.
This is mostly here defensively and indeed it shouldn't be possible to get here if NumPy's UTF-8 validation is working correctly.
I asked an AI to come up with a scenario that triggers this, and it turns out that you can actually add error handlers for python's UTF-8 validation: https://docs.python.org/3/library/codecs.html#codecs.register_error. If someone added their own error handler (why??) it's possible this error could happen.
We could also get here if there's a multithreaded race to wrote to a np.bytes_ array while a simultaneous cast to StringDType is running.
Both of those aren't a NumPy bug and it would be confusing if the error was written like it has to be.
There was a problem hiding this comment.
Ah, I see, but then my suggestion of a RuntimeError saying something weird happened is probably more appropriate. Interesting that it is possible at all...
There was a problem hiding this comment.
Fair enough. I just made it a RuntimeError.
| npy_gil_error(PyExc_TypeError, | ||
| "Invalid UTF-8 bytes found, cannot convert to UTF-8"); | ||
| goto fail; | ||
| np::raii::NpyStringAcquireAllocator alloc(descr); |
There was a problem hiding this comment.
I guess there has been a change here that I missed! Is this new way of dealing with the allocator used everywhere, or are you just changing as you touch relevant code?
There was a problem hiding this comment.
@WarrenWeckesser added it late last year. I think it makes sense to use it in C++ files like this one.
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
OK, as that was my only remaining comment, now all OK. Feel free to merge once the tests have run.
|
Thanks all, I really appreciate the help and advice you both have been giving me for the StringDType improvements I've been doing this summer. |
3 participants
PR summary
Towards fixing #32031.
This is one half of what #32031 proposes. The implementation ended up allowing me to delete a bunch of code, since the function I'm changing was the only customer of an old helper function that I inlined. I also made the void and bytes casts share the same loop, although I'm not touching void cast safety here.
AI Disclosure
I used an AI model to help debug the change.