Bug in pickling of toric varieties, II

[a1031e5c-5e2c-4c90-8ba4-91be67e304df]

This is a follow-up to the ticket #15050. Apart from the methods changed in #15050, there are five more methods of ToricVariety_field, namely

Todd_class, Chern_class, Chern_character, Kaehler_cone, Mori_cone

which use private variables for caching and procedure errors when pickled. This fix follows the same logic as #15050 and rewrites the caching using the decorator cached_method.

Depends on #15050

CC: @vbraun @novoselt

Component: algebraic geometry

Keywords: toric

Branch/Commit: u/jkeitel/toric_pickling_2 @ 15a4164

Reviewer: Volker Braun

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

[a1031e5c-5e2c-4c90-8ba4-91be67e304df]

Commit: 15a4164

[a1031e5c-5e2c-4c90-8ba4-91be67e304df]

Branch: u/jkeitel/toric_pickling_2/

[a1031e5c-5e2c-4c90-8ba4-91be67e304df]

Changed branch from u/jkeitel/toric_pickling_2/ to u/jkeitel/toric_pickling_2

[nbruin]

comment:3

So the better solution is: provide CPRFanoToricVariety_field_with_category with a reduce method that ensures that attributes such as _dimension_relative (probably all attributes required for computing a hash) get initialized in the construction phase and leave the rest of the dictionary to be initialized at the setstate phase.

As you can see from these example, this is required for proper pickling anyway because you would want

loads(dumps(variety.Todd_class()))

to work and it's not clear to me that clearing caches is going to resolve that issue.

Whether you want to strip the dictionary offerered by reduce for pickling of caches is a separate question.

[vbraun]

comment:4

In theory, I would agree that one needs to implement __reduce__ (basically, for all classes that inherit from UniqueRepresentation). But

1. It is really hard to doctest. Not only must you restore all the data for the hash, but if there is a hash collision then you also need all the data for __cmp__().

2. In practice, Python doesn't support circular __reduce__ so we'll end up just triggering that bug.

[nbruin]

comment:5

Replying to @vbraun:

In theory, I would agree that one needs to implement __reduce__ (basically, for all classes that inherit from UniqueRepresentation).

In fact, UniqueRepresentation should be particularly easy to pickle: the weak cache that gets used to see if objects already exist has the parameters used to instantiate them already cached as a key! If the objects in there have sane pickling themselves (and they should, because they're hashable, and hence fairly immutable, so their important properties haven't changed since they have been created, so any potentially circular stuff can go into setstate), a reasonable set of pickling parameters is already there! It may be worthwhile to see if UniqueRepresentation could offer a default reduce that gets a little towards that concept.

EDIT: And in fact, CachedRepresentation already implements this! See `sage/structure/unique_representation.py, line 560:

    def __classcall__(cls, *args, **options):
...
        if instance.__class__.__reduce__ == CachedRepresentation.__reduce__:
            instance._reduction = (cls, args, options)
...
    def __reduce__(self):
        return (unreduce, self._reduction)

So it's already doing that. It also means that the default pickling is throwing away the dictionary, which may be a bit rough.

By the way, toric varieties don't inherit from UniqueRepresentation. Their cohomology rings do, though, so you're probably running into this issue because toric varieties get hashed there (and in similar constructions).

1. It is really hard to doctest. Not only must you restore all the data for the hash, but if there is a hash collision then you also need all the data for __cmp__().

If our design decisions are being affected by whether something is hard to doctest then we really have cart and horse in the wrong order. Let's see. ToricVariety_field has these creation parameters:

fan, coordinate_names, coordinate_indices, base_field

You got to be able to decide equality based on these. The last 3 should definitely not introduce circularities in their construction phases. Let's look at fan:

cones, rays, lattice

Again, those should be the only parameters involved in the construction phase of a fan. The rest can go into setstate. I don't know exactly what things you need to define cones, rays, lattice, but it seems like those should also be strictly non-circularly constructible (any caches you might want to pickle can go into setstate).

2. In practice, Python doesn't support circular __reduce__ so we'll end up just triggering that bug.

And every time you're getting that (and we'd hope we run into an error report, because the silent failures are really painful), it's an indication that you got your construction/setstate balance wrong.

Given that the very nature of Python (and any normal programming language), any circular reference is always inserted by modifying and existing object(*). That's what setstate in pickling is for.

(*) There's of course Simon's example

class circ(object):
    def __init__(self,i)"
        self.i=i
        self.d={self:1}

which indeed seems like circularity-upon-construction, but the key here is that no circular input data is needed to complete the construction.

[vbraun]

Reviewer: Volker Braun

[vbraun]

comment:6

For the record, with this ticket the loads(dumps(variety.Todd_class())) works and without this ticket it fails miserably. Its really the caching that messes us up, because it puts additional circular references into the reduction without any user control. Except for disabling it via ClearCacheOnPickle mixin class or by overriding __reduce__. I still think we should make pickling of caches opt-in and not opt-out, because it is very easy to trip over and hard to doctest different combinations of cached outputs. If you want to pickle some result then just pickle the result, not the object that happens to cache the result.

But, in any case, this is not material for this ticket. The solution looks good within the constraints of the current caching system, so positive review.

[nbruin]

comment:9

I still think the current patch removes the symptom, but does not solve the underlying reason: ToricVarieties don't pickle in a way that is robust in the face of circular references. This is because ToricVarieties aren't sufficiently initialized in the construction phase of their unpickling to make their hash work. I don't know what guarantees pickling/unpickling makes about what happens between objects being constructed and having setstate called on them, but the circular references show that it isn't guaranteed that nothing happens in between. The problem happens to be triggered by circular references, but I don't think we have proof that will even be the only case when it happens.

Anyway, circular references aren't forbidden and pickling in general has been designed to have the means to deal with it. The next time someone introduces circular references on toric varieties, we'll have the same problem again. I'm sure ToricVarieties aren't the only data structures that have this problem. Unless one pays specific attention to this problem (or inherits from UniqueRepresentation!) one is likely to have this problem.

I don't particular disagree with the fact that caches aren't pickled -- that may well be a good idea. However, I do think that ClearCacheOnPickle is a really bad way of achieving that effect: it actually wipes the caches, rather than remove them from the dictionary that is submitted for pickling. That means that doing dumps(toric_variety) changes the performance of subsequent behaviour! EDIT: ClearCacheOnPickle actually does something quite reasonable. If super().__getstate__ produces some exceedingly exotic structures to pickle, it could miss CachedFunction instances in it, or not reproduce the containers not entirely faithfully. So I withdraw my objection to using ClearCacheOnPickle.

Why not just put a custom __reduce__ or __getnewargs__ on toricvarieties and solve the problem permanently? If you don't want to pickle caches, you can easily have that as a corollary.

[nbruin]

comment:10

I think this is actually an important issue: pickling is really valuable, especially because of the ever increasing importance on parallel processing, which tends to require interprocess communication. I don't think what we need to do is complicated or a lot of work. It's just that people need to get some experience and examples in what to do.

I'd happily write a patch, on this ticket, but the git branch stuff is beyond me.

Probably something like the following on CPRFanoToricVariety_field already does the trick:

    def __getnewargs__(self):
        return (self._Delta_polar,self._fan,self._coordinate_points,self.__point_to_ray,
                self._names,<whatever you need to get coordinate indices>,self._base_ring)

and indeed, you're going to need one of those for pretty much every __init__ you write...

In a way, the problem comes from CategoryObject, where __hash__ is defined in terms of repr, so in a way that's where pickling is broken. So perhaps we should have a __getnewargs__(self) there, a la:

    def __getnewargs__(self):
        return self._initargs

in which case of course we also need something along the lines of

    def __init__(self,*args,**kwargs):
        self._initargs=args
        self._initkwargs=kwargs ##we may need a custom reduce here: how do we deal with kwargs otherise?

CategoryObject already has custom __getstate__ and __setstate__. It's just that its pickle process doesn't seem to have been written with in mind that __hash__ might be needed prior to __setstate__ executing. Incidentally, CategoryObject caches the hash value, so if we have the following reduce method on CategoryObject we might skirt at least the premature hash problem:

  def __reduce__(self):
    return <constructor>,(type(self.C),...,self.hash(),),self.__dict__

where <constructor> is some factory that reconstructs the object (essentially via an empty __new__) but then, in addition, sets self._hash.

[nbruin]

comment:11

I don't think this ticket itself needs work, but that we need more info on how to solve the problem properly. If we can tackle it on CategoryObject we may not need to do anything on ToricVariety.

[vbraun]

comment:12

The only thing in ToricVariety that refers back to itself are caches, if it weren't for that it would be perfectly safe to pickle it by its dict. I don't think its a good solution to have everybody write a custom pickling just to manually exclude caches.

Also, all the changes in the attached branch are desirable anyways, so we should include them. Any further discussion of general frameworks for pickling should go elsewhere (perhaps #15156).