Opened 8 years ago

Closed 8 years ago

#5986 closed defect (fixed)

[with patch, positive review] Workaround mishandled nested classes in Python and cPickle

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

Description (last modified by nthiery)

With the python code below::

class A:

class B:

pass

Python 2.6 erroneously set the B.name to "B" instead of "A.B".

Furthermore, upon pickling (here in save_global) *and* unpickling (in load_global) a class with name "A.B" in a module mod, the standard cPickle module searches for "A.B" in mod.dict instead of looking up "A" and then "B" in the result.

This patch works around this by a patch to cPickle.c (in fact a copy of it in the Sage source tree; see #5985) which fixes the name for B to its appropriate value A.B, and inserts 'A.B' = A.B in mod.dict (hacky, but seems to work) the first time A.B is pickled, and fixes load_global to implement a proper lookup upon unpickling.

It also ensures that sage/interfaces/sage0.py uses loads/dumps from sage_object rather than calling directly cPickle.loads/dumps (+1 by cwitty on this change)

Python source experts are more than welcome to rework/rewrite this patch!

Depends on #5483 and #5985

Attachments (2)

cPickle-5986-nested-classes-submitted.patch​ (11.4 KB) - added by nthiery 8 years ago.
trac_5986-use-nested_class_metaclass.patch​ (17.5 KB) - added by nthiery 8 years ago.
Apply only this one

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by nthiery

  • Cc cwitty added
  • Description modified (diff)

Changed 8 years ago by nthiery

comment:2 Changed 8 years ago by nthiery

  • Description modified (diff)

comment:3 Changed 8 years ago by nthiery

  • Status changed from new to assigned

comment:4 follow-up: Changed 8 years ago by robertwb

This workaround it a bit to hackish for my taste, but it's been implemented and tested. Followup at #6121.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by jason

Replying to robertwb:

This workaround it a bit to hackish for my taste, but it's been implemented and tested. Followup at #6121.

Does that mean that this ticket can be closed?

comment:6 in reply to: ↑ 5 Changed 8 years ago by nthiery

  • Authors set to Nicolas M. Thiéry

Replying to jason:

Replying to robertwb:

This workaround it a bit to hackish for my taste, but it's been implemented and tested. Followup at #6121.

Does that mean that this ticket can be closed?

Not before Robert or someone else writes a proof of concept patch upon the category code proving that #6121 is a usable replacement for this one to get pickling to work for parents and categories. See discussion on Sage devel.

Changed 8 years ago by nthiery

Apply only this one

comment:7 Changed 8 years ago by nthiery

  • Authors changed from Nicolas M. Thiéry to Craig Citro, Nicolas M. Thiéry
  • Milestone changed from sage-4.1.3 to sage-4.1.2
  • Reviewers set to Robert Bradshaw

The newly attached patch implements a completely different fix, using #6110 and #6121.

William is ok integrating this in 4.1.2 if it's ready on time (see IRC).

Robert: please review! (unless you feel you should be an author, and get someone else to review it :-))

comment:8 Changed 8 years ago by robertwb

  • Status changed from needs_review to positive_review
  • Summary changed from [with patch, needs review] Workaround mishandled nested classes in Python and cPickle to [with patch, positive review] Workaround mishandled nested classes in Python and cPickle

Much less intrusive--too bad we didn't pursue this just a bit more back in June.

comment:9 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
Note: See TracTickets for help on using tickets.