Nested classes

[Nicolas M. Thiery]

Dear c-level python specialists,

Nested classes in python are really handy, and work pretty well,
except for a bug in pickling:

Let's define a nested class:

sage: class Bla:
....: class Ble:
....: pass
....:

Let's fake Bla is defined in a module, not in the interpreter:

sage: import __main__
sage: __main__.Bla = Bla

Dumping an element of Bla works just fine:
sage: x = Bla()
sage: dumps(x)
"x\x9ck`\xd2H\x8e\x8f\xcfM\xcc\xcc\x8b\x8f\xe7r\xcaI\xe4*d\xcc/d\xaa-dN\xd2\x03\x00v\xe3\x08'"

But dumping an element of Bla.Ble breaks:

sage: x = Bla.Ble()
sage: dumps(x)
---------------------------------------------------------------------------
PicklingError Traceback (most recent call last)

/home/nthiery/.sage/temp/zephyr/23765/_home_nthiery__sage_init_sage_0.py in <module>()

/opt/sage/local/lib/python2.5/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.dumps (sage/structure/sage_object.c:5539)()

PicklingError: Can't pickle __main__.Ble: attribute lookup __main__.Ble failed
> /opt/sage-3.4.rc0/devel/sage-combinat/sage/sage_object.pyx(571)sage.structure.sage_object.dumps (sage/structure/sage_object.c:5539)()
---------------------------------------------------------------------------

The reason behind this is twofold:

First, the name of the class is improperly set:

sage: Bla.Ble
<class __main__.Ble at 0xb1e7c98c>
sage: Bla.Ble.__name__
'Ble'

(which btw probably breaks other tools around)

Well, let's work around this:

sage: Bla.Ble.__name__ = "Bla.Ble"

But then, a second problem: the code of cPickle (around line 1977)
tries to lookup the attribute "Bla.Ble" in __main__, instead of
successively looking up "Bla" in __main__, and then "Ble" in the result:

sage: dumps(Bla.Ble)
---------------------------------------------------------------------------
PicklingError Traceback (most recent call last)
/home/nthiery/.sage/temp/zephyr/23765/_home_nthiery__sage_init_sage_0.py in <module>()
/opt/sage/local/lib/python2.5/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.dumps (sage/structure/sage_object.c:5539)()
PicklingError: Can't pickle __main__.Bla.Ble: attribute lookup __main__.Bla.Ble failed
> /opt/sage-3.4.rc0/devel/sage-combinat/sage/sage_object.pyx(571)sage.structure.sage_object.dumps (sage/structure/sage_object.c:5539)()

It would take me a couple hours to learn the syntax to write c-code
for python modules. Could any of the experts out there write me a
2-line patch upon cPickle:1977 to do the right thing (i.e. split
__name__, and make the attribute lookup for each bit)? Just a draft
will be fine; I am fine with reading and simple debuging.

Thanks in advance!

Best regards,
Nicolas

Here is the broken code:

mod = PyImport_ImportModule(module_str);
if (mod == NULL) {
cPickle_ErrFormat(PicklingError,
"Can't pickle %s: import of module %s "
"failed",
"OS", args, module);
goto finally;
}
############### HERE ####################
klass = PyObject_GetAttrString(mod, name_str);
if (klass == NULL) {
cPickle_ErrFormat(PicklingError,
"Can't pickle %s: attribute lookup %s.%s "
"failed",
"OSS", args, module, global_name);
goto finally;
}

--
Nicolas M. Thiéry "Isil" <nth...@users.sf.net>
http://Nicolas.Thiery.name/

[Carl Witty]

On Fri, Mar 13, 2009 at 1:03 AM, Nicolas M. Thiery
<Nicolas...@u-psud.fr> wrote:
> But then, a second problem: the code of cPickle (around line 1977)
> tries to lookup the attribute "Bla.Ble" in __main__, instead of
> successively looking up "Bla" in __main__, and then "Ble" in the result:

May I suggest a different workaround? (It's gross, but maybe less
gross than changing cPickle.)

sage: __main__.__dict__['Bla.Ble'] = Bla.Ble
sage: loads(dumps(x))
<__main__.Bla.Ble instance at 0x1a2e7eac>

Carl

[Nicolas M. Thiery]

Well, that's a bug in python that should eventually be fixed in
python. But yeah, your workaround could be a good way out.

Thanks!

Now, you don't have by any chance a magic spell, so that whenever such
a nested class Foo.Bar is created, 'Foo.Bar' gets inserted in its
module as above? (I guess I can hack around for my application).

Cheers,
Nicolas

[Mike Hansen]

On Fri, Mar 13, 2009 at 12:55 PM, Nicolas M. Thiery
<Nicolas...@u-psud.fr> wrote:
> Thanks!
>
> Now, you don't have by any chance a magic spell, so that whenever such
> a nested class Foo.Bar is created, 'Foo.Bar' gets inserted in its
> module as above? (I guess I can hack around for my application).

This is the purpose of metaclasses. The best introduction / primer on
metaclasses that I've read is at
http://cleverdevil.org/computing/78/

--Mike

[Carl Witty]

On Fri, Mar 13, 2009 at 12:55 PM, Nicolas M. Thiery

<Nicolas...@u-psud.fr> wrote:
>
> On Fri, Mar 13, 2009 at 11:37:34AM -0700, Carl Witty wrote:
>>
>> On Fri, Mar 13, 2009 at 1:03 AM, Nicolas M. Thiery
>> <Nicolas...@u-psud.fr> wrote:
>> > But then, a second problem: the code of cPickle (around line 1977)
>> > tries to lookup the attribute "Bla.Ble" in __main__, instead of
>> > successively looking up "Bla" in __main__, and then "Ble" in the result:
>>
>> May I suggest a different workaround?  (It's gross, but maybe less
>> gross than changing cPickle.)
>>
>> sage: __main__.__dict__['Bla.Ble'] = Bla.Ble
>> sage: loads(dumps(x))
>> <__main__.Bla.Ble instance at 0x1a2e7eac>
>
> Well, that's a bug in python that should eventually be fixed in
> python. But yeah, your workaround could be a good way out.

I certainly wouldn't call it a bug -- at the most, a missing feature.
I'm not even convinced that a patched version would be an improvement.
(For one thing, you would need to also patch the unpickle side, which
means that you can't write a pickle using your patch and read it in
older versions of Python. We don't care much about that in Sage, but
I think it's important to the Python people.)

Carl

[Nicolas M. Thiery]

Hi Mike!

> This is the purpose of metaclasses. The best introduction / primer on
> metaclasses that I've read is at
> http://cleverdevil.org/computing/78/

Thanks, I'll have a look.

I am already playing with meta classes for UniqueRepresentation and
dynamic classes, and the two of them readily do not collaborate
without special attention. So as of now, I find the concept extremely
fun, but I try to keep its potential uses to a strict minimum :-)

[Nicolas M. Thiery]

Dear Carl, dear all,

This is a call for comments and reviews for the new ticket #5986 whose
description is:

------------------------------------------------------------------------------
With the python code below::

class A:
class B:
pass

Python 2.6 erroneously set 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 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 (explain-pickle) and #5985
------------------------------------------------------------------------------

Thanks in advance!

Best,
Nicolas

On Fri, Mar 13, 2009 at 01:26:11PM -0700, Carl Witty wrote:
...

> > Well, that's a bug in python that should eventually be fixed in
> > python. But yeah, your workaround could be a good way out.
>
> I certainly wouldn't call it a bug -- at the most, a missing feature.
> I'm not even convinced that a patched version would be an improvement.
> (For one thing, you would need to also patch the unpickle side, which
> means that you can't write a pickle using your patch and read it in
> older versions of Python. We don't care much about that in Sage, but
> I think it's important to the Python people.)

Well, you can't pickle/unpickle it in the first place, so this can't
hurt more than currently.

[William Stein]

On Tue, May 5, 2009 at 12:30 AM, Nicolas M. Thiery
<Nicolas...@u-psud.fr> wrote:
>
>        Dear Carl, dear all,
>
> This is a call for comments and reviews for the new ticket #5986 whose
> description is:
>
> ------------------------------------------------------------------------------
> With the python code below::
>
> class A:
>    class B:
>        pass
>
> Python 2.6 erroneously set 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 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.

Have you posted this to the python-dev mailing list? This patch will
definitely not go into Sage unless it will definitely go into Python
at some point in the future. What if you are simply wrong in your
assertion that Python "erroneously set B.__name__ to "B" instead of
"A.B"?" Then you'll just be making it so Sage can never work with
any unpatched version of Python. Note that (1) in the entire history
of the Sage project, we have never once patched the Python
interpreter, except to fix small bus we found whose fixes were
upstreamed (the only thing I can think of along those lines is a
readline bug on Itanium), and (2) if we patch Python, then Sage for
Debian/Ubuntu will never be upgraded to the version of Sage with this
patch until the patch is upstreamed into Python.

--
William Stein
Associate Professor of Mathematics
University of Washington
http://wstein.org

[Nicolas M. Thiery]

On Tue, May 05, 2009 at 06:48:06AM -0700, William Stein wrote:
>
> On Tue, May 5, 2009 at 12:30 AM, Nicolas M. Thiery
> <Nicolas...@u-psud.fr> wrote:
> >
> >        Dear Carl, dear all,
> >
> > This is a call for comments and reviews for the new ticket #5986 whose
> > description is:
> >
> > ------------------------------------------------------------------------------
> > With the python code below::
> >
> > class A:
> >    class B:
> >        pass
> >
> > Python 2.6 erroneously set 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 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.
>
> Have you posted this to the python-dev mailing list?

Not yet but this of course definitely is the plan.

(sorry, I already have trouble keeping up with my current average
patch submission of one per day).

> This patch will definitely not go into Sage unless it will
> definitely go into Python at some point in the future.

What a nice way to start a polite negotiation between gentlemens.

(european vs american culture strikes back :-) )

> What if you are simply wrong in your assertion that Python
> "erroneously set B.__name__ to "B" instead of "A.B"?" Then you'll
> just be making it so Sage can never work with any unpatched version
> of Python. Note that (1) in the entire history of the Sage project,
> we have never once patched the Python interpreter, except to fix
> small bus we found whose fixes were upstreamed (the only thing I can
> think of along those lines is a readline bug on Itanium), and (2) if
> we patch Python, then Sage for Debian/Ubuntu will never be upgraded
> to the version of Sage with this patch until the patch is upstreamed
> into Python.

(1) Yes, maybe my assumption is wrong, though I strongly doubt it. In
any cases, what is for sure is that with the current version of
Python, nested classes cannot be pickled. And this is undoubtly a bug
(or missing feature if you prefer). The single purpose of my patch is
to fix this. Any other fix or workaround will do as well, and is more
than welcome.

(2) My patch does not touch the Python interpreter, but a copy of
cPickle within the Sage source tree. This does not prevent upgrading
python, as long as we keep our copy of cPickle reasonably updated. Of
course this is only acceptable as a temporary workaround.

(3) -1 on holding back further a large scale merge for a minor
technical detail that will eventually be resolved one way or the
other.

(4) My general feeling is that nested classes in Python are on the
edge of full maturity, with just a couple glitches here and
there. Typically, Florent mentionned to me that the devel version of
Sphynx now handled them correctly. This is why I feel confident using
them. And there is a clear advantage to using them (which I'll argue
about in my demo in Seattle).

Best,
Nicolas

[William Stein]

On Tue, May 5, 2009 at 9:34 AM, Nicolas M. Thiery
<Nicolas...@u-psud.fr> wrote:
> (2) My patch does not touch the Python interpreter, but a copy of
> cPickle within the Sage source tree. This does not prevent upgrading
> python, as long as we keep our copy of cPickle reasonably updated. Of
> course this is only acceptable as a temporary workaround.

Clever. That's actually a very good idea, which mostly negates my
concerns. Nonetheless, I would really like to hear from python-dev
about what *they* thinking about pickling and nested classes. What if
there are some good reasons for the current design that we're just
not thinking of?

[Nicolas M. Thiery]

> Clever. That's actually a very good idea, which mostly negates my
> concerns.

:-)

Credits shared with Carl and Mike!

> Nonetheless, I would really like to hear from python-dev about what
> *they* thinking about pickling and nested classes. What if there
> are some good reasons for the current design that we're just not
> thinking of?

Definitely!