Cached methods with do_pickle=True fail for UniqueRepresentation objects

[klee]

The decorator @cached_method(do_pickle=True) is used to make a method to cache the result and the cached result pickled along with the object.

This fails for objects of UniqueRepresentation class. It is a consequence of the defective behavior of UniqueRepresentation object not putting its state into the pickle.

This ticket solves the problem. A discussion on this issue was in

​https://groups.google.com/forum/#!topic/sage-devel/n7v71cnVTmk

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​f17e57a

Make UniqueRepresentation do_pickle

[nbruin]

We can definitely not turn pickling on by default. See #15692.

[klee]

This is not about turn pickling on by default. This turns on pickling only for @cached_methods with do_pickle=True.

[klee]

This ticket makes UniqueRepresentation objects do what other sage objects do, for pickling cached methods.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​09fc937

Ignore cached functions with do_pickle=False

[klee]

To a reviewer:

Before this ticket,

• the cached function attached to cached method with do_pickle=False (the default) is not pickled

• the cached function attached to cached method with do_pickle=True is not pickled

With this ticket and before the last commit,

• the cached function attached to cached method with do_pickle=False (the default) is pickled without the cache

• the cached function attached to cached method with do_pickle=True is pickled with the cache

After the last commit,

• the cached function attached to cached method with do_pickle=False (the default) is not pickled

• the cached function attached to cached method with do_pickle=True is pickled with the cache

This last pickling behaviour of UniqueRepresentation objects is somewhat halfway between the old pickling behavior of UniqueRepresentation objects and that of other sage objects (second behavior).

The last version seems preferable because it does not increase the size of a pickle to keep unnecessary information, namely cached function without the cache.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​c979635

Fix a format mistake

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​983577b	Merge branch 'develop'
​9d6e0c7	Rename to is_pickled_with_cache

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​6d47b68

A little more documentation fixes

[SimonKing]

After a brief look at the diff, it seems to me that there are many changes that have nothing to do with the purpose of this ticket: You remove from __future__ import absolute_import and you have various changes of the following form:

@@ -167,9 +167,9 @@ class Manifolds(Category_over_base_ring):
 
             TESTS::
 
-                sage: TestSuite(Manifolds(RR).Smooth()).run()
                 sage: Manifolds(RR).Smooth.__module__
                 'sage.categories.manifolds'
+                sage: TestSuite(Manifolds(RR).Smooth()).run()
             """
             return self._with_axiom('Smooth')

Why?

[klee]

Replying to SimonKing:

After a brief look at the diff, it seems to me that there are many changes that have nothing to do with the purpose of this ticket: You remove from __future__ import absolute_import

Right. This is nothing to do with the purpose of the ticket. You know that this import statement is not necessary any more since python 2.7. It happens that as I am writing a patch for a ticket, I encounter things to be easily fixed, like obvious typos, docstrings in bad style, unnecessary imports (like the above) and so on. Sometimes I fix them. Other times I pass them.

Is this bad? Do you think I should revert these small changes not related with the purpose of the ticket before I push the commits?

I think if developers do not make such spontaneous fixes, improvement of the overall quality of sage code will get slower.

and you have various changes of the following form:

@@ -167,9 +167,9 @@ class Manifolds(Category_over_base_ring):
 
             TESTS::
 
-                sage: TestSuite(Manifolds(RR).Smooth()).run()
                 sage: Manifolds(RR).Smooth.__module__
                 'sage.categories.manifolds'
+                sage: TestSuite(Manifolds(RR).Smooth()).run()
             """
             return self._with_axiom('Smooth')

Why?

This (and many others like this) was related with the ticket. Without this reordering, I got doctest failures. But strangely but happily, now with 8.9.beta5, I don't have them any more even without reordering. I will revert back these changes.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​c14d10b	Merge branch 'develop'
​bd0bebb	Merge branch 'develop'
​bcd500e	Revert back unnecessary changes in doctests

[gh-mwageringel]

Replying to klee:

Replying to SimonKing:

After a brief look at the diff, it seems to me that there are many changes that have nothing to do with the purpose of this ticket: You remove from __future__ import absolute_import

Right. This is nothing to do with the purpose of the ticket. You know that this import statement is not necessary any more since python 2.7.

I might be wrong, but I think this is still needed in Python 2.7, as I have recently had to add the statement in one of my own projects to solve a problem that did not exist with Python 3.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​084a2ab

Add back absolute_import

[klee]

Replying to gh-mwageringel:

Replying to klee:

Replying to SimonKing:

After a brief look at the diff, it seems to me that there are many changes that have nothing to do with the purpose of this ticket: You remove from __future__ import absolute_import

Right. This is nothing to do with the purpose of the ticket. You know that this import statement is not necessary any more since python 2.7.

I might be wrong, but I think this is still needed in Python 2.7, as I have recently had to add the statement in one of my own projects to solve a problem that did not exist with Python 3.

Ah, I am wrong. You are right. I had somehow wrong understanding on this matter. I added back the imports.

Thanks!

[klee]

After studying how cython objects pickling works, I am now more confident that the patch works well as promised.

[embray]

Before merged I would just want to see the commit history on this ticket cleaned up with a rebase: get rid of all the unnecessary merge commits, and things like "Add back absolute_import". I can help with that if need be.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​f84fef6

Make UniqueRepresentation do_pickle

[klee]

This ticket deserves more attention. For your info, the branch is python3-safe.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​874f6c6

Make UniqueRepresentation do_pickle

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​056dcb3	Merge branch 'develop' into picklable-trac28096
​5705d47	Merge branch 'develop' into picklable-trac28096
​d1c7efa	Merge branch 'develop'

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​0fd1acc

Make UniqueRepresentation do_pickle

[embray]

Ticket retargeted after milestone closed

[mkoeppe]

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​727b23f

Make do_pickle work for UniqueRepresentation objects

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​f0b387e

Fix typos

[klee]

I am not sure if this patch would incur any bad side effects. Experts on UniqueRepresentation are welcome to scrutinize.

[klee]

In particular, I do not understand Volker's comment in the discussion:

Not every circularly dependent collection of cython objects can be pickled. Cached values can make the difference between what can and cannot be pickled. So whether or not pickling works can't reliably be tested in that case.

[mkoeppe]

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

Setting a new milestone for this ticket based on a cursory review.

[mkoeppe]

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

[mkoeppe]

needs rebase

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​6f14c1c

Make do_pickle work for UniqueRepresentation objects