DEP: Undo deprecation for np.dtype() signature used by old pickles#31177
Conversation
Some (very) old pickles pass align=0, copy=1 as integers rather than bools. This just assumes no user will really do that, and allows it. It may be possible to also use `copyreg` to work around it, but since it is a rather specific signature this seems easier/just as well.
c2a2b90 to
86662bf
Compare
|
This fixes #30786, right? Ping @flying-sheep. |
|
Yeah, I just didn't check if the long discussion on docs warranted leaving it open. EDIT: but probably just as well to do. |
ngoldbaum
left a comment
There was a problem hiding this comment.
Storing old test data in a pickle file seems like a reasonable use-case so I'm happy to un-deprecate this. I spotted some incorrect C API uses below (with my own eyes, no Claude this time 😜).
It also maybe needs a release note? Kinda on the bubble. It is changing how the deprecations works a little though so that seems worth mentioning.
| // Some old pickles use 0, 1 exactly, assume no user passes it | ||
| // (It may also be possible to use `copyreg` instead.) | ||
| PyLong_CheckExact(oalign) && PyLong_AsLong(oalign) == 0 && | ||
| ocopy != NULL && PyLong_CheckExact(ocopy) && PyLong_AsLong(ocopy) == 1)) { |
There was a problem hiding this comment.
PyLong_AsLong can fail and set OverflowError: https://docs.python.org/3/c-api/long.html#c.PyLong_AsLong. Ditto for all the other PyLong_As functions. It also looks like PyLong_AsLong is soft-deprecated - maybe use PyLong_AsInt instead?
There was a problem hiding this comment.
Ack... yeah, annoying to add error handling here, but clearly needed...
PyLong_AsLong isn't soft depreated of course, PyLong_AS_LONG is.
There was a problem hiding this comment.
Well, I pushed a pretty annoying fix (annoying, because of how the control flow is).
I don't think this can ever fail, so I just made it a critical error, but I don't like baking this in when it isn't guaranteed in the C-API docs.
(The logic is really strange, because if it fails res == -1 the if branch is taken, tested locally by removing the PyLong_CheckExact which makes it possible to fail them)
|
I dunno, I was about to amend the 2.4.0 release notes to add a note that this corner case affecting pickles was solved in 2.4.5. But it feels a bit weird also? OTOH, I don't think a release note in 2.5 seems helpful. This eases a pain in 2.4-2.4.4 (2.4.5 should get this as a backport, I think), so the only interesting point would be where you actually run into the pickle loading failure... I'll add another comment to the original PR, I think that is the most likely place for someone actually to find the info... |
0c35300 to
1edf7b6
Compare
|
@seberg Thanks a lot for your work |
|
Thanks Sebastian. |
DEP: Undo deprecation for np.dtype() signature used by old pickles (#31177)
Some (very) old pickles pass align=0, copy=1 as integers rather than bools.
This just assumes no user will really do that, and allows it.
It may be possible to also use
copyregto work around it, but since it is a rather specific signature this seems easier/just as well.(No special tooling)
Closes gh-30786