Opened 11 months ago
Closed 9 months ago
#31182 closed defect (fixed)
Mutability class and pickling
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  misc  Keywords:  
Cc:  tscrim, mkoeppe, ghtobiasdiez, nbruin  Merged in:  
Authors:  Michael Jung  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  6cbd1fd (Commits, GitHub, GitLab)  Commit:  6cbd1fd918ef9fdc73310b10e9a50e6c9bf4b7ed 
Dependencies:  #31196  Stopgaps: 
Description (last modified by )
If the class Mutability
is used as mixin for mutability features, the pickling goes wrong (see here).
An attempt using this mixin class after #30281, still throws a pickling test error, e.g. in #30310:
File "src/sage/manifolds/chart_func.py", line 2944, in sage.manifolds.chart_func.MultiCoordFunction.__init__ Failed example: TestSuite(f).run() Expected nothing Got: Failure in _test_pickling: Traceback (most recent call last): File "sage/misc/persist.pyx", line 291, in sage.misc.persist.dumps (build/cythonized/sage/misc/persist.c:4284) return obj.dumps(compress) File "sage/structure/sage_object.pyx", line 476, in sage.structure.sage_object.SageObject.dumps (build/cythonized/sage/structure/sage_object.c:3790) return _base_dumps(self, compress=compress) File "sage/misc/persist.pyx", line 264, in sage.misc.persist._base_dumps (build/cythonized/sage/misc/persist.c:4012) gherkin = SagePickler.dumps(obj) File "sage/misc/persist.pyx", line 771, in sage.misc.persist.SagePickler.dumps (build/cythonized/sage/misc/persist.c:6801) pickler.dump(obj) TypeError: cannot pickle 'MultiCoordFunction' object <BLANKLINE> During handling of the above exception, another exception occurred: <BLANKLINE> Traceback (most recent call last): File "/home/michi/Projekte/sagedevel/local/lib/python3.8/sitepackages/sage/misc/sage_unittest.py", line 297, in run test_method(tester=tester) File "sage/structure/sage_object.pyx", line 647, in sage.structure.sage_object.SageObject._test_pickling (build/cythonized/sage/structure/sage_object.c:5007) tester.assertEqual(loads(dumps(self)), self) File "sage/misc/persist.pyx", line 293, in sage.misc.persist.dumps (build/cythonized/sage/misc/persist.c:4337) return _base_dumps(obj, compress=compress) File "sage/misc/persist.pyx", line 264, in sage.misc.persist._base_dumps (build/cythonized/sage/misc/persist.c:4012) gherkin = SagePickler.dumps(obj) File "sage/misc/persist.pyx", line 771, in sage.misc.persist.SagePickler.dumps (build/cythonized/sage/misc/persist.c:6801) pickler.dump(obj) TypeError: cannot pickle 'MultiCoordFunction' object  The following tests failed: _test_pickling
I found out that a noncythonized alternative works without errors.
Change History (31)
comment:1 Changed 11 months ago by
comment:2 Changed 11 months ago by
 Description modified (diff)
comment:3 Changed 11 months ago by
comment:4 Changed 11 months ago by
 Description modified (diff)
comment:5 Changed 11 months ago by
 Branch set to u/ghmjungmath/mutability_class_and_pickling
comment:6 Changed 11 months ago by
 Commit set to 439d0d0a1cb9cd0af35f1ee33962f8c6f26a49ce
New commits:
439d0d0  Trac #31182: enable auto pickle

comment:7 Changed 11 months ago by
 Status changed from new to needs_review
comment:8 Changed 11 months ago by
 Description modified (diff)
Patchbot is green. The change works with ticket #30310 and according changes.
comment:9 Changed 11 months ago by
 Cc ghtobiasdiez added
comment:10 Changed 11 months ago by
 Cc nbruin added
comment:11 Changed 11 months ago by
Would be nice if anyone could give adequate feedback, or cc someone who ought to be an expert. Many tickets depend on it (see #30261).
comment:12 followup: ↓ 13 Changed 11 months ago by
Why is it a Cython class anyway? Multiple inheritances among Cython classes are not going to work and for Python classes it seems to cause some troubles. I'd vote for turning Mutability
into a pure Python mixin class.
If one really needs the functionality on Cython level, what about a class SageObjectWithMutability
instead?
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 11 months ago by
Replying to ghmjungmath:
Why is it a Cython class anyway? Multiple inheritances among Cython classes are not going to work and for Python classes it seems to cause some troubles. I'd vote for turning
Mutability
into a pure Python mixin class.
This makes no sense. Having this be a Cython class does not influence whether another class *Python* class can inherit this, which can be done in Cython as well. You run into the exact same problem if you want another cdef
class to inherit from this and another cdef
class, which is actually worse because a cdef
class cannot inherit from a Python class.
If one really needs the functionality on Cython level, what about a class
SageObjectWithMutability
instead?
For speed, pure and simple.
comment:14 in reply to: ↑ 13 Changed 11 months ago by
comment:15 Changed 11 months ago by
comment:16 followup: ↓ 17 Changed 11 months ago by
comment:17 in reply to: ↑ 16 Changed 11 months ago by
Replying to ghmjungmath:
Replying to tscrim:
Replying to ghmjungmath:
Why is it a Cython class anyway? Multiple inheritances among Cython classes are not going to work and for Python classes it seems to cause some troubles. I'd vote for turning
Mutability
into a pure Python mixin class.This makes no sense. Having this be a Cython class does not influence whether another class *Python* class can inherit this, which can be done in Cython as well.
Right, it should work, and somehow the pickling is affected.
That is likely an issue with how the pickling is implemented and affects all C/Python classes equally. This likely requires implementations of __reduce__
in the corresponding classes that use it. It's been awhile since I've looked this closely at a pickling issue. There might be some better/more general way to do it.
You run into the exact same problem if you want another
cdef
class to inherit from this and anothercdef
class, which is actually worse because acdef
class cannot inherit from a Python class.I tend to disagree.
What I said isn't an opinion but a fact.
The class
Mutability
is intended to be mixed in, right? Every class in Sage must inherit fromSageObject
, either directly or within an inheritance tree. In order to useMutability
, we need therefore two inheritances sinceMutability
does not inherit fromSageObject
. But a second inheritance is not allowed if the subsequent class is a Cython class as well. Havingclass Sequence(SageObject, Mutability)
on the other hand is allowed but only ifMutability
is a Python class (see here for details).
There is a key difference here: the class being an *extension type*, i.e., a cdef
class. If you don't believe me, try it. Here is a minimal example:
sage: %%cython ....: cdef class Foo: ....: pass ....: cdef class Bar: ....: pass ....: class MyClass(Foo, Bar): # Change to cdef class to see it fail ....: pass ....: sage: C = MyClass()
My point is, classes which inherit from another Cython class, in particular any
cdef
class, can therefore never benefit from this mix in class ifMutability
stays Cythonic (this should hold true for almost every class in Sage).
This is not true. In fact, there are lots of examples in Sage that use Parent
and WithEqualityById
via UniqueRepresentation
.
On top of it all, the class as it is right now has no use at all. It's neither working on Python classes due to the above error nor on Cython classes due to its inheritance behavior.
If one really needs the functionality on Cython level, what about a class
SageObjectWithMutability
instead?For speed, pure and simple.
I see the need of a speedup. And I mean, we can keep this class as
cdef
as it is if we get the above bug to work. Then it can only be used in a very limited range of classes in Sage.
See above.
In any case, due to the above reasons, I suggest a class
SageObjectWithMutability
inheriting fromSageObject
implementing the mutability feature. That would be useful forSequence
,BundleConnection
,AffineConnection
and probably even more.
Strong 1. See above.
comment:18 Changed 11 months ago by
Ah shit! I remember, we had the same discussion in #30261. For some reason, it was burnt into my head that it wasn't supposed to work. Big sorry and thanks for your patience! #shameonme #embarrassed
comment:19 Changed 11 months ago by
@Travis: What about that?
def __getstate__(self): state = self.__dict__ state['_is_immutable'] = self._is_immutable return state
Works perfectly.
comment:20 followup: ↓ 21 Changed 11 months ago by
 Commit changed from 439d0d0a1cb9cd0af35f1ee33962f8c6f26a49ce to 1c5d47d8c9f1ccca7bafcd45eb6f67d4f71c64db
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1c5d47d  Trac #31182: __getstate__ and __setstate__

comment:21 in reply to: ↑ 20 Changed 11 months ago by
comment:22 Changed 11 months ago by
 Dependencies set to #31196
comment:23 Changed 11 months ago by
 Commit changed from 1c5d47d8c9f1ccca7bafcd45eb6f67d4f71c64db to d6d6ba416ad27f586ffbe2137c7a1867f1a78130
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5ecbf3a  Trac #31181: return added

4c33935  Trac #31196: minor code improvements, py3 compatibility, documentation improved

e5228d3  Trac #31196: cpdef require methods + example added

d957f73  Trac #31196: unnecessary line in docstring removed

d6d6ba4  Trac #31182: __getstate__ and __setstate__

comment:24 Changed 11 months ago by
If this approach gets a positive review as well as #30284 based upon that, I'll add an explicit doctest for the pickling with #30284 to this file.
If I understand this correctly, __setstate__
and __getstate__
are high level methods so that any implementation of __reduce__
would overwrite their behavior, which is exactly what we want.
comment:25 Changed 11 months ago by
 Commit changed from d6d6ba416ad27f586ffbe2137c7a1867f1a78130 to 6cbd1fd918ef9fdc73310b10e9a50e6c9bf4b7ed
Branch pushed to git repo; I updated commit sha1. New commits:
6cbd1fd  Trac #31182: doctests added for __setstate__ and __getstate__

comment:26 Changed 11 months ago by
This would work for any Python class I guess. If they implement their own version, then they are already taking responsibility for handling the mutability. It is a little hacky, but it is not so bad and it works for what is likely most cases. Let's see what the patchbot says.
comment:27 Changed 11 months ago by
I tried to write a test into the docstring:
sage: class A(SageObject, Mutability): ....: def __init__(self, val): ....: self._val = val ....: def change(self, val): ....: self._require_mutable() ....: self._val = val ....: def __hash__(self): ....: self._require_immutable() ....: return hash(self._val) sage: a = A(4) sage: loads(dumps(a))
The doctest fails with:
Failed example: loads(dumps(a)) Exception raised: Traceback (most recent call last): File "/home/michi/Projekte/sagedevel/local/lib/python3.8/sitepackages/sage/doctest/forker.py", line 714, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/michi/Projekte/sagedevel/local/lib/python3.8/sitepackages/sage/doctest/forker.py", line 1133, in compile_and_execute exec(compiled, globs) File "<doctest sage.structure.mutability.Mutability.__getstate__[2]>", line 1, in <module> loads(dumps(a)) File "sage/misc/persist.pyx", line 291, in sage.misc.persist.dumps (build/cythonized/sage/misc/persist.c:4284) return obj.dumps(compress) File "sage/structure/sage_object.pyx", line 476, in sage.structure.sage_object.SageObject.dumps (build/cythonized/sage/structure/sage_object.c:3789) return _base_dumps(self, compress=compress) File "sage/misc/persist.pyx", line 264, in sage.misc.persist._base_dumps (build/cythonized/sage/misc/persist.c:4012) gherkin = SagePickler.dumps(obj) File "sage/misc/persist.pyx", line 771, in sage.misc.persist.SagePickler.dumps (build/cythonized/sage/misc/persist.c:6801) pickler.dump(obj) _pickle.PicklingError: Can't pickle <class '__main__.A'>: attribute lookup A on __main__ failed
It is a bit odd since it works perfectly within a usual Sage instance. Only as a doctest this fails. Doctests where Mutability
is included in #30284 on the other hand work perfectly again.
comment:28 Changed 11 months ago by
Okay, seems to be perfectly normal, see _test_pickling
in sage_object.pyx
:
sage: class Bla(SageObject): pass sage: Bla()._test_pickling() Traceback (most recent call last): ... PicklingError: Can't pickle <class '__main__.Bla'>: attribute lookup ... failed TODO: for a stronger test, this could send the object to a remote Sage session, and get it back.
I'll just add, as already suggested, some tests from #30284 as soon as this ticket is ready, too.
comment:29 Changed 11 months ago by
Patchbot is satisfied here, too. :)
comment:30 Changed 11 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
This will work for now. We can revisit this if further problems arise as we use this class more frequently.
comment:31 Changed 9 months ago by
 Branch changed from u/ghmjungmath/mutability_class_and_pickling to 6cbd1fd918ef9fdc73310b10e9a50e6c9bf4b7ed
 Resolution set to fixed
 Status changed from positive_review to closed
Turns out that
helps.
But I cannot really tell why that is and if that's a reasonable approach. I am not a pickling expert.
See https://cython.readthedocs.io/en/latest/src/userguide/extension_types.html#controllingpickling.