cPickle: adds support for class pickling customization

[nthiery]

Original implementation:

The first patch imports the vanilla cPickle.c and test_cpickle.py
from python 2.5.2.p9 as sage.misc.cPickle, and updates accordingly the
cPickle imports throughout the sage library.

The second patch makes a very small modification to cPickle to allow
for customizing how class are pickled via metaclasses.

Final implementation: adds the second patch to the python spkg

See discussions on:

• http://groups.google.com/group/comp.lang.python/browse_thread/thread/66c282afc04aa39c/
• http://groups.google.com/group/sage-devel/browse_thread/thread/583048dc7d373d6a/

Thanks to Mike, Burcin, and Carl for suggestions on how to handle this.

CC: @sagetrac-sage-combinat @sagetrac-cwitty @saliola @burcin @craigcitro

Component: misc

Keywords: cPickle, pickling classes

Author: Craig Citro, Nicolas M. Thiéry

Reviewer: Craig Citro, Nicolas M. Thiéry

Merged: sage-4.2.alpha0

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

[nthiery]

comment:6

I just rebased the first patch upon 3.4.2 final. Please ignore (or better delete) the old version cPickle-5985-import.patch

[nthiery]

comment:7

Note: the cPickle imports in dsage will need to be updated as well.

[nthiery]

comment:8

Updated patch:

• Fixes a trivially failing doctest (don't know why I did not see them earlier)
• Adds some more doctest

[sagetrac-mabshoff]

comment:10

This patch needs an explicit addition of sage/misc/cPickle.c to MANIFEST.in for -sdist to work. It is also listed to have a positive review in the CategoriesRoadMap, but it isn't on the ticket.

Cheers,

Michael

[robertwb]

comment:11

Followup at #5986

[nthiery]

Attachment: cPickle-5985-import-submitted.patch.gz

[nthiery]

comment:12

Replying to @sagetrac-mabshoff:

This patch needs an explicit addition of sage/misc/cPickle.c to MANIFEST.in for -sdist to work.

The updated patch hopefully fixes this (please double check).

It is also listed to have a positive review in the CategoriesRoadMap, but it isn't on the ticket.

Oops, fixed. Someone was supposed to give a positive review, but apparently this has not occurred yet.

[craigcitro]

comment:13

I've rolled an spkg with the patch by Nicolas incorporated -- it's here:

http://sage.math.washington.edu/home/craigcitro/four-one/python-2.6.2.p2.spkg

It's not ready to be merged (I need to commit the changes in hg), but I'd like Nicolas to confirm that it still works before I play with it too much. Or, if there's an easy testcase, post that and I'll play with it.

I should have more time to look at this tonight (and give it a review, assuming it works).

[nthiery]

comment:14

Replying to @craigcitro:

I've rolled an spkg with the patch by Nicolas incorporated -- it's here:

http://sage.math.washington.edu/home/craigcitro/four-one/python-2.6.2.p2.spkg

It's not ready to be merged (I need to commit the changes in hg), but I'd like Nicolas to confirm that it still works before I play with it too much. Or, if there's an easy testcase, post that and I'll play with it.

I should have more time to look at this tonight (and give it a review, assuming it works).

Thanks for working on this!

The patch cPickle-5985-copy_reg_classes-submitted.patch includes a fairly complete testsuite (see the addition to sage/misc/test_cpickle_sage.py)

[nthiery]

comment:15

Attachment: cPickle-5985-copy_reg_classes-submitted.patch.gz

Replying to @nthiery:

Replying to @craigcitro:

I've rolled an spkg with the patch by Nicolas incorporated -- it's here:

http://sage.math.washington.edu/home/craigcitro/four-one/python-2.6.2.p2.spkg

It's not ready to be merged (I need to commit the changes in hg), but I'd like Nicolas to confirm that it still works before I play with it too much. Or, if there's an easy testcase, post that and I'll play with it.

I should have more time to look at this tonight (and give it a review, assuming it works).

Thanks for working on this!

The patch cPickle-5985-copy_reg_classes-submitted.patch includes a fairly complete testsuite (see the addition to sage/misc/test_cpickle_sage.py)

Oops, this testsuite used type.new(...) which apparently does not work anymore with Python 2.6. I just rewrote the testsuite so that it does not use this feature anymore. See attached patch.

Luckily enough, the real applications of this patch readily did not use this feature anymore!

[nthiery]

comment:16

Replying to @nthiery:

Oops, this testsuite used type.new(...) which apparently does not work anymore with Python 2.6. I just rewrote the testsuite so that it does not use this feature anymore. See attached patch.

For the record:

zephyr-/opt/sage-4.0.2>./sage

---

| Sage Version 4.0.2, Release Date: 2009-06-18 |
| Type notebook() for the GUI, and license() for information. |

---

Loading Sage library. Current Mercurial branch is: combinat
sage: class metaclass(type):
....: def new(cls):
....: return type.new(cls, "bla", (object,), dict())
....:
sage: metaclass()
<class 'main.bla'>

zephyr-~>sage

---

| Sage Version 4.1, Release Date: 2009-07-09 |
| Type notebook() for the GUI, and license() for information. |

---

Loading Sage library. Current Mercurial branch is: combinat
sage: class metaclass(type):
....: def new(cls):
....: return type.new(cls, "bla", (object,), dict())
....:
sage: metaclass()

---

Traceback (most recent call last):
File "", line 1, in
TypeError: type.init() takes 1 or 3 arguments

Apparently type.new now calls type.init. And I assume that can only be a change in python, not in Sage.

[craigcitro]

comment:17

I'm uploading a new python spkg, which is the same as the previous one I posted, but based on the most recent python spkg in Sage. It's here:

http://sage.math.washington.edu/home/craigcitro/python-2.6.2.p4.spkg

This fixes the issue by patching the files in our python spkg instead of importing and using our own copy of cPickle. It also makes the corresponding changes to pickle.

[nthiery]

comment:18

Replying to @craigcitro:

I'm uploading a new python spkg, which is the same as the previous one I posted, but based on the most recent python spkg in Sage. It's here:

http://sage.math.washington.edu/home/craigcitro/python-2.6.2.p4.spkg

This fixes the issue by patching the files in our python spkg instead of importing and using our own copy of cPickle. It also makes the corresponding changes to pickle.

Sounds good to me! Positive review, up to someone double checking the new patch I attached which imports the test file from the original version of the patch.

Carl, Craig, Robert, please do it soon! William is ok integrating this in 4.1.2 (see IRC).
I set back the release to 4.1.2

For the author / reviewer, I don't know exactly what to do since I wrote the first version, Craig reviewed it, wrote the second version which I reviewed :-) Please set whatever you feel appropriate

[nthiery]

Reviewer: Craig Citro, Nicolas M. Thiéry

[nthiery]

Author: Craig Citro, Nicolas M. Thiéry

[craigcitro]

comment:19

I'm happy with Nicolas's patch to test the new python spkg -- let's finally get this merged! Yay!

[mwhansen]

Attachment: trac_5985_test_class_pickling.patch.gz

Apply only this patch, after updating the python spkg linked to below

[mwhansen]

Merged: sage-4.2.alpha0

[mwhansen]

comment:20

I updated the patch to do "import cPickle" instead of importing it from sage.misc. After that, everything passes.