Opened 8 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 )
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.
Attachments (3)
Change History (23)
comment:1 Changed 8 years ago by
- 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 8 years ago by
- Cc cwitty added
comment:3 Changed 8 years ago by
- Cc saliola added
comment:4 Changed 8 years ago by
- Cc burcin added
comment:5 Changed 8 years ago by
- Description modified (diff)
comment:6 Changed 8 years ago by
comment:7 Changed 8 years ago by
Note: the cPickle imports in dsage will need to be updated as well.
comment:8 Changed 8 years ago by
Updated patch:
- Fixes a trivially failing doctest (don't know why I did not see them earlier)
- Adds some more doctest
comment:9 Changed 8 years ago by
- Status changed from new to assigned
comment:10 follow-up: ↓ 12 Changed 8 years ago by
- 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 8 years ago by
Followup at #5986
Changed 8 years ago by
comment:12 in reply to: ↑ 10 Changed 8 years ago by
- 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: ↓ 14 Changed 8 years ago by
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: ↓ 15 Changed 8 years ago by
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)
Changed 8 years ago by
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 8 years ago by
- 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
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: ↓ 18 Changed 8 years ago by
- 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
- 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 topickle
.
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
- 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!
comment:20 Changed 8 years ago by
- 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.
I just rebased the first patch upon 3.4.2 final. Please ignore (or better delete) the old version cPickle-5985-import.patch