Opened 9 years ago

Closed 5 years ago

#10962 closed defect (fixed)

Set_PythonType objects are not picklable

Reported by: stumpc5 Owned by: was
Priority: major Milestone: sage-6.5
Component: pickling Keywords:
Cc: sage-combinat Merged in:
Authors: Vincent Delecroix, Jeroen Demeyer Reviewers: Jeroen Demeyer, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: f28f02a (Commits) Commit: f28f02aeb6008daa88a267dc31d31963664c17dc
Dependencies: Stopgaps:

Description (last modified by nthiery)

    sage: S = sage.structure.parent.Set_PythonType(tuple)
    sage: dumps(S)
    ------------------------------------------------------------
    Traceback (most recent call last):
      File "<ipython console>", line 1, in <module>
      File "sage_object.pyx", line 842, in sage.structure.sage_object.dumps (sage/structure/sage_object.c:8274)
      File "sage_object.pyx", line 217, in sage.structure.sage_object.SageObject.dumps (sage/structure/sage_object.c:2183)
    PicklingError: Can't pickle <type 'function'>: attribute lookup __builtin__.function failed

Attachments (1)

trac_10962-pickle_set_pythontype.2.patch (5.7 KB) - added by vdelecroix 7 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 9 years ago by nthiery

  • Cc sage-combinat added
  • Component changed from combinatorics to pickling
  • Description modified (diff)
  • Owner changed from sage-combinat to was
  • Summary changed from Problem with pickle in CombinatorialFreeModule to Set_PythonType objects are not picklable

comment:2 Changed 7 years ago by vdelecroix

I implement a __reduce__ method for Set_PythonType and now it works...

Best, Vincent

comment:3 Changed 7 years ago by vdelecroix

  • Status changed from new to needs_review

Changed 7 years ago by vdelecroix

comment:4 Changed 7 years ago by vdelecroix

I undo a change which produces error (because of some metaclass buisness). I also add a method is_finite in relation with the cardinality one.

apply only: trac_10962-pickle_set_pythontype.2.patch

comment:5 Changed 7 years ago by vdelecroix

Hi,

I added a relatively fast check at the very begining of the init method that verifies that the argument is a type or not (using the function PyType_Check from the C-API. It is the cause of many errors (see the log of the patchbot) and I do not know whether it is a good thing to check...

See the following sage-devel thread.

comment:6 Changed 5 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/10962
  • Commit set to 18b968d45aa8649698eae4e5e70a88f9964e0e0d

New commits:

18b968dtrac #10962: make Set_PythonType pickable

comment:7 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This is wrong, it should be EXAMPLES::

EXAMPLES::

Let us create a parent in the category of rings::

comment:8 Changed 5 years ago by git

  • Commit changed from 18b968d45aa8649698eae4e5e70a88f9964e0e0d to 97f39849ff27767c6a99608d3a29db9b5738ae01

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

97f3984trac #10962: fix documentation

comment:9 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:10 Changed 5 years ago by vdelecroix

I got a strange error from the pickle jar. Could anyone perform the test?

sage -t --long structure/sage_object.pyx
**********************************************************************
File "structure/sage_object.pyx", line 1431, in sage.structure.sage_object.unpickle_all
Failed example:
    sage.structure.sage_object.unpickle_all()  # (4s on sage.math, 2011)
Expected:
    doctest:... DeprecationWarning: ...
    See http://trac.sagemath.org/... for details.
    Successfully unpickled ... objects.
    Failed to unpickle 0 objects.
Got:
    doctest:1: DeprecationWarning: OrderedAlphabet is deprecated; use Alphabet instead.
    See http://trac.sagemath.org/8920 for details.
    doctest:850: DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double.
    See http://trac.sagemath.org/4260 for details.
     * unpickle failure: load('/home/vincent/.sage/temp/mangouste/23352/dir_D94KcT//pickle_jar/_class__sage_groups_matrix_gps_matrix_group_element_MatrixGroupElement__.sobj')
    doctest:1: DeprecationWarning: gen_py.pari is deprecated, use sage.libs.pari.all.pari instead
    See http://trac.sagemath.org/17451 for details.
    doctest:632: DeprecationWarning: The "pari_mod" finite field implementation is deprecated
    See http://trac.sagemath.org/17297 for details.
    Failed:
    _class__sage_groups_matrix_gps_matrix_group_element_MatrixGroupElement__.sobj
    ----------------------------------------------------------------------
    ** This error is probably due to an old pickle failing to unpickle.
    ** See sage.structure.sage_object.register_unpickle_override for
    ** how to override the default unpickling methods for (old) pickles.
    ** NOTE: pickles should never be removed from the pickle_jar! 
    ----------------------------------------------------------------------
    Successfully unpickled 585 objects.
    Failed to unpickle 1 objects.
**********************************************************************
File "structure/sage_object.pyx", line 1439, in sage.structure.sage_object.unpickle_all
Failed example:
    sage.structure.sage_object.unpickle_all()
Expected:
    Successfully unpickled ... objects.
    Failed to unpickle 0 objects.
Got:
     * unpickle failure: load('/home/vincent/.sage/temp/mangouste/23352/dir_VbqHGc//pickle_jar/_class__sage_groups_matrix_gps_matrix_group_element_MatrixGroupElement__.sobj')
    Failed:
    _class__sage_groups_matrix_gps_matrix_group_element_MatrixGroupElement__.sobj
    ----------------------------------------------------------------------
    ** This error is probably due to an old pickle failing to unpickle.
    ** See sage.structure.sage_object.register_unpickle_override for
    ** how to override the default unpickling methods for (old) pickles.
    ** NOTE: pickles should never be removed from the pickle_jar! 
    ----------------------------------------------------------------------
    Successfully unpickled 585 objects.
    Failed to unpickle 1 objects.
**********************************************************************

comment:11 Changed 5 years ago by jdemeyer

  • Branch changed from u/vdelecroix/10962 to u/jdemeyer/ticket/10962
  • Modified changed from 12/19/14 17:46:14 to 12/19/14 17:46:14

comment:12 Changed 5 years ago by jdemeyer

  • Commit changed from 97f39849ff27767c6a99608d3a29db9b5738ae01 to 2908e5797bb558895695abd5443ad718cc14d5fb

I confirm the unpickling error, it is due to

TypeError                                 Traceback (most recent call last)
<ipython-input-3-d62ad2672fb4> in <module>()
----> 1 load('/home/jdemeyer/.sage/temp/tamiyo/6215/dir_LJVs2X//pickle_jar/_class__sage_groups_matrix_gps_matrix_group_element_MatrixGroupElement__.sobj')

/usr/local/src/sage-config/src/sage/structure/sage_object.pyx in sage.structure.sage_object.load (build/cythonized/sage/structure/sage_object.c:11233)()
    945 
    946     ## Load file by absolute filename
--> 947     X = loads(open(filename).read(), compress=compress)
    948     try:
    949         X._default_filename = os.path.abspath(filename)

/usr/local/src/sage-config/src/sage/structure/sage_object.pyx in sage.structure.sage_object.loads (build/cythonized/sage/structure/sage_object.c:13238)()
   1319     unpickler.find_global = unpickle_global
   1320 
-> 1321     return unpickler.load()
   1322 
   1323 

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/groups/matrix_gps/pickling_overrides.pyc in __setstate__(self, state)
     36         degree = state['_MatrixGroup_gap__n']
     37         from sage.libs.all import libgap
---> 38         libgap_group = libgap.Group(libgap(matrix_gens))
     39         self.__init__(degree, ring, libgap_group)
     40 

/usr/local/src/sage-config/src/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9585)()
   1102             mor = <map.Map> self._convert_from_hash.get(R)
   1103         except KeyError:
-> 1104             mor = <map.Map> self._internal_convert_map_from(R)
   1105 
   1106         if mor is not None:

/usr/local/src/sage-config/src/sage/structure/parent.pyx in sage.structure.parent.Parent._internal_convert_map_from (build/cythonized/sage/structure/parent.c:19460)()
   2581             return self._convert_from_hash.get(S)
   2582         except KeyError:
-> 2583             mor = self.discover_convert_map_from(S)
   2584             # Before trac #14711, the morphism has been
   2585             # put both into _convert_from_list and into

/usr/local/src/sage-config/src/sage/structure/parent.pyx in sage.structure.parent.Parent.discover_convert_map_from (build/cythonized/sage/structure/parent.c:20064)()
   2624             from sage.structure.coerce_maps import ListMorphism
   2625             if isinstance(S, Sequences):
-> 2626                 return ListMorphism(S, self.convert_map_from(list))
   2627 
   2628         mor = self._generic_convert_map(S)

/usr/local/src/sage-config/src/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.ListMorphism.__init__ (build/cythonized/sage/structure/coerce_maps.c:8911)()
    529     def __init__(self, domain, Map real_morphism):
    530         if not PY_TYPE_CHECK(domain, Parent):
--> 531             domain = Set_PythonType(domain)
    532         Map.__init__(self, domain, real_morphism.codomain())
    533         self._coerce_cost = real_morphism._coerce_cost + 3

/usr/local/src/sage-config/src/sage/structure/parent.pyx in sage.structure.parent.Set_PythonType (build/cythonized/sage/structure/parent.c:24290)()
   2990 cdef _type_set_cache = {}
   2991 
-> 2992 cpdef Parent Set_PythonType(theType):
   2993     """
   2994     Return the (unique) Parent that represents the set of Python objects

/usr/local/src/sage-config/src/sage/structure/parent.pyx in sage.structure.parent.Set_PythonType (build/cythonized/sage/structure/parent.c:24199)()
   3021         return _type_set_cache[theType]
   3022     except KeyError:
-> 3023         _type_set_cache[theType] = theSet = Set_PythonType_class(theType)
   3024         return theSet
   3025 

/usr/local/src/sage-config/src/sage/structure/parent.pyx in sage.structure.parent.Set_PythonType_class.__init__ (build/cythonized/sage/structure/parent.c:24417)()
   3055         """
   3056         if not isinstance(theType, type):
-> 3057             raise TypeError("must be intialized with a type, not %r" % theType)
   3058         Set_generic.__init__(self, element_constructor=theType, category=Sets())
   3059         self._type = theType

TypeError: must be intialized with a type, not Category of sequences in Full MatrixSpace of 2 by 2 dense matrices over Finite Field of size 3

New commits:

2908e57Remove useless C/API calls

comment:13 follow-up: Changed 5 years ago by jdemeyer

Question: would it be valid to use Set_PythonType() on a Category instead of a type?

comment:14 Changed 5 years ago by jdemeyer

  • Milestone set to sage-6.5
  • Status changed from needs_review to needs_work

comment:15 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by vdelecroix

Replying to jdemeyer:

Question: would it be valid to use Set_PythonType() on a Category instead of a type?

No. By the way I do not understand why Posets and Sequences are categories and not Parent...

comment:16 Changed 5 years ago by vdelecroix

  • Branch changed from u/jdemeyer/ticket/10962 to u/vdelecroix/10962
  • Commit changed from 2908e5797bb558895695abd5443ad718cc14d5fb to da81792004986e23a3dee4c92b0cc229b64741f2
  • Status changed from needs_work to needs_review

New commits:

da81792trac #10962: fix unpickle (Sequences is not a parent)

comment:17 Changed 5 years ago by jdemeyer

  • Branch changed from u/vdelecroix/10962 to u/jdemeyer/ticket/10962
  • Modified changed from 12/20/14 07:26:22 to 12/20/14 07:26:22

comment:18 Changed 5 years ago by jdemeyer

  • Commit changed from da81792004986e23a3dee4c92b0cc229b64741f2 to 4b5c3732a910b99b8f43b5b4eb0da13c8f1c7f27
  • Reviewers set to Jeroen Demeyer

I added one extra commit to simplify the code and remove a compiler warning for parent.pyx (which is not your fault, but which should be fixed anyway).


New commits:

4b5c373Remove useless C/API calls; improve error message

comment:19 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work
sage -t --long src/sage/symbolic/maxima_wrapper.py
**********************************************************************
File "src/sage/symbolic/maxima_wrapper.py", line 113, in sage.symbolic.maxima_wrapper.MaximaWrapper._symbolic_
Failed example:
    SR(u) is t # indirect doctest
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 850, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.symbolic.maxima_wrapper.MaximaWrapper._symbolic_[2]>", line 1, in <module>
        SR(u) is t # indirect doctest
      File "sage/structure/parent.pyx", line 1104, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9585)
        mor = <map.Map> self._internal_convert_map_from(R)
      File "sage/structure/parent.pyx", line 2583, in sage.structure.parent.Parent._internal_convert_map_from (build/cythonized/sage/structure/parent.c:19460)
        mor = self.discover_convert_map_from(S)
      File "sage/structure/parent.pyx", line 2632, in sage.structure.parent.Parent.discover_convert_map_from (build/cythonized/sage/structure/parent.c:20119)
        mor = self._generic_convert_map(S)
      File "sage/structure/parent_old.pyx", line 534, in sage.structure.parent_old.Parent._generic_convert_map (build/cythonized/sage/structure/parent_old.c:8211)
        return parent.Parent._generic_convert_map(self, S)
      File "sage/structure/parent.pyx", line 2142, in sage.structure.parent.Parent._generic_convert_map (build/cythonized/sage/structure/parent.c:16530)
        return coerce_maps.DefaultConvertMap_unique(S, self)
      File "sage/structure/coerce_maps.pyx", line 39, in sage.structure.coerce_maps.DefaultConvertMap.__init__ (build/cythonized/sage/structure/coerce_maps.c:2712)
        domain = Set_PythonType(domain)
      File "sage/structure/parent.pyx", line 2996, in sage.structure.parent.Set_PythonType (build/cythonized/sage/structure/parent.c:24328)
        cpdef Parent Set_PythonType(theType):
      File "sage/structure/parent.pyx", line 3027, in sage.structure.parent.Set_PythonType (build/cythonized/sage/structure/parent.c:24237)
        _type_set_cache[theType] = theSet = Set_PythonType_class(theType)
      File "sage/structure/parent.pyx", line 3061, in sage.structure.parent.Set_PythonType_class.__init__ (build/cythonized/sage/structure/parent.c:24455)
        raise TypeError("must be intialized with a type, not %r" % theType)
    TypeError: must be intialized with a type, not parent(log(sqrt(2) + 1) + log(sqrt(2) - 1))
**********************************************************************

comment:20 Changed 5 years ago by git

  • Commit changed from 4b5c3732a910b99b8f43b5b4eb0da13c8f1c7f27 to edf526e2a9b181c058620e1b42b1b378c37d99a7

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

edf526eDefine a parent() method for MaximaWrapper objects

comment:21 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:22 in reply to: ↑ 15 ; follow-up: Changed 5 years ago by nthiery

Replying to vdelecroix:

Replying to jdemeyer:

Question: would it be valid to use Set_PythonType() on a Category instead of a type?

No. By the way I do not understand why Posets and Sequences are categories and not Parent...

The category corresponding to Parent is Sets. Now, why is there both Parent and Sets? The latter is an abstract class, while the former is a concrete implementation (and currently the only concrete base implementation; but we could imagine having others). Of course, a lot of the current features of Parent really should be moved up in Sets or in other categories. Some just need to be taken care of (handling some backward compatibility issues). Some are more intrinsically difficult because of the lack of multiple inheritance in Cython; that's analog to the Ring versus Rings situation.

comment:24 in reply to: ↑ 22 ; follow-up: Changed 5 years ago by vdelecroix

Replying to nthiery:

Replying to vdelecroix:

Replying to jdemeyer:

Question: would it be valid to use Set_PythonType() on a Category instead of a type?

No. By the way I do not understand why Posets and Sequences are categories and not Parent...

The category corresponding to Parent is Sets. Now, why is there both Parent and Sets? The latter is an abstract class, while the former is a concrete implementation (and currently the only concrete base implementation; but we could imagine having others). Of course, a lot of the current features of Parent really should be moved up in Sets or in other categories. Some just need to be taken care of (handling some backward compatibility issues). Some are more intrinsically difficult because of the lack of multiple inheritance in Cython; that's analog to the Ring versus Rings situation.

Thanks Nicolas. But I do not understand why it is an answer to my question: "why Posets and Sequences are categories and not Parent?" As far as I understand, to declare something to have a category it needs to inherit from Parent (via Parent.__init__(category=XYZ)). So I do not see how to avoid Parent by "having others". The issue of Ring versus Rings has nothing to do with my question, or I missed something.

Vincent

comment:25 in reply to: ↑ 23 ; follow-ups: Changed 5 years ago by vdelecroix

  • Authors changed from Vincent Delecroix to Vincent Delecroix, Jeroen Demeyer
  • Status changed from needs_review to needs_info

Replying to jdemeyer:

This finally passes all doctests. We just need somebody to review my last two commits:

I am fine with the first one. But the result of .parent() should be a parent isn't it? Why not

def parent(self):
    return Set_PythonType(type(self))

On the other hand, the MaximaWrapper does not inherit from Element which is perfectly fine. I am worried that we need this method. The bug for me is the following behavior

sage: parent(sin(1).maxima_methods())
parent(sin(1))

which should just behave like

sage: parent(int(1))
<type 'int'>

(which is also strange but perfectly fine if uniform).

The reason is that there is a too permissive __getattr__ in MaximaWrapper which never raises AttributeError. I would really prefer that:

  • either we move your solution at the level of SageObject
  • or we hack __getattr__ of MaximaWrapper

Vincent

comment:26 in reply to: ↑ 24 ; follow-up: Changed 5 years ago by nthiery

Replying to vdelecroix:

Replying to nthiery:

Replying to vdelecroix:

Replying to jdemeyer:

Question: would it be valid to use Set_PythonType() on a Category instead of a type?

No. By the way I do not understand why Posets and Sequences are categories and not Parent...

The category corresponding to Parent is Sets. Now, why is there both Parent and Sets? The latter is an abstract class, while the former is a concrete implementation (and currently the only concrete base implementation; but we could imagine having others). Of course, a lot of the current features of Parent really should be moved up in Sets or in other categories. Some just need to be taken care of (handling some backward compatibility issues). Some are more intrinsically difficult because of the lack of multiple inheritance in Cython; that's analog to the Ring versus Rings situation.

Thanks Nicolas. But I do not understand why it is an answer to my question: "why Posets and Sequences are categories and not Parent?" As far as I understand, to declare something to have a category it needs to inherit from Parent (via Parent.__init__(category=XYZ)). So I do not see how to avoid Parent by "having others". The issue of Ring versus Rings has nothing to do with my question, or I missed something.

Hmm I am not sure I understand your question then. Can you rephrase it?

Just a quick note for now: to have a category, you need to be a CategoryObject? (surprise surprise ...).

comment:27 in reply to: ↑ 25 Changed 5 years ago by jdemeyer

Replying to vdelecroix:

But the result of .parent() should be a parent isn't it?

It doesn't have to be. For things which don't have a parent, it's accepted to return a type instead. After all, the global parent(x) function returns type(x) if x has no parent.

comment:28 Changed 5 years ago by git

  • Commit changed from edf526e2a9b181c058620e1b42b1b378c37d99a7 to f28f02aeb6008daa88a267dc31d31963664c17dc

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

f28f02aDefine parent() to be type() for SageObject

comment:29 in reply to: ↑ 25 Changed 5 years ago by jdemeyer

  • Status changed from needs_info to needs_review

Replying to vdelecroix:

  • either we move your solution at the level of SageObject

Done! This commit replaces the last one.

comment:30 in reply to: ↑ 26 Changed 5 years ago by vdelecroix

Replying to nthiery:

Hmm I am not sure I understand your question then. Can you rephrase it?

Why Posets and Sequences do not inherit from CategoryObject?

Vincent

comment:31 Changed 5 years ago by vdelecroix

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Vincent Delecroix
  • Status changed from needs_review to positive_review

All right. Thanks.

comment:32 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/10962 to f28f02aeb6008daa88a267dc31d31963664c17dc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.