On Tuesday, September 10, 2013 6:43:34 AM UTC-7, Simon King wrote: > > > I am puzzled: I believe that CachedRepresentation *IS* a new-style class, > "new-style class" being defined as a type inheriting from <object>): > OK, never mind. Indeed, it's a cython file, not a python file.
> > I don't think the default CachedRepresentation.__reduce__ should *just* > > look at __dict__, if anything it should use first try __getstate__() to > > give the subclass the opportunity to override which attributes to > pickle. > > Sounds like a good idea. > +1 > > Also, I don't know if Python handles circularities in the subsequent > > arguments of unreduce better than the first argument. Hopefully, it > does. > That's what it's for! The subsequent return values generated by __reduce__ are NOT passed to (in this case) unreduce. They are passed to __setstate__, and only after all the required construction calls have been made (because otherwise there is nothing to pass). > I don't think that the __reduce__ method of CachedRepresentation is to > blame for troubles with circularity. If anything, problems arise with > classes that are *not* using CachedRepresentation, simply because they > do *not* use __reduce__. > I concur with that assessment. The CachedRepresentation __reduce__ is a good example of how most reduces should look (except that any subsequent state is thrown away). > > Let's do an example: > > sage: from sage.structure.unique_representation import unreduce > sage: class C(object): > ....: def __init__(self, a): > ....: self.a = a > ....: self._reduce = (C, (a,), {}) > ....: def __reduce__(self): > ....: return (unreduce, self._reduce, self.__getstate__()) > ....: def __getstate__(self): > ....: print "calling getstate" > ....: return self.__dict__ > ....: def __setstate__(self, D): > ....: print "calling setstate" > ....: self.__dict__ = dict(D) > ....: def __repr__(self): > ....: return "<%s>"%self.a > sage: c = C(1) > sage: c.x = 5 > sage: d = loads(dumps(c)) > calling getstate > calling setstate > sage: d is c > False > sage: d.x > 5 > > So, yes, it would *seem* work---with one important problem: If you would > do the same with CachedRepresentation, then the same setstate/getstate > trickery would be played as well on a *cached* object! In particular, > when you do > loads(dumps(c)) > then it gives back c, but nevertheless __setstate__ would be executed > before returning c! > > This is, in the best case, a waste of time. > I don't quite agree with that. First of all, the getstate should read `self.__dict__.update(D)`. If we'd follow Python's example, we'd actually do try: self.__dict__.update(D) except <appropriate error>: for k,v in D: setattr(self,k,v) but that's only a backup strategy that may not come into play for us. The reason why it shouldn't be a big problem to pickle the dict is because any changes to that dict would have happened by calls to this CachedRepresentation originally, and since the code producing those calls should be aware that this is a globally unique object, they should have made sure already that whatever they did to __dict__is not a bad thing to do. Normally I'd expect the __dict__ to contain valuable information (cached info?) that allows the global object to perform its function (as defined by its creation parameters, of course) better. So it may not be a waste of time at all. It may be required to let the pickle in question perform with the same efficiency after unpacking as it did when it was before it was pickled, and not trigger possibly expensive recomputations. If other objects in the pickle would always trigger the creation of certain entries in the dict, we could even end up with an inconsistent state, because those other objects might be relying on that added state implicitly (perhaps that state is contains nondeterministic bits and even if recomputation is triggered you may end up with subtle differences) I agree that there may be lots of things in the dict that are not worth pickling, but CachedRepresentation is such a generic object that it's not in a position to tell. Given that Python's default unpickle strategy is: do the absolute minimum for object creation and reconstruct everything in setstate, I think we'll get the least surprising behaviour if we also stick with "setstate (almost) everything" by default. [As we found on #15156, cachedfunction objects and maps do presently have __reduce__ methods that tend to lead to bad circularity, but those are easily resolved by shifting more to setstate] -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at http://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/groups/opt_out.