#15692 closed defect (fixed)
Value of cached methods should not always be pickled
Reported by:  SimonKing  Owned by:  

Priority:  major  Milestone:  sage7.0 
Component:  pickling  Keywords:  pickling, hash, cache, days71 
Cc:  ncohen, vbraun, nbruin, novoselt, jkeitel, nthiery  Merged in:  
Authors:  Julian Rüth  Reviewers:  Vincent Delecroix, David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  a18cbf5 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  #16337  Stopgaps: 
Description
In #15278, the hash of graphs was turned into a @cached_method
. If a cached method is pickled, then the cached value is pickled as well. In the case of a hash value, this is not always what we want, since the hash value might depend on the memory address of an object, or worse: The hash function might change in future implementations.
That's to say: If you save object O now, together with hash value 123, and later load it, then hash(O) will still be 123, even if a newly constructed object P with O==P might have hash(P)==456, because of a change in the hash function.
This obviously is a problem. There is something called sage.misc.cachefunc.ClearCacheOnPickle
, but I don't know if this would really help in this case. Also, this would clear all cache values at once. Also, I don't know if it works for cached methods that do not take arguments (beside self
).
I suggest that we should instead have an optional parameter for sage.misc.cachefunc.CachedMethodCallerNoArgs
that determines whether or not the cached value is preserved, and use it on graphs (and future applications of cached method on __hash__
).
Change History (69)
comment:1 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:2 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:3 Changed 7 years ago by
 Cc vbraun nbruin novoselt jkeitel added
comment:4 Changed 7 years ago by
 Branch set to u/saraedum/ticket/15692
 Created changed from 01/18/14 09:30:18 to 01/18/14 09:30:18
 Modified changed from 05/12/14 10:05:02 to 05/12/14 10:05:02
comment:5 Changed 7 years ago by
 Dependencies set to #16337
comment:6 Changed 7 years ago by
 Modified changed from 05/12/14 17:43:50 to 05/12/14 17:43:50
 Status changed from new to needs_review
comment:7 Changed 7 years ago by
 Commit set to 07178496787dfa2e02b7bab8e32ca8e2971edaba
I'm in support of this although I am not the right person to review the changes.
New commits:
ce27b84  Removed sage.misc.cachefunc.ClearCacheOnPickle

e27e316  Propagate key of a @cached_method correctly

94fcddb  Merge branch 'u/saraedum/ticket/16337' of git://trac.sagemath.org/sage into ticket/15692

72fce8b  Added a pickle parameter for @cached_method

0717849  Enable pickling of the cache for groebner_basis()

comment:8 Changed 7 years ago by
 Commit changed from 07178496787dfa2e02b7bab8e32ca8e2971edaba to 9eab84b8c4f377576d067a9b7ca0eba43244f317
Branch pushed to git repo; I updated commit sha1. New commits:
9eab84b  Merge branch 'develop' into ticket/15692

comment:9 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:10 Changed 6 years ago by
 Status changed from needs_review to needs_work
does not apply, needs rebase
comment:11 Changed 6 years ago by
 Commit changed from 9eab84b8c4f377576d067a9b7ca0eba43244f317 to 13b4e4cd7dc1ebd4f524dbe9cb33fb2a8775c3d6
Branch pushed to git repo; I updated commit sha1. New commits:
13b4e4c  Merge branch 'develop' into t/15692/ticket/15692

comment:12 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:13 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.9
 Status changed from needs_review to needs_work
one failing doctest, see patchbot report
comment:14 Changed 6 years ago by
 Commit changed from 13b4e4cd7dc1ebd4f524dbe9cb33fb2a8775c3d6 to 616e86189f0d3a30c4885e38e01a619005446855
Branch pushed to git repo; I updated commit sha1. New commits:
616e861  Fix doctest in sage/misc/cachefunc.pyx

comment:15 Changed 6 years ago by
 Status changed from needs_work to needs_review
New commits:
616e861  Fix doctest in sage/misc/cachefunc.pyx

New commits:
616e861  Fix doctest in sage/misc/cachefunc.pyx

comment:16 followup: ↓ 19 Changed 6 years ago by
 Status changed from needs_review to needs_work
Hello,
In _cached_function_unpickle
instead of
for k,v in cache: ret.cache[k] = v
you can simply do ret.cache.update(cache)
. Moreover, in that function you should document the argument cache
.
I think that disabling the cache by default on pickle is a good improvement. However, the class ClearCacheOnPickle
that you remove in your branch had several purposes: clear the cache on pickle but also on copy. Your version keep the cache on copy and that might not be appropriate for classes that used ClearCacheOnPickle
.
Vincent
comment:17 Changed 6 years ago by
 Reviewers set to Vincent Delecroix
comment:18 Changed 6 years ago by
 Commit changed from 616e86189f0d3a30c4885e38e01a619005446855 to b2aa4073a6724e0c92c719f8d3eb232ef0bc2102
Branch pushed to git repo; I updated commit sha1. New commits:
b2aa407  Improve docstring of sage.misc.cachefunc._cached_function_unpickle

comment:19 in reply to: ↑ 16 ; followup: ↓ 21 Changed 6 years ago by
Replying to vdelecroix:
I think that disabling the cache by default on pickle is a good improvement. However, the class
ClearCacheOnPickle
that you remove in your branch had several purposes: clear the cache on pickle but also on copy. Your version keep the cache on copy and that might not be appropriate for classes that usedClearCacheOnPickle
.
Thanks for pointing this out. What should we do about this? Should caches ever be copied actually?
comment:20 Changed 6 years ago by
 Status changed from needs_work to needs_info
comment:21 in reply to: ↑ 19 ; followup: ↓ 22 Changed 6 years ago by
Replying to saraedum:
Replying to vdelecroix:
I think that disabling the cache by default on pickle is a good improvement. However, the class
ClearCacheOnPickle
that you remove in your branch had several purposes: clear the cache on pickle but also on copy. Your version keep the cache on copy and that might not be appropriate for classes that usedClearCacheOnPickle
.Thanks for pointing this out. What should we do about this? Should caches ever be copied actually?
I guess not. And if you really want to, it is always possible to fill the cache manually.
comment:22 in reply to: ↑ 21 ; followup: ↓ 23 Changed 6 years ago by
Do you have any idea how I could implement that? I could for example implement __getstate__
for sage parents and elements, but then whenever it is overwritten I would have to rely on the super implementation being called. I could also implement __copy__
there which is muss less likely to be overwritten but still the problem is the same.
The problem is that unlike with pickling, __getstate__
is not called on the cached methods when a shallow copy is made (only for a deep copy.)
One thing I could think of is putting the id
of the original object into each of its caches. Any call to the cache could check that id (which could be a small but significant performance loss) and complain if it does not match the element the cache is associated to.
I think that implementing __copy__
is probably the best path to take.
comment:23 in reply to: ↑ 22 Changed 6 years ago by
Replying to saraedum:
Do you have any idea how I could implement that? I could for example implement
__getstate__
for sage parents and elements, but then whenever it is overwritten I would have to rely on the super implementation being called.
(I'm pretty sure that __getstate__
should call its super unless one really knows what one is doing. A bigger problem is that __getstate__
is only a convention that the standard __reduce__
uses. I'm sure there are classes that implement their own __reduce__
far below the point where you'd be thinking of injecting your __getstate__
)
On #15207 there is some discussion of some possible infrastructure for managing these things. Perhaps it's relevant for your case?
I could also implement
__copy__
there which is muss less likely to be overwritten but still the problem is the same.
I would recommend leaving __copy__
out of it. That has nothing to do with the pickling/serialization protocol. It provides a "cheap" way of replicating a mutable structure (note that for immutable structures, __copy__
routinely returns self
, because immutability implies) so that mutations of one do not affect the other.
Mutable structures need to take care that they invalidate caches already (when mutated). I would say that a default behaviour where copy
creates a new object with the same caches would be appropriate. The default behaviour of "copy" is to copy the instance dict, which largely establishes this.
If this is not appropriate behaviour, it seems to me that the class has rather special needs and hence should provide its own __copy__
.
EDIT: I see that ClearCacheOnPickle
gets its copy
behaviour because under certain circumstances, copy(...)
will revert to calling __getstate__
. So I would expect that this would continue to be the case. Since the "clear cache on copy" is a bit of a sideeffect of the implementation of ClearCacheOnPickle
I would try and see if just changing the copy behaviour gives acceptable results. If not then providing a custom __copy__
on just the class that originally inherited from ClearCacheOnPickle
would do the trick. Going forward, I think having caches replicated (or probably even shared!) between shallow copies by default doesn't seem like an unreasonable default.
comment:24 Changed 6 years ago by
Ok. Thanks for the input. I will implement it like that. Explicitly implement __copy__
where it is necessary to keep the old behaviour and leave everything else like it is in my branch.
comment:25 Changed 6 years ago by
 Cc nthiery added
Trying to implement this, I realized that the point of the ClearCacheOnPickle
on these classes was certainly not to clear *all* caches on copy.
Also, ClearCacheOnPickle
is broken, since it replaces any cache dictionary with an actual dict
, i.e., it would replace a weak dictionary with a regular one.)
I guess there was in each case some cached method which should not be pickled (or probably copied.)
So it is probably better to look at the actual occurences and fix the copying there instead of just hacking a "drop some caches" __copy__
method which probably does too much and might even break if we change the cached methods next time around.
The classes which carry ClearCacheOnPickle
are:
ToricVariety_field
where it was added in0be2c23d
with the comment "Do not pickle caches of toric varieties". So it could be alright if we still copied them.WeilGroup_gens
where it was added when the class did not provide the functionality to clear caches on copy. So it might be alright to copy them.AmbientSpace
andRootSpace
where it is not clear why it was added exactly. Maybe Nicolas could clarify this (author of the commit which added it.)
Btw. the lengthy discussion in #11115 (this is where the clear caches on copy was introduced) does not make any mention of "copy".
comment:26 Changed 6 years ago by
 Status changed from needs_info to needs_review
Since there has been no reaction for quite a while, I'm setting this back to needs review. I think my current implementation does the right thing.
comment:27 Changed 6 years ago by
 Status changed from needs_review to needs_work
From the patchbot
sage t long src/sage/misc/cachefunc.pyx ********************************************************************** File "src/sage/misc/cachefunc.pyx", line 3185, in sage.misc.cachefunc.cached_method Failed example: id(d) == hash(d) Expected: True Got: False ********************************************************************** 1 item had failures: 1 of 30 in sage.misc.cachefunc.cached_method [830 tests, 1 failure, 15.75 s]  sage t long src/sage/misc/cachefunc.pyx # 1 doctest failed 
comment:28 Changed 6 years ago by
 Status changed from needs_work to needs_review
That test does not fail for me. It is reported to be fine by another patchbot. Maybe the patchbot that ran it is broken somehow? I can not really imagine that something "random" is happening here.
comment:29 Changed 6 years ago by
 Commit changed from b2aa4073a6724e0c92c719f8d3eb232ef0bc2102 to 67c7782df66179ebc0e7dd09b2518f76b6ecf198
Branch pushed to git repo; I updated commit sha1. New commits:
67c7782  Merge branch 'develop' of git://trac.sagemath.org/sage into t/15692/ticket/15692

comment:30 followup: ↓ 31 Changed 6 years ago by
What's the use case for pickle=True
? I would say that a cache should never be pickled.
comment:31 in reply to: ↑ 30 ; followup: ↓ 32 Changed 6 years ago by
Replying to jdemeyer:
What's the use case for
pickle=True
? I would say that a cache should never be pickled.
If something in cache is expensive to compute and you want to dump your object so that later you can load it with cache full. I use it regularly for my personal classes and some time ago it was also used for lists of reflexive polytopes (to store points and faces that were taking many seconds to recompute). One may argue whether it is the best way philosophically or not, but it is easy and it works, so it is nice to have this possibility available.
comment:32 in reply to: ↑ 31 Changed 6 years ago by
Replying to novoselt:
If something in cache is expensive to compute and you want to dump your object so that later you can load it with cache full. I use it regularly for my personal classes and some time ago it was also used for lists of reflexive polytopes (to store points and faces that were taking many seconds to recompute). One may argue whether it is the best way philosophically or not, but it is easy and it works, so it is nice to have this possibility available.
That seems like an argument for making pickle=True
the default.
Something else: couldn't this be implemented more easily by a new class instead of adding pickle
arguments everywhere?
comment:33 Changed 6 years ago by
 Milestone changed from sage6.9 to sage7.0
 Status changed from needs_review to needs_work
One easy simplification: make CacheDict
never pickle its contents and just choose the type dict
or CacheDict
.
comment:34 Changed 6 years ago by
One easy simplification: make CacheDict
never pickle its contents and just choose the type dict
or CacheDict
.
comment:35 Changed 6 years ago by
This is not grammatically correct:
Per default, the contents of the cache are not pickle::
comment:36 followup: ↓ 40 Changed 6 years ago by
Can you replace the pickle
keyword and attribute to do_pickle
since that more accurately describes the intention.
Also, please explain the reason for changing both the pickling/unpickling of the cached function themselves as well as the CacheDict
. It seems that they both solve your problem and I don't see why this problem needs to be solved twice.
comment:37 Changed 5 years ago by
 Commit changed from 67c7782df66179ebc0e7dd09b2518f76b6ecf198 to 54539bcd5bddd7038fb5b0fd46a992c6ff96d238
Branch pushed to git repo; I updated commit sha1. New commits:
54539bc  Merge branch 'develop' into t/15692/ticket/15692

comment:38 Changed 5 years ago by
 Commit changed from 54539bcd5bddd7038fb5b0fd46a992c6ff96d238 to 505c7af667df54ee28441a74da0896fe358ddb31
comment:39 Changed 5 years ago by
 Commit changed from 505c7af667df54ee28441a74da0896fe358ddb31 to e3a7eb4e7ec0852fb058e5455d270067846ea2b6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e3a7eb4  Do not pickle cached methods

comment:40 in reply to: ↑ 36 Changed 5 years ago by
Replying to jdemeyer:
Can you replace the
pickle
keyword and attribute todo_pickle
since that more accurately describes the intention.
Fixed.
Also, please explain the reason for changing both the pickling/unpickling of the cached function themselves as well as the
CacheDict
. It seems that they both solve your problem and I don't see why this problem needs to be solved twice.
You are right. I did look at the (do_)pickle parameter in too many places.
comment:41 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:42 Changed 5 years ago by
 Keywords days71 added
comment:43 Changed 5 years ago by
 Commit changed from e3a7eb4e7ec0852fb058e5455d270067846ea2b6 to 746dc53e1d767870430a3770eef1159c6534b9b1
Branch pushed to git repo; I updated commit sha1. New commits:
746dc53  fix grammar in doctest

comment:44 Changed 5 years ago by
 Reviewers changed from Vincent Delecroix to Vincent Delecroix, David Roe
 Status changed from needs_review to positive_review
Looks good to me.
comment:45 Changed 5 years ago by
I'm not going to change this from positive review, but I found the tiniest doctest typo:
+ Per default, the contents of the cache are not pickle::
pickle should be pickled here.
comment:46 Changed 5 years ago by
I think you're looking at an old version; that typo was what was fixed in 746dc53.
comment:47 Changed 5 years ago by
Awesome!
comment:48 Changed 5 years ago by
 Status changed from positive_review to needs_work
Fails on 32bit:
sage t long src/sage/misc/cachefunc.pyx ********************************************************************** File "src/sage/misc/cachefunc.pyx", line 3161, in sage.misc.cachefunc.cached_method Failed example: id(d) == hash(d) Expected: True Got: False ********************************************************************** 1 item had failures: 1 of 30 in sage.misc.cachefunc.cached_method [843 tests, 1 failure, 8.96 s]
comment:49 Changed 5 years ago by
 Commit changed from 746dc53e1d767870430a3770eef1159c6534b9b1 to 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4
comment:50 Changed 5 years ago by
 Status changed from needs_work to positive_review
comment:51 Changed 5 years ago by
Since this is such a minor change, I set this back to positive review.
comment:52 Changed 5 years ago by
I suspect I am getting this in sageongentoo pulling volker's current develop branch because of this:
File "/usr/lib64/python2.7/sitepackages/sage/modular/overconvergent/weightspace.py", line 473, in sage.modular.overconvergent.weightspace.AlgebraicWeight Failed example: w == loads(dumps(w)) Exception raised: Traceback (most recent call last): File "/usr/lib64/python2.7/sitepackages/sage/doctest/forker.py", line 501, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/lib64/python2.7/sitepackages/sage/doctest/forker.py", line 864, in compile_and_execute exec(compiled, globs) File "<doctest sage.modular.overconvergent.weightspace.AlgebraicWeight[3]>", line 1, in <module> w == loads(dumps(w)) File "sage/structure/element.pyx", line 913, in sage.structure.element.Element.__richcmp__ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/element.c:9542) return (<Element>self)._richcmp_(<Element>other, op) File "sage/structure/element.pyx", line 948, in sage.structure.element.Element._richcmp_ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/element.c:9722) c = left._cmp_(right) File "sage/structure/element.pyx", line 964, in sage.structure.element.Element._cmp_ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/element.c:10101) return left_cmp(right) File "/usr/lib64/python2.7/sitepackages/sage/modular/overconvergent/weightspace.py", line 417, in __cmp__ return cmp(self.values_on_gens(), other.values_on_gens()) File "/usr/lib64/python2.7/sitepackages/sage/modular/overconvergent/weightspace.py", line 380, in values_on_gens return ( self(self.parent()._param), self.teichmuller_type()) File "/usr/lib64/python2.7/sitepackages/sage/modular/overconvergent/weightspace.py", line 539, in __call__ if self._p**(x.precision_absolute()) < self._chi.conductor(): File "sage/misc/cachefunc.pyx", line 1717, in sage.misc.cachefunc.CachedMethodPickle.__call__ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/misc/cachefunc.c:8703) return CM(*args,**kwds) File "sage/misc/cachefunc.pyx", line 2401, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/misc/cachefunc.c:12708) self.cache = f(self._instance) File "/usr/lib64/python2.7/sitepackages/sage/modular/dirichlet.py", line 755, in conductor if self.modulus() == 1 or self.is_trivial(): File "sage/misc/cachefunc.pyx", line 1717, in sage.misc.cachefunc.CachedMethodPickle.__call__ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/misc/cachefunc.c:8703) return CM(*args,**kwds) File "sage/misc/cachefunc.pyx", line 2401, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/misc/cachefunc.c:12708) self.cache = f(self._instance)
and ending after many lines with
File "sage/misc/cachefunc.pyx", line 2401, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/misc/cachefunc.c:12708) self.cache = f(self._instance) File "/usr/lib64/python2.7/sitepackages/sage/modular/dirichlet.py", line 1738, in element v = M([dlog[x] for x in self.values_on_gens()]) File "/usr/lib64/python2.7/sitepackages/sage/modular/dirichlet.py", line 1705, in values_on_gens v = tuple([pows[i] for i in self.element()]) File "sage/misc/cachefunc.pyx", line 2401, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/misc/cachefunc.c:12708) self.cache = f(self._instance) File "/usr/lib64/python2.7/sitepackages/sage/modular/dirichlet.py", line 1730, in element M = P._module File "/usr/lib64/python2.7/sitepackages/sage/modular/dirichlet.py", line 2137, in _module len(self.unit_gens())) File "sage/structure/factory.pyx", line 361, in sage.structure.factory.UniqueFactory.__call__ (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/factory.c:1585) key, kwds = self.create_key_and_extra_args(*args, **kwds) File "sage/structure/factory.pyx", line 459, in sage.structure.factory.UniqueFactory.create_key_and_extra_args (/scratch2/portage/scimathematics/sage9999/work/sage9999/srcpython2_7/build/cythonized/sage/structure/factory.c:2722) return self.create_key(*args, **kwds), {} File "/usr/lib64/python2.7/sitepackages/sage/modules/free_module.py", line 336, in create_key rank = int(sage.rings.integer.Integer(rank)) RuntimeError: maximum recursion depth exceeded while calling a Python object ********************************************************************** 1 item had failures: 2 of 6 in sage.modular.overconvergent.weightspace.AlgebraicWeight [108 tests, 2 failures, 0.32 s]  sage t long /usr/lib64/python2.7/sitepackages/sage/modular/overconvergent/weightspace.py # 2 doctests failed 
There are two of those. There is always the possibility that I missed a patch in python
or some other place that deals with this but I don't think so.
comment:53 Changed 5 years ago by
 Branch changed from u/saraedum/ticket/15692 to 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4
 Resolution set to fixed
 Status changed from positive_review to closed
comment:54 Changed 5 years ago by
 Commit 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4 deleted
 Resolution fixed deleted
 Status changed from closed to new
comment:55 Changed 5 years ago by
Indeed the two doctest failures
comment:56 Changed 5 years ago by
Actually the main failure is in sage/modular/dirichlet.py
, the other sage/modular/overconvergent/weightspace.py
fails because it is calling dirichlet. So one only needs to fix dirichlet.py.
comment:57 Changed 5 years ago by
 Branch changed from 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4 to u/saraedum/ticket/15692
 Commit set to 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4
comment:58 Changed 5 years ago by
 Commit changed from 7ef7a4120395d2645c4246bb23ffb9d3f7183dd4 to a152dc4a2d7f07841152421bde45c003b6d2b6f1
Branch pushed to git repo; I updated commit sha1. New commits:
d2da8dc  remove unused do_pickle field from CachedMethod

6c36386  Merge branch 'u/saraedum/ticket/15692' of git://trac.sagemath.org/sage into t/15692/ticket/15692

125ec7f  Merge branch 'develop' of https://github.com/sagemath/sage into t/15692/ticket/15692

aaebb01  Merge branch 'develop' into t/15692/ticket/15692

a152dc4  Pickle caches of dirichlet characters

comment:59 Changed 5 years ago by
 Status changed from new to needs_review
The dirichlet.py
doctests pass now. Let's see what the patchbot thinks about the rest.
comment:60 Changed 5 years ago by
 Status changed from needs_review to needs_work
 Work issues set to pickle jar
comment:61 Changed 5 years ago by
 Commit changed from a152dc4a2d7f07841152421bde45c003b6d2b6f1 to 141af2efcb44d8048fed779d2b719d20a243c31c
Branch pushed to git repo; I updated commit sha1. New commits:
141af2e  Unpickle legacy dirichlet characters

comment:62 Changed 5 years ago by
 Status changed from needs_work to needs_review
 Work issues pickle jar deleted
comment:63 Changed 5 years ago by
 Status changed from needs_review to needs_work
Looks like your last commit needs a tiny little bit more work
sage t long src/sage/modular/dirichlet.py ********************************************************************** File "src/sage/modular/dirichlet.py", line 1760, in sage.modular.dirichlet.DirichletCharacter.__setstate__ Failed example: loads(dumps(e)) == e Expected nothing Got: True
From the bot.
comment:64 Changed 5 years ago by
 Commit changed from 141af2efcb44d8048fed779d2b719d20a243c31c to a18cbf54adcc6684fdd9206fa837fe07a2942215
Branch pushed to git repo; I updated commit sha1. New commits:
a18cbf5  Fixed doctest for element of DirichletGroup

comment:65 Changed 5 years ago by
Sorry, that was dumb. I hope it's better now :)
New commits:
a18cbf5  Fixed doctest for element of DirichletGroup

comment:66 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:67 Changed 5 years ago by
 Status changed from needs_review to positive_review
Bot happy, I am putting this back to positive.
comment:68 Changed 5 years ago by
 Branch changed from u/saraedum/ticket/15692 to a18cbf54adcc6684fdd9206fa837fe07a2942215
 Resolution set to fixed
 Status changed from positive_review to closed
comment:69 Changed 5 years ago by
 Commit a18cbf54adcc6684fdd9206fa837fe07a2942215 deleted
In #15149 it was proposed that caches should be 'optin and not optout' (comment:6:ticket:15149). Imho most caches are not used for things that are horribly expensive to compute, they are just too expensive to be computed again all the time.
Would somebody mind if I implemented it that way, i.e., the default for a cache would be that it is not pickled?