-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: Exception in pickle loading #28645
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
Conversation
@jorisvandenbossche @datapythonista since the CI failure is in web_and_docs. It looks like there is a problem trying to unpickle something from pytables? is this something we should have a regular test for? |
The error is in the Otherwise adding a new tests sounds good. |
Well, the docs is just code that is ran with the latest pandas. So there is a line of code that is working on master and failing with this branch (the pytables thing you see at the end is something else (ignored), you need to scroll up in the logs to find the actual error). It's this (assuming 'pandas' is your local cloned repo):
So we have a pickle file that reads fine on master, and the changes in this branch broke that. Now, I assume the pickle file was written with pandas 0.8.0, so we don't guarantee support for that anymore, so it might not be a problem that you are breaking it (although t might be useful to understand why, to be sure it will not be a problem for more recent versions of pandas) The error on this branch is:
|
pandas/io/pickle.py
Outdated
try: | ||
return pc.load(f, encoding=None) | ||
except Exception: | ||
except UnicodeDecodeError: |
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't remove that code in 0.8.0 whatsnew? (e.g. make it a code-block)?
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.
looks like this worked
Note that by the changes in this PR, we now have a pickle file generated by pandas that we cannot longer read. So instead of just making this a code block, I would at least investigate a bit the reason why this happens to make sure this is not a valid use case that is now breaking. |
not sure I undertstand this last point. we no longer guarntee compatiblity for pickles < 0.20.3 (we did this in 0.25), so not sure this is an issue at all. |
I am not sure either. But if we are not sure, we should investigate it before just removing the code. The pickle file contains a dataframe with just float columns, so doesn't seem something that should break to me. |
So since it was about encoding errors, I tried to write a simple pickle file with pandas 0.24.2 in Python 2, and read it with master and this branch:
and reading with master works, with this branch I get:
|
@jorisvandenbossche excellent, can you make a test case out of this example? i'm fine with pinning it into this branch or seeing it go in first |
updated with a test based on Joris's example |
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.
Looks good! small comment
|
||
import pandas as pd | ||
fx = pd.read_pickle('data/fx_prices') | ||
import matplotlib.pyplot as plt | ||
|
||
``Series.plot`` now supports a ``secondary_y`` option: | ||
|
||
.. ipython:: python | ||
.. code-block:: python |
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 you turn those again into ipython blocks 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.
@jbrockmendel do we need to revert 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.
this was reverted and then un-reverted, following #28645 (comment)
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.
cool
@jbrockmendel the doc example still gives problems after restoring the py2 compat? |
Yes. Re-disabling sounds goods. |
re-disabled and green |
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.
any outstanding issues here? |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff