Opened 9 years ago

Closed 8 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 9 years ago.
cPickle-5985-copy_reg_classes-submitted.patch (3.7 KB) - added by nthiery 8 years ago.
trac_5985_test_class_pickling.patch (2.8 KB) - added by mhansen 8 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 9 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 9 years ago by nthiery

  • Cc cwitty added

comment:3 Changed 9 years ago by saliola

  • Cc saliola added

comment:4 Changed 9 years ago by burcin

  • Cc burcin added

comment:5 Changed 9 years ago by nthiery

  • Description modified (diff)

comment:6 Changed 9 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 9 years ago by nthiery

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

comment:8 Changed 9 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 9 years ago by nthiery

  • Status changed from new to assigned

comment:10 follow-up: Changed 9 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 9 years ago by robertwb

Followup at #5986

Changed 9 years ago by nthiery

comment:12 in reply to: ↑ 10 Changed 9 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 8 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 8 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 8 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 8 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 8 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 8 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 8 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 8 years ago by mhansen

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

comment:20 Changed 8 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.