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:

Status badges

Description (last modified by 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

Change History (45)

comment:1 Changed 3 years ago by klee

  • Authors set to Kwankyu Lee
  • Branch set to u/klee/28096

comment:2 Changed 3 years ago by git

  • Commit set to f17e57ac893ad00d63f4f20d90f32bc53a676b32

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

f17e57aMake UniqueRepresentation do_pickle

comment:3 Changed 3 years ago by klee

  • Status changed from new to needs_review

comment:4 Changed 3 years ago by nbruin

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

comment:5 Changed 3 years ago by klee

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 klee

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

comment:7 Changed 3 years ago by git

  • Commit changed from f17e57ac893ad00d63f4f20d90f32bc53a676b32 to 09fc937402ebf6e1ef8c531739662cd6e48c9753

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

09fc937Ignore cached functions with do_pickle=False

comment:8 Changed 3 years ago by 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.

comment:9 Changed 3 years ago by git

  • Commit changed from 09fc937402ebf6e1ef8c531739662cd6e48c9753 to c97963582ae90fe0aabbff4b8e1f3e60304f4cd4

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

c979635Fix a format mistake

comment:10 Changed 3 years ago by git

  • Commit changed from c97963582ae90fe0aabbff4b8e1f3e60304f4cd4 to 9d6e0c799ac4c198e98e2298fd318b152d419b84

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

983577bMerge branch 'develop'
9d6e0c7Rename to is_pickled_with_cache

comment:11 Changed 3 years ago by git

  • Commit changed from 9d6e0c799ac4c198e98e2298fd318b152d419b84 to 6d47b688ba68b713ac0d0b73d80a28a7747559e7

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

6d47b68A little more documentation fixes

comment:12 Changed 3 years ago by klee

  • Description modified (diff)

comment:13 Changed 3 years ago by SimonKing

  • Cc SimonKing added

comment:14 follow-up: Changed 3 years ago by 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?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by 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.

comment:16 Changed 3 years ago by git

  • Commit changed from 6d47b688ba68b713ac0d0b73d80a28a7747559e7 to bcd500ed0a00f1af7966dadd050a62e5e43b6520

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

c14d10bMerge branch 'develop'
bd0bebbMerge branch 'develop'
bcd500eRevert back unnecessary changes in doctests

comment:17 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by 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.

comment:18 Changed 3 years ago by git

  • Commit changed from bcd500ed0a00f1af7966dadd050a62e5e43b6520 to 084a2abb8d5c7b28d952d8f39265d5086c7ee9c9

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

084a2abAdd back absolute_import

comment:19 in reply to: ↑ 17 Changed 3 years ago by 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!

comment:20 Changed 3 years ago by klee

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 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.

comment:22 Changed 3 years ago by git

  • Commit changed from 084a2abb8d5c7b28d952d8f39265d5086c7ee9c9 to f84fef6fbf104649f87016daa85380c2480bffbb

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

f84fef6Make UniqueRepresentation do_pickle

comment:23 Changed 3 years ago by klee

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

comment:24 Changed 3 years ago by git

  • Commit changed from f84fef6fbf104649f87016daa85380c2480bffbb to 874f6c64cb0a383f8b53a309ee0bb67440cc54c8

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

874f6c6Make UniqueRepresentation do_pickle

comment:25 Changed 3 years ago by git

  • Commit changed from 874f6c64cb0a383f8b53a309ee0bb67440cc54c8 to d1c7efa211954447fb22351a2eefcf05080edc46

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

056dcb3Merge branch 'develop' into picklable-trac28096
5705d47Merge branch 'develop' into picklable-trac28096
d1c7efaMerge branch 'develop'

comment:26 Changed 3 years ago by klee

  • Status changed from needs_review to needs_work

comment:27 Changed 3 years ago by git

  • Commit changed from d1c7efa211954447fb22351a2eefcf05080edc46 to 0fd1acca08965aeb8a77bc4ec280d9c84df44685

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

0fd1accMake UniqueRepresentation do_pickle

comment:28 Changed 3 years ago by klee

  • Status changed from needs_work to needs_review

comment:29 Changed 2 years ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:30 Changed 2 years ago by mkoeppe

  • 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 mkoeppe

  • Cc tscrim added
  • Status changed from needs_review to needs_work
  • Work issues set to needs rebase

comment:32 Changed 2 years ago by git

  • Commit changed from 0fd1acca08965aeb8a77bc4ec280d9c84df44685 to 727b23f14038a0ba21020426e41501cee0dcb02f

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

727b23fMake do_pickle work for UniqueRepresentation objects

comment:33 Changed 2 years ago by git

  • Commit changed from 727b23f14038a0ba21020426e41501cee0dcb02f to f0b387e8a76e1ad93e6d413ba1601549fd19c41a

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

f0b387eFix typos

comment:34 Changed 2 years ago by klee

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

Last edited 2 years ago by klee (previous) (diff)

comment:35 Changed 2 years ago by klee

  • Status changed from needs_work to needs_review
  • Work issues needs rebase deleted

comment:36 Changed 2 years ago by 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.

comment:37 Changed 2 years ago by mkoeppe

  • Cc nbruin added

comment:38 Changed 20 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:39 Changed 16 months ago by mkoeppe

  • 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 mkoeppe

  • 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 mkoeppe

  • 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:42 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

needs rebase

comment:43 Changed 3 months ago by git

  • Commit changed from f0b387e8a76e1ad93e6d413ba1601549fd19c41a to 6f14c1cfb108f6b5f2146be0678d8a4112c2b948

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

6f14c1cMake do_pickle work for UniqueRepresentation objects

comment:44 Changed 3 months ago by klee

  • Status changed from needs_work to needs_review

comment:45 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7
Note: See TracTickets for help on using tickets.