Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#5483 closed enhancement (fixed)

[with patch; positive review] Add explain_pickle module; allow overriding class lookup for unpickling

Reported by: cwitty Owned by: cwitty
Priority: major Milestone: sage-4.0.1
Component: misc Keywords: pickling
Cc: was Merged in: 4.0.1.rc0
Authors: Carl Witty Reviewers: Nicolas Thiery, William Stein
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nthiery)

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.

Attachments (2)

trac5483-explain-pickle-v2.patch (138.4 KB) - added by cwitty 10 years ago.
trac5483-explain-pickle-v2-review.patch (30.2 KB) - added by nthiery 10 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by nthiery

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.

comment:2 Changed 10 years ago by mabshoff

  • Summary changed from [with preliminary patch; not ready for review; request comments] Add explain_pickle module; allow overriding class lookup for unpickling to [with patch; not ready for review; request comments] Add explain_pickle module; allow overriding class lookup for unpickling

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

Changed 10 years ago by cwitty

comment:3 Changed 10 years ago by cwitty

  • Summary changed from [with patch; not ready for review; request comments] Add explain_pickle module; allow overriding class lookup for unpickling to [with patch; needs review] Add explain_pickle module; allow overriding class lookup for unpickling

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

comment:4 Changed 10 years ago by mabshoff

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

Cheers,

Michael

comment:5 Changed 10 years ago by nthiery

  • Cc was added; nthiery removed
  • Description modified (diff)
  • Keywords pickling added

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?

Changed 10 years ago by nthiery

comment:6 Changed 10 years ago by nthiery

  • Summary changed from [with patch; needs review] Add explain_pickle module; allow overriding class lookup for unpickling to [with patch; positive review] Add explain_pickle module; allow overriding class lookup for unpickling

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

comment:7 Changed 10 years ago by mhansen

  • Summary changed from [with patch; positive review] Add explain_pickle module; allow overriding class lookup for unpickling to [with patch; needs work] Add explain_pickle module; allow overriding class lookup for unpickling

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

Carl, is it trivial?

comment:8 Changed 10 years ago by cwitty

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.

comment:9 Changed 10 years ago by mhansen

  • Resolution set to fixed
  • Status changed from new to closed
  • Summary changed from [with patch; needs work] Add explain_pickle module; allow overriding class lookup for unpickling to [with patch; positive review] Add explain_pickle module; allow overriding class lookup for unpickling

I fixed the doctest.

Merged in 4.0.1.rc0.

comment:10 Changed 10 years ago by mhansen

  • Authors set to Carl Witty
  • Merged in set to 4.0.1.rc0
  • Reviewers set to Nicolas Thiery, William Stein
Note: See TracTickets for help on using tickets.