Opened 10 years ago

Closed 10 years ago

#5985 closed defect (fixed)

[with spkg, with positive review] cPickle: adds support for class pickling customization

Reported by: nthiery Owned by: nthiery
Priority: critical Milestone: sage-4.2
Component: misc Keywords: cPickle, pickling classes
Cc: sage-combinat, cwitty, saliola, burcin, craigcitro Merged in: sage-4.2.alpha0
Authors: Craig Citro, Nicolas M. Thiéry Reviewers: Craig Citro, Nicolas M. Thiéry
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by 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:

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

Attachments (3)

cPickle-5985-import-submitted.patch (131.5 KB) - added by nthiery 10 years ago.
cPickle-5985-copy_reg_classes-submitted.patch (3.7 KB) - added by nthiery 10 years ago.
trac_5985_test_class_pickling.patch (2.8 KB) - added by mhansen 10 years ago.
Apply only this patch, after updating the python spkg linked to below

Download all attachments as: .zip

Change History (23)

comment:1 Changed 10 years ago by nthiery

  • Description modified (diff)
  • Milestone set to sage-4.0
  • Summary changed from cPickle: adds support for class pickling customizing and for nested classes to [with patch, needs review] cPickle: adds support for class pickling customization

comment:2 Changed 10 years ago by nthiery

  • Cc cwitty added

comment:3 Changed 10 years ago by saliola

  • Cc saliola added

comment:4 Changed 10 years ago by burcin

  • Cc burcin added

comment:5 Changed 10 years ago by nthiery

  • Description modified (diff)

comment:6 Changed 10 years ago by nthiery

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

comment:7 Changed 10 years ago by nthiery

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

comment:8 Changed 10 years ago by nthiery

Updated patch:

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

comment:9 Changed 10 years ago by nthiery

  • Status changed from new to assigned

comment:10 follow-up: Changed 10 years ago by mabshoff

  • Summary changed from [with patch, needs review] cPickle: adds support for class pickling customization to [with patch, needs work] cPickle: adds support for class pickling customization

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

comment:11 Changed 10 years ago by robertwb

Followup at #5986

Changed 10 years ago by nthiery

comment:12 in reply to: ↑ 10 Changed 10 years ago by nthiery

  • Summary changed from [with patch, needs work] cPickle: adds support for class pickling customization to [with patch, needs review] cPickle: adds support for class pickling customization

Replying to 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.

comment:13 follow-up: Changed 10 years ago by 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).

comment:14 in reply to: ↑ 13 ; follow-up: Changed 10 years ago by 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)

comment:15 in reply to: ↑ 14 ; follow-up: Changed 10 years ago by nthiery

  • Cc craigcitro added

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!

comment:16 in reply to: ↑ 15 Changed 10 years ago by nthiery

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 "<ipython console>", line 1, in <module>

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.

comment:17 follow-up: Changed 10 years ago by craigcitro

  • Summary changed from [with patch, needs review] cPickle: adds support for class pickling customization to [with spkg, needs review] cPickle: adds support for class pickling customization

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.

comment:18 in reply to: ↑ 17 Changed 10 years ago by nthiery

  • Authors set to Craig Citro, Nicolas M. Thiéry
  • Description modified (diff)
  • Milestone changed from sage-4.1.3 to sage-4.1.2
  • Priority changed from major to critical
  • Reviewers set to Craig Citro, Nicolas M. Thiéry

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

comment:19 Changed 10 years ago by craigcitro

  • Status changed from needs_review to positive_review
  • Summary changed from [with spkg, needs review] cPickle: adds support for class pickling customization to [with spkg, with positive review] cPickle: adds support for class pickling customization

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

Changed 10 years ago by mhansen

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

comment:20 Changed 10 years ago by mhansen

  • Merged in set to sage-4.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

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

Note: See TracTickets for help on using tickets.