Opened 3 years ago
Last modified 2 months ago
#28096 needs_review defect
Cached methods with do_pickle=True fail for UniqueRepresentation objects
Reported by: | klee | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-9.7 |
Component: | pickling | Keywords: | |
Cc: | SimonKing, tscrim, nbruin | Merged in: | |
Authors: | Kwankyu Lee | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/klee/28096 (Commits, GitHub, GitLab) | Commit: | 6f14c1cfb108f6b5f2146be0678d8a4112c2b948 |
Dependencies: | Stopgaps: |
Description (last modified by )
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
Change History (45)
comment:1 Changed 3 years ago by
- Branch set to u/klee/28096
comment:2 Changed 3 years ago by
- Commit set to f17e57ac893ad00d63f4f20d90f32bc53a676b32
comment:3 Changed 3 years ago by
- Status changed from new to needs_review
comment:4 Changed 3 years ago by
We can definitely not turn pickling on by default. See #15692.
comment:5 Changed 3 years ago by
This is not about turn pickling on by default. This turns on pickling only for @cached_methods
with do_pickle=True
.
comment:6 Changed 3 years ago by
This ticket makes UniqueRepresentation
objects do what other sage objects do, for pickling cached methods.
comment:7 Changed 3 years ago by
- Commit changed from f17e57ac893ad00d63f4f20d90f32bc53a676b32 to 09fc937402ebf6e1ef8c531739662cd6e48c9753
Branch pushed to git repo; I updated commit sha1. New commits:
09fc937 | Ignore cached functions with do_pickle=False
|
comment:8 Changed 3 years ago by
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.
comment:9 Changed 3 years ago by
- Commit changed from 09fc937402ebf6e1ef8c531739662cd6e48c9753 to c97963582ae90fe0aabbff4b8e1f3e60304f4cd4
Branch pushed to git repo; I updated commit sha1. New commits:
c979635 | Fix a format mistake
|
comment:10 Changed 3 years ago by
- Commit changed from c97963582ae90fe0aabbff4b8e1f3e60304f4cd4 to 9d6e0c799ac4c198e98e2298fd318b152d419b84
comment:11 Changed 3 years ago by
- Commit changed from 9d6e0c799ac4c198e98e2298fd318b152d419b84 to 6d47b688ba68b713ac0d0b73d80a28a7747559e7
Branch pushed to git repo; I updated commit sha1. New commits:
6d47b68 | A little more documentation fixes
|
comment:12 Changed 3 years ago by
- Description modified (diff)
comment:13 Changed 3 years ago by
- Cc SimonKing added
comment:14 follow-up: ↓ 15 Changed 3 years ago by
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?
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 17 Changed 3 years ago by
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.
comment:16 Changed 3 years ago by
- Commit changed from 6d47b688ba68b713ac0d0b73d80a28a7747559e7 to bcd500ed0a00f1af7966dadd050a62e5e43b6520
comment:17 in reply to: ↑ 15 ; follow-up: ↓ 19 Changed 3 years ago by
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.
comment:18 Changed 3 years ago by
- Commit changed from bcd500ed0a00f1af7966dadd050a62e5e43b6520 to 084a2abb8d5c7b28d952d8f39265d5086c7ee9c9
Branch pushed to git repo; I updated commit sha1. New commits:
084a2ab | Add back absolute_import
|
comment:19 in reply to: ↑ 17 Changed 3 years ago by
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!
comment:20 Changed 3 years ago by
After studying how cython objects pickling works, I am now more confident that the patch works well as promised.
comment:21 Changed 3 years ago by
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.
comment:22 Changed 3 years ago by
- Commit changed from 084a2abb8d5c7b28d952d8f39265d5086c7ee9c9 to f84fef6fbf104649f87016daa85380c2480bffbb
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f84fef6 | Make UniqueRepresentation do_pickle
|
comment:23 Changed 3 years ago by
This ticket deserves more attention. For your info, the branch is python3-safe.
comment:24 Changed 3 years ago by
- Commit changed from f84fef6fbf104649f87016daa85380c2480bffbb to 874f6c64cb0a383f8b53a309ee0bb67440cc54c8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
874f6c6 | Make UniqueRepresentation do_pickle
|
comment:25 Changed 3 years ago by
- Commit changed from 874f6c64cb0a383f8b53a309ee0bb67440cc54c8 to d1c7efa211954447fb22351a2eefcf05080edc46
comment:26 Changed 3 years ago by
- Status changed from needs_review to needs_work
comment:27 Changed 3 years ago by
- Commit changed from d1c7efa211954447fb22351a2eefcf05080edc46 to 0fd1acca08965aeb8a77bc4ec280d9c84df44685
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0fd1acc | Make UniqueRepresentation do_pickle
|
comment:28 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:29 Changed 2 years ago by
- Milestone changed from sage-8.9 to sage-9.1
Ticket retargeted after milestone closed
comment:30 Changed 2 years ago by
- Milestone changed from sage-9.1 to sage-9.2
Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.
comment:31 Changed 2 years ago by
- Cc tscrim added
- Status changed from needs_review to needs_work
- Work issues set to needs rebase
comment:32 Changed 2 years ago by
- Commit changed from 0fd1acca08965aeb8a77bc4ec280d9c84df44685 to 727b23f14038a0ba21020426e41501cee0dcb02f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
727b23f | Make do_pickle work for UniqueRepresentation objects
|
comment:33 Changed 2 years ago by
- Commit changed from 727b23f14038a0ba21020426e41501cee0dcb02f to f0b387e8a76e1ad93e6d413ba1601549fd19c41a
Branch pushed to git repo; I updated commit sha1. New commits:
f0b387e | Fix typos
|
comment:34 Changed 2 years ago by
I am not sure if this patch would incur any bad side effects. Experts on UniqueRepresentation
are welcome to scrutinize.
comment:35 Changed 2 years ago by
- Status changed from needs_work to needs_review
- Work issues needs rebase deleted
comment:36 Changed 2 years ago by
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.
comment:37 Changed 2 years ago by
- Cc nbruin added
comment:38 Changed 20 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:39 Changed 16 months ago by
- Milestone changed from sage-9.3 to sage-9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:40 Changed 11 months ago by
- Milestone changed from sage-9.4 to sage-9.5
Setting a new milestone for this ticket based on a cursory review.
comment:41 Changed 6 months ago by
- Milestone changed from sage-9.5 to sage-9.6
Stalled in needs_review
or needs_info
; likely won't make it into Sage 9.5.
comment:43 Changed 3 months ago by
- Commit changed from f0b387e8a76e1ad93e6d413ba1601549fd19c41a to 6f14c1cfb108f6b5f2146be0678d8a4112c2b948
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6f14c1c | Make do_pickle work for UniqueRepresentation objects
|
comment:44 Changed 3 months ago by
- Status changed from needs_work to needs_review
comment:45 Changed 2 months ago by
- Milestone changed from sage-9.6 to sage-9.7
Branch pushed to git repo; I updated commit sha1. New commits:
Make UniqueRepresentation do_pickle