Ticket #5986 (closed defect: fixed)

Opened 10 months ago

Last modified 5 months ago

[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 Author(s): Craig Citro, Nicolas M. Thiéry
Report Upstream: Reviewer(s): Robert Bradshaw
Merged in: sage-4.2.alpha0 Work issues:

Description (last modified by nthiery) (diff)

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

cPickle-5986-nested-classes-submitted.patch Download (11.4 KB) - added by nthiery 10 months ago.
trac_5986-use-nested_class_metaclass.patch Download (17.5 KB) - added by nthiery 5 months ago.
Apply only this one

Change History

  Changed 10 months ago by nthiery

  • cc cwitty added
  • description modified (diff)

Changed 10 months ago by nthiery

  Changed 10 months ago by nthiery

  • description modified (diff)

  Changed 10 months ago by nthiery

  • status changed from new to assigned

follow-up: ↓ 5   Changed 10 months ago by robertwb

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

in reply to: ↑ 4 ; follow-up: ↓ 6   Changed 6 months 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?

in reply to: ↑ 5   Changed 6 months ago by nthiery

  • author 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 5 months ago by nthiery

Apply only this one

  Changed 5 months ago by nthiery

  • reviewer set to Robert Bradshaw
  • milestone changed from sage-4.1.3 to sage-4.1.2
  • author changed from Nicolas M. Thiéry to Craig Citro, Nicolas M. Thiéry

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 :-))

  Changed 5 months 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.

  Changed 5 months ago by mhansen

  • status changed from positive_review to closed
  • resolution set to fixed
  • merged set to sage-4.2.alpha0
Note: See TracTickets for help on using tickets.