Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround mishandled nested classes in Python and cPickle #5986

Closed
nthiery opened this issue May 5, 2009 · 13 comments
Closed

Workaround mishandled nested classes in Python and cPickle #5986

nthiery opened this issue May 5, 2009 · 13 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented May 5, 2009

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

CC: @sagetrac-sage-combinat @sagetrac-cwitty

Component: misc

Keywords: nested classes

Author: Craig Citro, Nicolas M. Thiéry

Reviewer: Robert Bradshaw

Merged: sage-4.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/5986

@nthiery nthiery added this to the sage-4.2 milestone May 5, 2009
@nthiery nthiery self-assigned this May 5, 2009
@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented May 6, 2009

comment:2

Attachment: cPickle-5986-nested-classes-submitted.patch.gz

@nthiery

This comment has been minimized.

@robertwb
Copy link
Contributor

comment:4

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

@jasongrout
Copy link
Member

comment:5

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?

@nthiery
Copy link
Contributor Author

nthiery commented Sep 22, 2009

comment:6

Replying to @jasongrout:

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.

@nthiery
Copy link
Contributor Author

nthiery commented Sep 22, 2009

Author: Nicolas M. Thiéry

@nthiery
Copy link
Contributor Author

nthiery commented Oct 11, 2009

Attachment: trac_5986-use-nested_class_metaclass.patch.gz

Apply only this one

@nthiery
Copy link
Contributor Author

nthiery commented Oct 11, 2009

comment:7

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

@nthiery
Copy link
Contributor Author

nthiery commented Oct 11, 2009

Changed author from Nicolas M. Thiéry to Craig Citro, Nicolas M. Thiéry

@nthiery
Copy link
Contributor Author

nthiery commented Oct 11, 2009

Reviewer: Robert Bradshaw

@nthiery nthiery modified the milestones: sage-4.2, sage-4.1.2 Oct 11, 2009
@robertwb
Copy link
Contributor

comment:8

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

@mwhansen
Copy link
Contributor

Merged: sage-4.2.alpha0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants