-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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.
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
REGR: Fix reading old pickles from pandas 1.3.5 #56308
Conversation
pandas/core/internals/blocks.py
Outdated
|
||
# Old pandas (<=1.3.0) will call this function with placements | ||
# that aren't necessarily BlockPlacements when unpickling | ||
if not isinstance(placement, BlockPlacement): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be done in _setstate__
or something? a lot of effort has gone into optimizing this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also maybe deprecate reading of such-old pickles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea on deprecating reading old pickles, I'll do that as a follow-up (1.3.5 is 2 years old at this point I think).
How would I change __setstate__
? I'm not familiar at all with pickle stuff.
I thought __reduce__
calls new_block
directly in 1.3.5.
pandas/pandas/_libs/internals.pyx
Lines 485 to 493 in 66e3805
cpdef __reduce__(self): | |
# We have to do some gymnastics b/c "ndim" is keyword-only | |
from functools import partial | |
from pandas.core.internals.blocks import new_block | |
args = (self.values, self.mgr_locs.indexer) | |
func = partial(new_block, ndim=self.ndim) | |
return func, args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah, __reduce__
sounds like the right place for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, the old __reduce__
(from 1.3.5) is getting called though, which is leading to the problem, since it directly calls new_block
.
The newer reduce goes through _unpickle_block
, which does the conversion to BlockPlacement
for us.
Are we OK just taking the perf hit for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need patching in compat.pickle_compat then. That is always a PITA.
i think id be against taking the perf hit for something so old since it would be hit a zillion times.
Huh, the pickle_compat route is even simpler. I just re-routed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thanks so much for looking into this @lithomas1 |
…m pandas 1.3.5) (#56335) Backport PR #56308: REGR: Fix reading old pickles from pandas 1.3.5 Co-authored-by: Thomas Li <[email protected]>
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.