Removing pickles from the pickle jar?

[Jeroen Demeyer]

I personally don't know much about the pickle jar in Sage, apart that
it's the tarball pickle_jar/pickle_jar.tar.bz2 in extcode(*).

I thought it was meant to test unpickling of old pickles, which would
mean that one normally should not remove pickles from the jar. But
#9265 does exactly that: it removes some old pickles because they failed
to unpickle in the new version. Since I don't know much about the
pickle jar, I don't have an opinion on this. But if there is objection
on #9265, it will need to be fixed.


(*) recall that extcode is devel/ext since sage-5.4.beta2, it used to be
data/extcode.

[Simon King]

Hi Jeroen,

On 2012-10-15, Jeroen Demeyer <jdem...@cage.ugent.be> wrote:
> But
> #9265 does exactly that: it removes some old pickles because they failed
> to unpickle in the new version. Since I don't know much about the
> pickle jar, I don't have an opinion on this. But if there is objection
> on #9265, it will need to be fixed.

I believe that backwards compatibility is important. One might *perhaps*
accept that pickles created with Sage-2.x would become partially uneatable
with Sage-5.4. But the comments on #9265 seem to say that some pickles
created with Sage-5.3 would become uneatable with Sage-5.4 (or whatever
version #9265 will be merged). I don't think that would be a good idea.

How do the failing pickles look like? Are they pickled by "name of the
class plus content of __dict__" (AFAIK that's the default in Python)? Or
by "name of the class plus __init__ arguments plus data for
__setstate__"? Or by some separate unpickling function plus arguments,
returned by some "__reduce__" method?

In all cases, there should be ways to keep old data legible, resp. to
automatically transform an old data format into a new one.

Best regards,
Simon

[Jeroen Demeyer]

Some comments from the relevant tickets:

from Volker Braun:
> You are not supposed to update the pickle jar, it is only here to ensure backward compatibility. If at all possible, you should be using the register_unpickle_override to work around the change and unpickle into the new class.

from Nicolas M. Thi�ry:
> Just my 2 cents: at this point, partition tuples are a rather peripheral feature. If anyone has saved some pickle containing one, most likely he is in the Sage-Combinat group. Well most likely it's Andrew actually. So I vote for not wasting Andrew's time and simply dropping backward compatibility in that particular situation. I am not taking much risk by volunteering to help whoever might have trouble with such an old pickle :-)

[Andrew Mathas]

I was the person who updated the pickle jar in trac 9265. Like Jeroen, "I personally don't know much about the pickle jar in Sage", but I was given some instructions about what to do which came down to set SAGE_PICKLE_JAR, untar the pickle jar and copy the new pickles in, and tar the jar up again.I certainly was not aware that old pickles (which are essentially being deprecated) should not be removed.

It would help if there was some good (or even bad) documentation available which described how to update the pickle jar. As far as I am aware this is not covered in the developers guide, or anywhere else. I did search for documentation to complement the notes that I was given but I only found an oblique reference to occasionally needing to update the pickle jar in
sage.structure.sage_object.unpickle_all and http://trac.sagemath.org/sage_trac/ticket/10768 which opens a ticket to improve the current mechanism.

If it is decided that the pickles that I removed need to be reinstated I'll do this of course. I suggest that the best way to do this is with an additional ticket/patch on top of #9265.

Andrew

[Simon King]

Hi Andrew,

On 2012-10-15, Andrew Mathas <andrew...@gmail.com> wrote:
> It would help if there was some good (or even bad) documentation available
> which described how to update the pickle jar.
> As far as I am aware this is
> not covered in the developers guide, or anywhere else.

If I understand correctly, we are talking here about "modifying" in the
sense of "removing stuff". IMHO, it would be totally
against the purpose of the pickle jar, if we would encourage (or just
explain how) to remove stuff from the pickle jar.

If the pickle jar doc tests break then I think it is *not* the pickle
jar that needs to be updated. Instead, it is the unpickling methods that
need to be fixed.

> documentation to complement the notes that I was given but I only found an
> oblique reference to occasionally needing to update the pickle jar in
> sage.structure.sage_object.unpickle_all

Occasionally update the pickle jar? OK, but "update" in what sense? Of
course, one occasionally needs to add *new* pickles to the pickle jar,
namely when a new frequently used and complicated class was added, or
when the pickling of an existing class has changed (then, one needs to
demonstrate that both the old and the new format can be read).

> If it is decided that the pickles that I removed need to be reinstated I'll
> do this of course.

Here is my vote: You may add stuff to the pickle jar. But please do not
remove stuff from the pickle jar.

Best regards,
Simon

[David Roe]

Here is my vote: You may add stuff to the pickle jar. But please do not
remove stuff from the pickle jar.

+1

[Andrew Mathas]

Hi Simon,

If I understand correctly, we are talking here about "modifying" in the
sense of "removing stuff". IMHO, it would be totally
against the purpose of the pickle jar, if we would encourage (or just
explain how) to remove stuff from the pickle jar.

If the pickle jar doc tests break then I think it is *not* the pickle
jar that needs to be updated. Instead, it is the unpickling methods that
need to be fixed.

Actually, I think that you and I are talking about two different things: I am happy to maintain the pickle_jar whenever possible in the way the sage community wishes but I would really like this to be documented properly by the people who how know how it is supposed to work.

I removed these files from the pickle jar not because I wantonly wanted to destroy pickles but because I thought that this was the correct way to fix the problem - it does, after all, fix the doc tests. If this procedure was properly documented in the development guide, for example then we would not be having this conversation.

Having said that I am happy to maintain old pickles, if it's possible, I should add that with my limited knowledge of pickles it does seem to me when a class is refactored (as here) then it will not always be possible to unpickle the old pickle because the new and old data structures may not be compatible. Perhaps I am being naive, however, as I am yet to figure out how register_unpickle_override works.

Cheers,
Andrew



 

[Jean-Pierre Flori]

On Tuesday, October 16, 2012 8:47:03 AM UTC+2, Andrew Mathas wrote:

Hi Simon,

If I understand correctly, we are talking here about "modifying" in the
sense of "removing stuff". IMHO, it would be totally
against the purpose of the pickle jar, if we would encourage (or just
explain how) to remove stuff from the pickle jar.

If the pickle jar doc tests break then I think it is *not* the pickle
jar that needs to be updated. Instead, it is the unpickling methods that
need to be fixed.

Actually, I think that you and I are talking about two different things: I am happy to maintain the pickle_jar whenever possible in the way the sage community wishes but I would really like this to be documented properly by the people who how know how it is supposed to work.

I think the doc here explicitly says that the default pickle jar should be updated from time to time:
http://www.sagemath.org/doc/reference/sage/structure/sage_object.html#sage.structure.sage_object.picklejar

I also agree it may not be a sensible choice to remove old pickles (the point is to be sure they can still be loaded! so removing them to pass doctests is a bad fix in my opinion), nor to keep the ones from sage 0.4 whose class got deprecated in sage 1.1.

[Andrew Mathas]

I also agree it may not be a sensible choice to remove old pickles (the point is to be sure they can still be loaded! so removing them to pass doctests is a bad fix in my opinion), nor to keep the ones from sage 0.4 whose class got deprecated in sage 1.1.

 
Just for the record the pickles removed were for classes being deprecated.
A.

[Nicolas M. Thiery]

My point of view:

- The pickle jar is here to test that Sage supports
loading back old pickles for backward compatibility.

- The general rule is that Sage should strive for backward
compatibility.

- There can however be a few circumstances where keeping backward
compatibility is far more work than it's worth for the community. In
such cases, it can be decided by consensus (e.g. through a vote) to
break it.

- As a special case, the general rule is to not remove stuff from the
pickle jar, except on special circumstances where it's not worth the
trouble (we have already done that on some rare occasions).

- For the case at hand, I am fine breaking backward compatibility for
the reasons I mentioned on the ticket.

Note that updating a pickle in the pickle jar is not much better than
removing a pickle; in both case we don't test any more backward
compatibility for an old pickle. For doing this cleanly, we should
have versioned pickles, as proposed on:

http://trac.sagemath.org/sage_trac/ticket/10768

By the way, this ticket goes in the same general direction of having a
single mercurial/git repository for all of Sage.

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

[Volker Braun]

I'm confused here, the workflow should be deprecate -> wait one year -> remove deprecated functionality. But you are saying you want to remove the pickles at the beginning of the deprecation period? That sounds wrong.

[Andrew Mathas]

On Tuesday, 16 October 2012 22:00:13 UTC+11, Volker Braun wrote:

I'm confused here, the workflow should be deprecate -> wait one year -> remove deprecated functionality. But you are saying you want to remove the pickles at the beginning of the deprecation period? That sounds wrong.

Hi Volker,

I agree that this "wrong", however, I also think that it is unavoidable in some cases. I would be very happy to be corrected on this.

I have just uploaded a new patch trac_9265--tableaux_categories_pickles-am.patch to trac which adds a bunch of unpickle overrides to fix all but four of the unpickling errors (see the ticket). The ones that remain are all due, I think, to not being able to unpickle the class Tableau_class which is being deprecated by this patch.

I have tried to fix the unpickling of Tableau_class using

register_unpickle_override('sage.combinat.tableau', 'Tableau_class', Tableau)

but this does not work. My guess is that it is not possible to unpickle the deprecated Tableau_class objects using the new Tableau class objects because the underlying classes are too different.

If some one can see how to do this please let me know.

Andrew

[Nils Bruin]

On Oct 16, 7:29 pm, Andrew Mathas <andrew.mat...@gmail.com> wrote:

> register_unpickle_override('sage.combinat.tableau', 'Tableau_class',
> Tableau)
>
> but this does not work. My guess is that it is not possible to unpickle the

> deprecated *Tableau_class* objects using the new *Tableau* class objects

> because the underlying classes are too different.
>
> If some one can see how to do this please let me know.

The mechanism of unpickling is probably that the arguments for the
call to Tableau_class required to reconstruct the desired instance are
first "unpickled" (by whatever means appropriate for their types and
then a call is made to Tableau_class with those arguments. I assume
that the unpickle_override just ensures that now Tableau gets called,
but with arguments identical to what Tableau used to get! If the
arguments required between Tableau_class and Tableau are not
compatible then this approach doesn't work.

What you probably need is

register_unpickle_override('sage.combinat.tableau', 'Tableau_class',

construct_Tableau_from_Tableau_class_data)

where construct_Tableau_from_Tableau_class_data is a function that
takes as arguments the same arguments that Tableau_class would take
and returns an instance of Tableau that is as close as possible to
whatever Tableau_class would otherwise have been produced.

This is how you provide an "upgrade path". I think there are more
complicated pickle/unpickle mechanisms than this (__getstate__ and
__setstate__ stuff), but I don't see how a
"register_unpickle_override" would be able to get in between there.

I guess to start with you could put a function in there that prints
all its arguments to get some idea what you have to interface with.

[Nils Bruin]

OK, sorry. The hook is elsewhere:

You should provide a __setstate__ method on Tableau (or if you don't
like that, a proxy class that you provide with
register_unpickle_override). Now you have to write __setstate__ in
such a way that it can distinguish between input data for either
Tableau_class or for Tableau and take appropriate action in either
case.

The default unpickling seems to be:
- make a new class instance (but don't run __init__)
- set the instance attribute dictionary to a value obtained from the
pickle.

so if you changed the layout of the attributes that won't work.

When you supply a __setstate__ method you get to decide yourself what
goes into the dictionary (or whatever other actions you want to take).
It should probably look something like:

def __setstate__(self,a):
if <a is a dictionary for Tableau_class>:
self.new_attribute=a['old_attribute']
... and other conversions ...
else:
self.__dict__.update(a)

[Andrew Mathas]

Excellent. Thank you Nils. That solves my problem. I'd worked out that the problem is caused by the fact Tableau_class is a subclass of CombinatorialObject whereas Tableau also inherits from Element. It is the Element.__setstate__ that is causing the problem. I should be able to sort it out now.

Thanks again,
Andrew

[Andrew Mathas]

One of the things that I like about sage is python, but what I like least about it is documentation, which is often missing or inadequate.

Because there is no documentation about the pickle_jar, and how it should be used, I wasted 4+ hours on this. First I was told to replace the broken pickles, which I did. When it was explained to me that this wasn't how the pickle_jar is supposed to work I found the documentation on register_unpickle_override to be about as useful as trying to explain matrix multiplication using 1x1 matrices. A non-trivial example would help.

Sage needs documentation explaining what the pickle_jar is for, and what should be done when a pickle in the jar is broken. Having documentation would have saved me 3+ hours, some of Jeroen's time, plus all of the time people have spent on this thread. Old hands in sage know what the pickle_jar is for and how to use it but I don't see how newer people can learn this if it is not written down somewhere.

To end on a positive note, thanks again to Nils who gave me the hint that I needed to fix the problems with #9265 and #13072.

Andrew

[Nils Bruin]

On Oct 17, 3:42 pm, Andrew Mathas <andrew.mat...@gmail.com> wrote:
> One of the things that I like about sage is python, but what I like least
> about it is documentation, which is often missing or inadequate.

And which should then be improved, preferably by the person who has
just identified in what way the documentation is deficient!
Pickle is a python thing, so for a large part you should look in the
python documentation:

http://docs.python.org/library/pickle.html#what-can-be-pickled-and-unpickled

In particular, one of the hints there would have set you on the right
track:

>Similarly, when class instances are pickled, their class’s code and data are not pickled along with them. Only the instance data are pickled. This is done on purpose, so you can fix bugs in a class or add methods to the class and still load objects that were created with an earlier version of the class. If you plan to have long-lived objects that will see many versions of a class, it may be worthwhile to put a version number in the objects so that suitable conversions can be made by the class’s __setstate__() method.

On the other hand, I didn't know about register_unpickle_override. I
think that's a sage thing, so that might be a place to put better
pointers. Perhaps a less trivial example?

from sage.structure.sage_object import register_unpickle_override
class A(object):
def __init__(self,value):
self.original_attribute = value
def __repr__(self):
return "B(%s) instance"%self.new_attribute
class B(object):
def __init__(self,value):
self.new_attribute = value
def __setstate__(self,args):
try:
self.new_attribute = args['new_attribute']
except KeyError:
#must be an old pickle
self.new_attribute = args['original_attribute']
def __repr__(self):
return "B(%s) instance"%self.new_attribute

sage: a = A(10)
sage: register_unpickle_override('__main__', 'A', B)
sage: b = B(20)
sage: an = loads(dumps(a))
sage: print an
B(10) instance
sage: bn = loads(dumps(b))
B(20) instance
sage: print bn

Someone should feel free to work this into appropriate doc. I thought
this was pretty discoverable already :-).

By the way, Andrew, 4 hours for learning how python's pickle protocol
works isn't so bad :-).

Cheers,

Nils

[Andrew Mathas]

I guess my point is that even though pickles are a python thing, maintaining the pickle_jar itself is a sage thing and it should be documented what it does and how it is supposed to work (I'll take on board your anticipated hint about improving documentation:).

It's a little ironic, but looking back over the trac ticket for #9265 the comments show that two months ago I resolved most of the pickle problems "correctly" using register_unpickle_override but as I couldn't fix the Tableau_class pickles I decided to replace all of the offending pickles. If there was documentation which said "thou shall not destroy pickles in the pickle_jar" this would not have happened.

A.

[Florent Hivert]

Hi Andrew,

I created the following ticket when I was learning Sage four years ago.

Ticket #5294 (new defect)
On sage-combinat-devel Michael wrote:

"The pickle jar is not in the documentation AFAIK and it definitely should
be. So someone who thinks this is a good idea please open a ticket."

I definitely think this is a good idea.

I think you should close it as duplicate or reuse it.

Cheers,

Florent

[Travis Scrimshaw]

Hey,

   On that note, I think there should also be a link to the pickle jar doc in sage_object.py which is where the pickle jar test actually fails (and perhaps the error message suggesting it might be the pickle jar).

Best,

Travis

[Andrew Mathas]

I have just uploaded a patch to http://trac.sagemath.org/sage_trac/ticket/5294 which adds documentation about the pickle_jar to the reference manual. (Thanks Florent for creating the ticket, year ago). Possibly something should also go in the developers guide.

The main aim of the patch is to tell people what to do when their patch breaks an old pickle in the pickle_jar. The patch gives more information about to do (and not to do) when a pickle fails to unpickle.

I suspect that most people first become aware of the pickle_jar when a pickle fail to unpickle in a doctest. The patch causes a hopefully more informative error message like the following to be printed:
{{{

Failed:
_class__sage_combinat_crystals_affine_AffineCrystalFromClassicalAndPromotion_with_category_element_class__.sobj
_class__sage_combinat_crystals_tensor_product_CrystalOfTableaux_with_category_element_class__.sobj
_class__sage_combinat_crystals_tensor_product_TensorProductOfCrystalsWithGenerators_with_category__.sobj
_class__sage_combinat_tableau_Tableau_class__.sobj
--------------------------------------------------
** This error is probably due to an old pickle failing to unpickle.
** See sage.structure.sage_object.register_unpickle_override for
** how to override the default unpickling methods for (old) pickles.
** NOTE: pickles should never be removed from the pickle_jar!
--------------------------------------------------
Successfully unpickled 583 objects.
Failed to unpickle 4 objects.
}}}
It also adds more examples to the `register_unpickle_override` documentation about how to fix broken pickles.

In order to put the error message above into the manual, the patch contains an example which intentionally breaks a pickle (but only in the example). Unfortunately, as a side effect the patch fails a doctest. This seems to have nothing to do with the patch but, rather, is a consequence of the following which looks like a bug to me:

{{{
sage: from sage.structure.sage_object import unpickle_all
sage: %timeit unpickle_all()
/usr/local/src/sage/sage-5.3/local/lib/python/timeit.py:195: DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double.
See http://trac.sagemath.org/4260 for details.
  timing = self.inner(it, self.timer)

------------------------------------------------------------------------
Unhandled SIGABRT: An abort() occurred in Sage.
This probably occurred because a *compiled* component of Sage has a bug
in it and is not properly wrapped with sig_on(), sig_off(). You might
want to run Sage under gdb with 'sage -gdb' to debug this.
Sage will now terminate.
------------------------------------------------------------------------
/usr/local/src/sage/sage-5.3/spkg/bin/sage: line 335: 73974 Abort trap: 6           sage-ipython "$@" -i
}}}

Can anyone confirm whether this is a bug or known issue? Either way, is there a way around this?

Any comments on the patch, especially from those above who feel strongly about the pickle_jar, are most welcome.

Andrew

[Andrew Mathas]

I have finalised my pickle jar documentation patch http://trac.sagemath.org/sage_trac/ticket/5294 on trac and changed its' status to needs review.

Can those people who were worried that explaining in the documentation how the pickle_jar works please check to see whether they are happy with the explanations that I have added. If some one would like to review it that would be good too:)

Andrew