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: sage-9.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:

Status badges

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/sage-build/local/lib/python3.7/site-packages/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 Travis Scrimshaw

Cc: Jeroen Demeyer Simon King added

Simon, Jeroen, do you have any ideas on how to best do this?

comment:2 Changed 3 years ago by Simon King

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 hard-coded 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 Changed 3 years ago by Travis Scrimshaw

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 in reply to:  3 Changed 3 years ago by Nils Bruin

Replying to tscrim:

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__()

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 straight-up weakref, so you can dereference it with self.US(). If you get None than the acted-upon 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 implementation-dependent.

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 Travis Scrimshaw

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 Travis Scrimshaw

Authors: Travis Scrimshaw
Branch: public/pickling/actions-29031
Commit: 4958fdbd915bb6d90faa9684ae1a9e2f58ca6798
Status: newneeds_review

Okay, here is the branch with the custom __reduce__ methods.


New commits:

4958fdbAdding custom __reduce__ methods to Action and subclasses.

comment:7 Changed 3 years ago by Matthias Köppe

Milestone: sage-9.1sage-9.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 git

Commit: 4958fdbd915bb6d90faa9684ae1a9e2f58ca6798cbf439f968dc014d5cb8aa993ace33f08dfb96b4

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

cbf439fMerge branch 'public/pickling/actions-29031' of git://trac.sagemath.org/sage into t/29031/actions

comment:9 Changed 2 years ago by Sébastien Labbé

Status: needs_reviewneeds_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 Sébastien Labbé

Reviewers: Sébastien Labbé

comment:11 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:12 Changed 2 years ago by git

Commit: cbf439f968dc014d5cb8aa993ace33f08dfb96b4d4991d3b5ef441209187a01acd56a97b03e4d36a

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

d4991d3is can

comment:13 Changed 2 years ago by David Roe

Status: needs_workpositive_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 Volker Braun

Branch: public/pickling/actions-29031d4991d3b5ef441209187a01acd56a97b03e4d36a
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.