Opened 11 months ago

Closed 9 months ago

#31182 closed defect (fixed)

Mutability class and pickling

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.3
Component: misc Keywords:
Cc: tscrim, mkoeppe, gh-tobiasdiez, 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:

Status badges

Description (last modified by gh-mjungmath)

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/sage-devel/local/lib/python3.8/site-packages/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 non-cythonized alternative works without errors.

Change History (31)

comment:1 Changed 11 months ago by gh-mjungmath

Turns out that

from sage.misc.decorators import sage_wraps
+cimport cython

+@cython.auto_pickle(True)
cdef class Mutability:

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#controlling-pickling.

Last edited 11 months ago by gh-mjungmath (previous) (diff)

comment:2 Changed 11 months ago by gh-mjungmath

  • Description modified (diff)

comment:3 Changed 11 months ago by gh-mjungmath

  • Authors Michael Jung deleted

comment:4 Changed 11 months ago by gh-mjungmath

  • Description modified (diff)

comment:5 Changed 11 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/mutability_class_and_pickling

comment:6 Changed 11 months ago by gh-mjungmath

  • Authors set to Michael Jung
  • Commit set to 439d0d0a1cb9cd0af35f1ee33962f8c6f26a49ce

New commits:

439d0d0Trac #31182: enable auto pickle

comment:7 Changed 11 months ago by gh-mjungmath

  • Status changed from new to needs_review

comment:8 Changed 11 months ago by gh-mjungmath

  • Description modified (diff)

Patchbot is green. The change works with ticket #30310 and according changes.

Last edited 11 months ago by gh-mjungmath (previous) (diff)

comment:9 Changed 11 months ago by gh-mjungmath

  • Cc gh-tobiasdiez added

comment:10 Changed 11 months ago by gh-mjungmath

  • Cc nbruin added

comment:11 Changed 11 months ago by gh-mjungmath

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 follow-up: Changed 11 months ago by gh-mjungmath

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 mix-in class.

If one really needs the functionality on Cython level, what about a class SageObjectWithMutability instead?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 11 months ago by tscrim

Replying to gh-mjungmath:

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 mix-in 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 gh-mjungmath

Last edited 11 months ago by gh-mjungmath (previous) (diff)

comment:15 Changed 11 months ago by gh-mjungmath

Last edited 11 months ago by gh-mjungmath (previous) (diff)

comment:16 follow-up: Changed 11 months ago by gh-mjungmath

Last edited 11 months ago by gh-mjungmath (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 11 months ago by tscrim

Replying to gh-mjungmath:

Replying to tscrim:

Replying to gh-mjungmath:

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 mix-in 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 another cdef class, which is actually worse because a cdef 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 from SageObject, either directly or within an inheritance tree. In order to use Mutability, we need therefore two inheritances since Mutability does not inherit from SageObject. But a second inheritance is not allowed if the subsequent class is a Cython class as well. Having class Sequence(SageObject, Mutability) on the other hand is allowed but only if Mutability 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 if Mutability 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 speed-up. 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 from SageObject implementing the mutability feature. That would be useful for Sequence, BundleConnection, AffineConnection and probably even more.

Strong -1. See above.

comment:18 Changed 11 months ago by gh-mjungmath

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

Last edited 11 months ago by gh-mjungmath (previous) (diff)

comment:19 Changed 11 months ago by gh-mjungmath

@Travis: What about that?

    def __getstate__(self):
        state = self.__dict__
        state['_is_immutable'] = self._is_immutable
        return state

Works perfectly.

comment:20 follow-up: Changed 11 months ago by git

  • Commit changed from 439d0d0a1cb9cd0af35f1ee33962f8c6f26a49ce to 1c5d47d8c9f1ccca7bafcd45eb6f67d4f71c64db

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

1c5d47dTrac #31182: __getstate__ and __setstate__

comment:21 in reply to: ↑ 20 Changed 11 months ago by gh-mjungmath

Replying to git:

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

1c5d47dTrac #31182: __getstate__ and __setstate__

Here we go. Please check this out. Works perfectly with #30284.

comment:22 Changed 11 months ago by gh-mjungmath

  • Dependencies set to #31196

comment:23 Changed 11 months ago by git

  • Commit changed from 1c5d47d8c9f1ccca7bafcd45eb6f67d4f71c64db to d6d6ba416ad27f586ffbe2137c7a1867f1a78130

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

5ecbf3aTrac #31181: return added
4c33935Trac #31196: minor code improvements, py3 compatibility, documentation improved
e5228d3Trac #31196: cpdef require methods + example added
d957f73Trac #31196: unnecessary line in docstring removed
d6d6ba4Trac #31182: __getstate__ and __setstate__

comment:24 Changed 11 months ago by gh-mjungmath

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 git

  • Commit changed from d6d6ba416ad27f586ffbe2137c7a1867f1a78130 to 6cbd1fd918ef9fdc73310b10e9a50e6c9bf4b7ed

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

6cbd1fdTrac #31182: doctests added for __setstate__ and __getstate__

comment:26 Changed 11 months ago by tscrim

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 gh-mjungmath

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/sage-devel/local/lib/python3.8/site-packages/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/michi/Projekte/sage-devel/local/lib/python3.8/site-packages/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 gh-mjungmath

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 gh-mjungmath

Patchbot is satisfied here, too. :)

comment:30 Changed 11 months ago by tscrim

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

  • Branch changed from u/gh-mjungmath/mutability_class_and_pickling to 6cbd1fd918ef9fdc73310b10e9a50e6c9bf4b7ed
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.