Opened 3 years ago
Closed 2 years ago
#29031 closed defect (fixed)
Actions cannot be pickled
Reported by:  Travis Scrimshaw  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  pickling  Keywords:  actions 
Cc:  Jeroen Demeyer, Simon King, Nils Bruin  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Sébastien Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  d4991d3 (Commits, GitHub, GitLab)  Commit:  d4991d3b5ef441209187a01acd56a97b03e4d36a 
Dependencies:  Stopgaps: 
Description
We cannot pickle actions:
sage: V = QQ^3 sage: v = V((1, 2, 3)) sage: cm = get_coercion_model() sage: a = cm.get_action(V, QQ, operator.mul) sage: dumps(a)  AttributeError Traceback (most recent call last) [snip] /home/travis/sagebuild/local/lib/python3.7/sitepackages/sage/categories/functor.pyx in sage.categories.functor.Functor.__reduce__ (build/cythonized/sage/categories/functor.c:2242)() 212 """ 213 return (_Functor_unpickle, > 214 (self.__class__, list(self.__dict__.items()), 215 self.__domain, self.__codomain)) 216 AttributeError: 'sage.structure.coerce_actions.RightModuleAction' object has no attribute '__dict__'
This might require some custom __reduce__
methods unless we give the corresponding Cython classes a __dict__
.
Change History (14)
comment:1 Changed 3 years ago by
Cc:  Jeroen Demeyer Simon King added 

comment:2 Changed 3 years ago by
I don't have time right now to look into the code, but if I recall correctly, we have different (cython)types for different actions. I.e., in each case it is hardcoded what the action does (for instance: "apply the ._lmul_ method of the left factor to the the right factor" or so).
From what I recall, the input data for any action is the pair (acting parent, acted upon parent). Hence, something like
def __reduce__(self): return type(self), (the two parents)
should work.
comment:3 followup: 4 Changed 3 years ago by
Cc:  Nils Bruin added 

Okay, so for this I was thinking Functor
and its subclasses (in particular, Action
) should mimic Map
and have an _extra_slots
and _update_slots
methods. However, this doesn't work as I get errors of the form:
TypeError: sage.categories.functor.Functor.__new__(sage.structure.coerce_actions.LeftModuleAction) is not safe, use sage.structure.coerce_actions.LeftModuleAction.__new__()
and I don't know how to get around that with the current attempt (of course, I can fall back to custom __reduce__
for each class). I also have another issue irregardless, the weakref
in the Action.US
slot, and I don't know how to get the object. Any ideas?
comment:4 Changed 3 years ago by
Replying to tscrim:
Okay, so for this I was thinking
Functor
and its subclasses (in particular,Action
) should mimicMap
and have an_extra_slots
and_update_slots
methods. However, this doesn't work as I get errors of the form:TypeError: sage.categories.functor.Functor.__new__(sage.structure.coerce_actions.LeftModuleAction) is not safe, use sage.structure.coerce_actions.LeftModuleAction.__new__()
I think that's because actions have another memory layout. Looking at coerce_action.pxd
, the problem is ModuleAction
, so that would probably be the right spot to implement the custom reduce
.
The fact that there's a weakref indicates the need for a custom reduce anyway, unless there's an established pattern recognized already (so that the reduce can exist higher up): the US
slot is just a straightup weakref, so you can dereference it with self.US()
. If you get None
than the actedupon set was already garbage collected. Then the action is defunct and hence unpickleable.
You might be able to get away with the following on Action
:
def __reduce__(self): return type(self), (self.G, self.underlying_set())
where underlying_set
derefs the weakref and raises an error if defunct.
In the end, I think it's much safer to pickle these things with a call to the actual constructor than via a new
and slot manipulations: it's much less implementationdependent.
For the most part, I don't think these actions SHOULD be pickled anyway, but rather rediscovered. But because they should just be constructible from their actor and set, pickling them via a constructor is fairly straightforward.
comment:5 Changed 3 years ago by
Thanks for the explanations. I agree that these particular actions should not be pickled, but I was using them as easy examples for when an action is stored as an attribute of an object. In particular, this came up in #25902. I will implement custom __reduce__
for the relevant classes.
comment:6 Changed 3 years ago by
Authors:  → Travis Scrimshaw 

Branch:  → public/pickling/actions29031 
Commit:  → 4958fdbd915bb6d90faa9684ae1a9e2f58ca6798 
Status:  new → needs_review 
Okay, here is the branch with the custom __reduce__
methods.
New commits:
4958fdb  Adding custom __reduce__ methods to Action and subclasses.

comment:7 Changed 3 years ago by
Milestone:  sage9.1 → sage9.2 

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.
comment:8 Changed 2 years ago by
Commit:  4958fdbd915bb6d90faa9684ae1a9e2f58ca6798 → cbf439f968dc014d5cb8aa993ace33f08dfb96b4 

Branch pushed to git repo; I updated commit sha1. New commits:
cbf439f  Merge branch 'public/pickling/actions29031' of git://trac.sagemath.org/sage into t/29031/actions

comment:9 Changed 2 years ago by
Status:  needs_review → needs_work 

Looks good to me, but it inserts 5 typos. Can you please do the following changes (5 times)?
is can +can
comment:10 Changed 2 years ago by
Reviewers:  → Sébastien Labbé 

comment:11 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:12 Changed 2 years ago by
Commit:  cbf439f968dc014d5cb8aa993ace33f08dfb96b4 → d4991d3b5ef441209187a01acd56a97b03e4d36a 

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

comment:13 Changed 2 years ago by
Status:  needs_work → positive_review 

I fixed the documentation typos that slabbe pointed out, so I'm setting this to positive review.
comment:14 Changed 2 years ago by
Branch:  public/pickling/actions29031 → d4991d3b5ef441209187a01acd56a97b03e4d36a 

Resolution:  → fixed 
Status:  positive_review → closed 
Simon, Jeroen, do you have any ideas on how to best do this?