Skip to content
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

Add explain_pickle module; allow overriding class lookup for unpickling #5483

Closed
sagetrac-cwitty mannequin opened this issue Mar 11, 2009 · 13 comments
Closed

Add explain_pickle module; allow overriding class lookup for unpickling #5483

sagetrac-cwitty mannequin opened this issue Mar 11, 2009 · 13 comments

Comments

@sagetrac-cwitty
Copy link
Mannequin

sagetrac-cwitty mannequin commented Mar 11, 2009

explain_pickle is an unpickler (intended to be totally compatible with the cPickle unpickler) that produces Sage source code, which can then be evaluated by sage_eval. This is useful to see exactly how the unpickle process works. For example:

sage: explain_pickle(dumps(3))

pg_make_integer = unpickle_global('sage.rings.integer', 'make_integer')
pg_make_integer('3')
sage: explain_pickle(dumps(3), in_current_sage=True)

from sage.rings.integer import make_integer
make_integer('3')

I think the code works, but I'm not done writing documentation and doctests.

CC: @williamstein

Component: misc

Author: Carl Witty

Reviewer: Nicolas Thiery, William Stein

Merged: 4.0.1.rc0

Issue created by migration from https://trac.sagemath.org/ticket/5483

@sagetrac-cwitty sagetrac-cwitty mannequin added this to the sage-4.0.1 milestone Mar 11, 2009
@sagetrac-cwitty sagetrac-cwitty mannequin self-assigned this Mar 11, 2009
@nthiery
Copy link
Contributor

nthiery commented Apr 16, 2009

comment:1

I am not technically qualified to review this, patch it has been in the sage-combinat queue for a couple weeks, and has proven really useful! So +1 from Florent and myself.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Apr 16, 2009

comment:2

Carl: Can you change the summary in case this patch is ready for review?

I changed it so that this ticket is picked up by the right report.

Cheers,

Michael

@sagetrac-mabshoff sagetrac-mabshoff mannequin changed the title [with preliminary patch, not ready for review, request comments] Add explain_pickle module; allow overriding class lookup for unpickling [not ready for review, request comments] Add explain_pickle module; allow overriding class lookup for unpickling Apr 16, 2009
@sagetrac-cwitty
Copy link
Mannequin Author

sagetrac-cwitty mannequin commented May 17, 2009

comment:3

Attachment: trac5483-explain-pickle-v2.patch.gz

I finally managed to finish writing the doctests (and fixed a few bugs in the process).

@sagetrac-cwitty sagetrac-cwitty mannequin changed the title [not ready for review, request comments] Add explain_pickle module; allow overriding class lookup for unpickling Add explain_pickle module; allow overriding class lookup for unpickling May 17, 2009
@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented May 19, 2009

comment:4

The new file(s) should get added to the reference manual so that people actually can read about them ;).

Cheers,

Michael

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor

nthiery commented May 22, 2009

comment:5

I have been using this patch on a regular basis without any issue. It is extremely useful, and very well and thoroughly documented.

The code itself is for the most part straightforward, yet pretty technical.
By its nature of the code, the included systematic unit tests should catch most potential bugs.
Checking the tests themselves would require a thorough knowledge of the cpickle protocol which I do not have. But which I trust Carl to have.
The included integration test (comparing the result with cPickle on all the sage pickle jar) is a positive sign.
Also this code is mainly intended for interactive users, so remaining bugs in them should not cause a threat to the rest of the Sage library.
Besides, this patch is a blocker for the category integration.

I am about to attach a short patch fixing some ReST stuff, and adding explain_pickle to the reference manual as recommended by Michael.

For all those reason, I would give my two thumbs up for a positive review. And for advertising this cool tool beyond the Sage world.

William: what do you think?

@nthiery
Copy link
Contributor

nthiery commented May 22, 2009

comment:6

Attachment: trac5483-explain-pickle-v2-review.patch.gz

Oral comment by William: no reason not to integrate this. Positive Review.

@mwhansen
Copy link
Contributor

mwhansen commented Jun 1, 2009

comment:7

I get the failure at http://sage.pastebin.com/m4bec1638

Carl, is it trivial?

@sagetrac-cwitty
Copy link
Mannequin Author

sagetrac-cwitty mannequin commented Jun 2, 2009

comment:8

This appears to be a difference in Python's repr(), possibly between Python 2.5.2 (from Sage 3.4.2 on my laptop, where I developed the patch) and Python 2.5.4 (from Sage 4.0 on sage.math).

Python 2.5.2:

>>> v = ([],)
>>> v[0].append(v)
>>> repr(v)
'([([...],)],)'

Python 2.5.4:

>>> v = ([],)
>>> v[0].append(v)
>>> repr(v)
'([(...)],)'

I don't have time to experiment further right now, but I would suggest changing the expected output to the new version, and if it's consistent across all the test platforms then chalk it up to a change in Python and call it good.

@mwhansen
Copy link
Contributor

mwhansen commented Jun 3, 2009

comment:9

I fixed the doctest.

Merged in 4.0.1.rc0.

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2009

Author: Carl Witty

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2009

Reviewer: Nicolas Thiery, William Stein

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2009

Merged: 4.0.1.rc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants