jdemeyer commented 7 years ago

Tarball: https://www.python.org/ftp/python/2.7.13/Python-2.7.13.tgz

Note: upgrade to Python 2.7.12 done in #19735.

CC: @simon-king-jena @hivert @slel @EmmanuelCharpentier

Component: packages: standard

Author: Jeroen Demeyer

Branch/Commit: 5aeaf5c

Reviewer: Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/22037

tobihan commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1,5 @@
+Python 2.7.13rc1 leads to problems with sage related to __new__. It is likely related to this change that was included in Python:
+https://bugs.python.org/issue5322

+See the following discussion on sage-devel:
+https://groups.google.com/forum/#!topic/sage-devel/IbugChsM26E

tobihan commented 7 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-Python 2.7.13rc1 leads to problems with sage related to __new__. It is likely related to this change that was included in Python:
+Python 2.7.13rc1 leads to problems with sage related to `__new__`. It is likely related to this change that was included in Python:
 https://bugs.python.org/issue5322

 See the following discussion on sage-devel:

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -3,3 +3,5 @@

 See the following discussion on sage-devel:
 https://groups.google.com/forum/#!topic/sage-devel/IbugChsM26E
+
+But first we should upgrade to Python 2.7.12: #19735

tobihan commented 7 years ago

Attachment: example.tar.gz

This is a small example showing how the ClasscallMetaclass leads to the error.

tobihan commented 7 years ago

comment:4

So we have

class UniqueRepresentation(CachedRepresentation, WithEqualityById)

where CachedRepresentation uses ClasscallMetaclass and WithEqualityById is a cdef class.

If I change that to

class UniqueRepresentation(WithEqualityById, CachedRepresentation)

the error goes away. I don't know if that would be a correct fix though.

nthiery commented 7 years ago

comment:5

Replying to @tobihan:

So we have

class UniqueRepresentation(CachedRepresentation, WithEqualityById)

where CachedRepresentation uses ClasscallMetaclass and WithEqualityById is a cdef class.

If I change that to

class UniqueRepresentation(WithEqualityById, CachedRepresentation)

the error goes away. I don't know if that would be a correct fix though.

In principle, the features provided by the two super classes are orthogonal (equality vs construction). Right now, I don't remember enough the details to see whether there could be subtleties due to one class being cdef. I'll dig further.

Simon, any opinion?

Cheers, Nicolas

nthiery commented 7 years ago

Description changed:

--- 
+++ 
@@ -5,3 +5,5 @@
 https://groups.google.com/forum/#!topic/sage-devel/IbugChsM26E

 But first we should upgrade to Python 2.7.12: #19735
+
+See also https://bugs.python.org/issue25731, where a patch at the same location was reverted as backward incompatible after causing a failure in Sage.

tobihan commented 7 years ago

comment:8

Indeed Juliens example from https://bugs.python.org/issue25731 also exposes this issue:

$ cat foo.pxd 
cdef class B:
    cdef object b
$ cat foo.pyx 
cdef class A:
    pass

cdef class B:
    def __init__(self, b):
        self.b = b
$ cat bar.py
from foo import A, B

class C(A, B):
    def __init__(self):
        B.__init__(self, 1)

C()
$ cython foo.pyx && gcc -I/usr/include/python2.7 -Wall -shared -fPIC -o foo.so foo.c
$ python -c 'import bar'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "bar.py", line 7, in <module>
    C()
TypeError: foo.A.__new__(C) is not safe, use foo.B.__new__()

nthiery commented 7 years ago

comment:9

Replying to @tobihan:

So we have

class UniqueRepresentation(CachedRepresentation, WithEqualityById)

where CachedRepresentation uses ClasscallMetaclass and WithEqualityById is a cdef class.

If I change that to

class UniqueRepresentation(WithEqualityById, CachedRepresentation)

the error goes away. I don't know if that would be a correct fix though.

Looking back at the code, this still feels rather harmless.

I ran all long Sage tests; beside trivial doctests failures due to mro printouts, the only errors all boil down to the following:

sage -t --long src/sage/combinat/root_system/cartan_type.py
**********************************************************************
File "src/sage/combinat/root_system/cartan_type.py", line 2999, in sage.combinat.root_system.cartan_type.CartanType_simple_finite.__setstate__
Failed example:
    unpickle_build(si1, {'tools':pg_unpickleModule('sage.combinat.root_system.type_A'), 't':['A', si2], 'letter':'A', 'n':si2})
Exception raised:
    Traceback (most recent call last):
      File "/opt/sage-git2/local/lib/python2.7/site-packages/sage/combinat/root_system/cartan_type.py", line 3014, in __setstate__
        self.__class__ = T.__class__
    TypeError: __class__ assignment: 'CartanType_simple_finite' object layout differs from 'CartanType'

This is in a stub class whose only purpose is to support unpickling rather old pickles (Sage <= 4.0). It's a stub for an old class that does not exist anymore; upon unpickling an object, the set_state method substitutes its stub class by the appropriate new class. This is unsurprisingly fragile. I am unsure why it did not fail before and fails now.

In any cases, making the stub class derive from UniqueRepresentation ensures that the two classes have the same layout, which is slightly cleaner than it was earlier. So that sounds like an acceptable fix.

I am about to push a branch with this fix and trivially updated doctests where needed. Let's see if the patchbot confirms that all test pass.

Still would be happy to have feedback from Simon.

Cheers, Nicolas

nthiery commented 7 years ago

Branch: u/nthiery/upgrade_to_python_2_7_13

nthiery commented 7 years ago

comment:11

Note: not being super familiar with the process, the above branch does not do the upgrade to Python 2.7.13; just the workaround suggested by Tobbias.

New commits:

`8968314`	`22037: switch order of the bases of the class UniqueRepresentation`
`b7bd459`	`22037: more robust backward-compatibility stub class CartanType_simple_finite`

nthiery commented 7 years ago

Commit: b7bd459

tobihan commented 7 years ago

comment:12

There are several places that need to be changed. The problem is that the errors are discovered one by one. Is there a standard way to rebuild just a single .pyx file? Right now I have to do a rebuild of sagelib everytime I change one. This is my patch right now, and there are still new errors coming up:

--- a/src/sage/structure/unique_representation.py
+++ b/src/sage/structure/unique_representation.py
@@ -1176,7 +1176,7 @@
     return cls(*args, **keywords)

-class UniqueRepresentation(CachedRepresentation, WithEqualityById):
+class UniqueRepresentation(WithEqualityById, CachedRepresentation):
     r"""
     Classes derived from UniqueRepresentation inherit a unique
     representation behavior for their instances.
--- a/src/sage/categories/cartesian_product.py
+++ b/src/sage/categories/cartesian_product.py
@@ -18,7 +18,7 @@

 native_python_containers   = set([tuple, list, set, frozenset])

-class CartesianProductFunctor(CovariantFunctorialConstruction, MultivariateConstructionFunctor):
+class CartesianProductFunctor(MultivariateConstructionFunctor, CovariantFunctorialConstruction):
     """
     A singleton class for the Cartesian product functor.

--- a/src/sage/rings/infinity.py
+++ b/src/sage/rings/infinity.py
@@ -544,7 +544,7 @@

 class UnsignedInfinityRing_class(Singleton, Ring):
-
+    __new__ = Ring.__new__
     def __init__(self):
         """
         Initialize ``self``.
@@ -950,6 +950,7 @@
     pass

 class InfinityRing_class(Singleton, Ring):
+    __new__ = Ring.__new__
     def __init__(self):
         """
         Initialize ``self``.
--- a/src/sage/structure/formal_sum.py
+++ b/src/sage/structure/formal_sum.py
@@ -314,7 +314,7 @@
             w.append((coeff,last))
         self._data = w

-class FormalSums(UniqueRepresentation, Module):
+class FormalSums(Module, UniqueRepresentation):
     """
     The R-module of finite formal sums with coefficients in some ring R.

--- a/src/sage/misc/inherit_comparison.pyx
+++ b/src/sage/misc/inherit_comparison.pyx
@@ -83,5 +83,5 @@
                 t.tp_compare = b.tp_compare
         super(InheritComparisonMetaclass, self).__init__(*args)

-class InheritComparisonClasscallMetaclass(InheritComparisonMetaclass, ClasscallMetaclass):
+class InheritComparisonClasscallMetaclass(ClasscallMetaclass, InheritComparisonMetaclass):
     pass
--- a/src/sage/rings/polynomial/polynomial_ring.py
+++ b/src/sage/rings/polynomial/polynomial_ring.py
@@ -1439,7 +1439,7 @@
             return self._monics_max( max_degree )
         raise ValueError("you should pass exactly one of of_degree and max_degree")

-class PolynomialRing_commutative(PolynomialRing_general, commutative_algebra.CommutativeAlgebra):
+class PolynomialRing_commutative(commutative_algebra.CommutativeAlgebra, PolynomialRing_general):
     """
     Univariate polynomial ring over a commutative ring.
     """
--- a/src/sage/rings/real_arb.pyx
+++ b/src/sage/rings/real_arb.pyx
@@ -355,7 +355,7 @@
     if _do_sig(myprec): sig_off()
     return 0

-class RealBallField(UniqueRepresentation, Field):
+class RealBallField(Field, UniqueRepresentation):
     r"""
     An approximation of the field of real numbers using mid-rad intervals, also
     known as balls.

nthiery commented 7 years ago

comment:13

Ouch, this is getting more annoying. Let's see how big the list grows. It's more worrying to have to move UniqueRepresentation to second place.

sage -b rebuilds just the Sage library. This is usually good enough when only a single pyx file needs to be rebuilt.

Have a good night,

tobihan commented 7 years ago

comment:14

Is better not to change the order of base classes and instead just define __new__ everywhere like I did for example for InfinityRing_class?

nthiery commented 7 years ago

comment:15

Ah, I had missed that. If this works, this would be acceptable as a temporary workaround. My first guess is that this would break the CachedRepresentation; but I'll give it a try.

Anyone available for making a branch with Python 2.7.13, so that we can all easilly experiment with it? Otherwise, I'll try to squeeze this sometime today, but really no promise.

Thanks Tobias for investigating this hard!

slel commented 7 years ago

Description changed:

--- 
+++ 
@@ -4,6 +4,7 @@
 See the following discussion on sage-devel:
 https://groups.google.com/forum/#!topic/sage-devel/IbugChsM26E

-But first we should upgrade to Python 2.7.12: #19735
+See also https://bugs.python.org/issue25731, where a patch at the same location was reverted as backward incompatible after causing a failure in Sage.

-See also https://bugs.python.org/issue25731, where a patch at the same location was reverted as backward incompatible after causing a failure in Sage.
+Note: upgrade to Python 2.7.12 done in #19735.
+

slel commented 7 years ago

comment:17

19735 "Upgrade to Python 2.7.12" was just closed.

jdemeyer commented 7 years ago

comment:18

Replying to @nthiery:

sage -b rebuilds just the Sage library. This is usually good enough when only a single pyx file needs to be rebuilt.

Although it should be said that sage -b should be used only if you "know what you are doing". It's safer to run make sagelib (build Sage library) or make build (build all Sage packages) or plain make (build Sage + documentation). The make commands have dependency checking, so an outdated library will be rebuilt if needed. When you only run sage -b, you might get into trouble when switching branches because some dependencies might not be updated.

tobihan commented 7 years ago

comment:19

Thanks. I'm using Debian packages for the dependencies so I'm just rebuilding sagelib anyway. I was asking if it was possible to rebuild just a single .pyx file.

Meanwhile the patch is getting bigger and bigger. I think I need to add __new__ = Parent.__new__ to all classes that have the base classes "Uniquerepresentation, Parent", which are many. And others. At least now sage starts and one can run the tests to uncover more errors. Should I keep going on with this?

--- a/src/sage/structure/unique_representation.py
+++ b/src/sage/structure/unique_representation.py
@@ -1342,3 +1342,4 @@
         ....:
         sage: b = bla()
     """
+    __new__ = object.__new__
--- a/src/sage/categories/cartesian_product.py
+++ b/src/sage/categories/cartesian_product.py
@@ -105,6 +105,7 @@
     :class:`CartesianProductsCategory`.

     """
+    __new__ = MultivariateConstructionFunctor.__new__
     _functor_name = "cartesian_product"
     _functor_category = "CartesianProducts"
     symbol = " (+) "
--- a/src/sage/rings/infinity.py
+++ b/src/sage/rings/infinity.py
@@ -544,7 +544,7 @@

 class UnsignedInfinityRing_class(Singleton, Ring):
-
+    __new__ = Ring.__new__
     def __init__(self):
         """
         Initialize ``self``.
@@ -950,6 +950,7 @@
     pass

 class InfinityRing_class(Singleton, Ring):
+    __new__ = Ring.__new__
     def __init__(self):
         """
         Initialize ``self``.
--- a/src/sage/structure/formal_sum.py
+++ b/src/sage/structure/formal_sum.py
@@ -336,6 +336,7 @@
         sage: TestSuite(FormalSums(QQ)).run()

     """
+    __new__ = Module.__new__
     Element = FormalSum
     @staticmethod
     def __classcall__(cls, base_ring = ZZ):
--- a/src/sage/misc/inherit_comparison.pyx
+++ b/src/sage/misc/inherit_comparison.pyx
@@ -84,4 +84,5 @@
         super(InheritComparisonMetaclass, self).__init__(*args)

 class InheritComparisonClasscallMetaclass(InheritComparisonMetaclass, ClasscallMetaclass):
+    __new__ = ClasscallMetaclass.__new__
     pass
--- a/src/sage/rings/polynomial/polynomial_ring.py
+++ b/src/sage/rings/polynomial/polynomial_ring.py
@@ -1443,6 +1443,7 @@
     """
     Univariate polynomial ring over a commutative ring.
     """
+    __new__ = commutative_algebra.CommutativeAlgebra.__new__
     def __init__(self, base_ring, name=None, sparse=False, element_class=None, category=None):
         if base_ring not in _CommutativeRings:
             raise TypeError("Base ring %s must be a commutative ring."%repr(base_ring))
--- a/src/sage/rings/real_arb.pyx
+++ b/src/sage/rings/real_arb.pyx
@@ -398,6 +398,7 @@
         sage: RBF.zero()
         0
     """
+    __new__ = Field.__new__
     Element = RealBall

     @staticmethod
--- a/src/sage/structure/dynamic_class.py
+++ b/src/sage/structure/dynamic_class.py
@@ -460,6 +460,7 @@
     pass

 class DynamicInheritComparisonMetaclass(DynamicMetaclass, InheritComparisonMetaclass):
+    __new__ = type.__new__
     pass

 class DynamicInheritComparisonClasscallMetaclass(DynamicMetaclass, InheritComparisonClasscallMetaclass):
--- a/src/sage/rings/complex_arb.pyx
+++ b/src/sage/rings/complex_arb.pyx
@@ -234,6 +234,7 @@
         ...
         ValueError: Precision must be at least 2.
     """
+    __new__ = Field.__new__
     Element = ComplexBall

     @staticmethod
--- a/src/sage/rings/pari_ring.py
+++ b/src/sage/rings/pari_ring.py
@@ -165,6 +165,7 @@
         sage: loads(R.dumps()) is R
         True
     """
+    __new__ = ring.Ring.__new__
     Element = Pari

     def __init__(self):
--- a/src/sage/rings/contfrac.py
+++ b/src/sage/rings/contfrac.py
@@ -78,6 +78,7 @@
         sage: CFF.category()
         Category of fields
     """
+    __new__ = Field.__new__
     class Element(ContinuedFraction_periodic,FieldElement):
         r"""
         A continued fraction of a rational number.
--- a/src/sage/rings/number_field/number_field.py
+++ b/src/sage/rings/number_field/number_field.py
@@ -1264,6 +1264,7 @@
         ....:         assert elt.is_integral()

     """
+    __new__ = number_field_base.NumberField.__new__
     def __init__(self, polynomial, name, latex_name,
                  check=True, embedding=None, category=None,
                  assume_disc_small=False, maximize_at_primes=None, structure=None):
--- a/src/sage/matrix/matrix_space.py
+++ b/src/sage/matrix/matrix_space.py
@@ -137,6 +137,7 @@
         ...
         ValueError: number of rows and columns may be at most...
     """
+    __new__ = parent_gens.ParentWithGens.__new__
     _no_generic_basering_coercion = True

     @staticmethod
--- a/src/sage/schemes/generic/scheme.py
+++ b/src/sage/schemes/generic/scheme.py
@@ -791,6 +791,7 @@
         :class:`sage.schemes.generic.algebraic_scheme.AffineSpace`.

     """
+    __new__ = Scheme.__new__
     def __init__(self, R, S=None, category=None):
         """
         Construct the affine scheme with coordinate ring `R`.
--- a/src/sage/combinat/partition.py
+++ b/src/sage/combinat/partition.py
@@ -5061,6 +5061,7 @@
         sage: P.list()
         [[3, 2]]
     """
+    __new__ = Parent.__new__
     @staticmethod
     def __classcall_private__(cls, n=None, **kwargs):
         """
--- a/src/sage/sets/non_negative_integers.py
+++ b/src/sage/sets/non_negative_integers.py
@@ -67,7 +67,7 @@
     TODO: do not use ``NN`` any more in the doctests for
     ``NonNegativeIntegers``.
     """
-
+    __new__ = Parent.__new__
     def __init__(self, category=None):
         """
         TESTS::
--- a/src/sage/structure/sequence.py
+++ b/src/sage/structure/sequence.py
@@ -410,6 +410,7 @@
         Finite Field of size 5

     """
+    __new__ = list.__new__
     def __init__(self, x, universe=None, check=True, immutable=False,
                  cr=False, cr_str=None, use_sage_types=False):
         """
--- a/src/sage/structure/list_clone_demo.pyx
+++ b/src/sage/structure/list_clone_demo.pyx
@@ -61,7 +61,7 @@
         sage: IncreasingArrays().element_class
         <type 'sage.structure.list_clone_demo.IncreasingArray'>
     """
-
+    __new__ = Parent.__new__
     def __init__(self):
         """
         TESTS::
--- a/src/sage/sets/finite_enumerated_set.py
+++ b/src/sage/sets/finite_enumerated_set.py
@@ -80,7 +80,7 @@
         sage: S.first().parent()
         Integer Ring
     """
-
+    __new__ = Parent.__new__
     @staticmethod
     def __classcall__(cls, iterable):
         """
--- a/src/sage/combinat/derangements.py
+++ b/src/sage/combinat/derangements.py
@@ -130,6 +130,7 @@
         sage: D2.random_element() # random
         [2, 3, 1, 3, 1, 2]
     """
+    __new__ = Parent.__new__
     @staticmethod
     def __classcall_private__(cls, x):
         """
--- a/src/sage/combinat/permutation.py
+++ b/src/sage/combinat/permutation.py
@@ -5003,6 +5003,7 @@
         sage: p.random_element()
         [5, 1, 2, 4, 3]
     """
+    __new__ = Parent.__new__
     @staticmethod
     def __classcall_private__(cls, n=None, k=None, **kwargs):
         """
--- a/src/sage/categories/examples/infinite_enumerated_sets.py
+++ b/src/sage/categories/examples/infinite_enumerated_sets.py
@@ -73,7 +73,7 @@
         running ._test_pickling() . . . pass
         running ._test_some_elements() . . . pass
     """
-
+    __new__ = Parent.__new__
     def __init__(self):
         """
         TESTS::
--- a/src/sage/categories/examples/sets_with_grading.py
+++ b/src/sage/categories/examples/sets_with_grading.py
@@ -23,6 +23,7 @@
         sage: E.graded_component(100)
         {100}
     """
+    __new__ = Parent.__new__
     def __init__(self):
         r"""
         TESTS::
--- a/src/sage/sets/cartesian_product.py
+++ b/src/sage/sets/cartesian_product.py
@@ -53,6 +53,7 @@

     .. automethod:: _cartesian_product_of_elements
     """
+    __new__ = Parent.__new__
     def __init__(self, sets, category, flatten=False):
         r"""
         INPUT:

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -6,5 +6,6 @@

 See also https://bugs.python.org/issue25731, where a patch at the same location was reverted as backward incompatible after causing a failure in Sage.

+**Tarball**: https://www.python.org/ftp/python/2.7.13/Python-2.7.13rc1.tgz
+
 Note: upgrade to Python 2.7.12 done in #19735.
-

jdemeyer commented 7 years ago

Changed branch from u/nthiery/upgrade_to_python_2_7_13 to u/jdemeyer/upgrade_to_python_2_7_13

jdemeyer commented 7 years ago

comment:22

This branch now contains both the upgrade to Python 2.7.13rc1 as well as the commits by Nicolas.

New commits:

`c35ea3c`	`Upgrade python2 to 2.7.12 and update tinfo and uuid patches`
`85e931b`	`fix sage_timeit and word.py for python 2.7.12`
`a119bf7`	`Upgrade to Python 2.7.13rc1`
`a735fa5`	`22037: switch order of the bases of the class UniqueRepresentation`
`ba486e1`	`22037: more robust backward-compatibility stub class CartanType_simple_finite`

jdemeyer commented 7 years ago

Changed commit from b7bd459 to ba486e1

tobihan commented 7 years ago

comment:23

I now have a patch that changes more than 100 files and that's still not all. I don't think fixing this case-by-case in this way is viable. In any case this would be just a temporary fix. If we could find a simpler workaround in Sage that would be great, otherwise we'll have to try to get the Python change reverted I guess.

jdemeyer commented 7 years ago

comment:24

Replying to @tobihan:

we'll have to try to get the Python change reverted I guess.

I have no idea how feasible that is, but that would be the best solution. There is clearly a backwards-incompatible change being done, so that would be a good argument to revert the change.

nthiery commented 7 years ago

comment:25

Replying to @tobihan:

I now have a patch that changes more than 100 files and that's still not all. I don't think fixing this case-by-case in this way is viable.

Sounds like it indeed.

In any case this would be just a temporary fix. If we could find a simpler workaround in Sage that would be great, otherwise we'll have to try to get the Python change reverted I guess.

I assume you have tried to do this xxx.__new__ change in the UniqueRepresentation base class, and that did not work, right? Plausibly because which xxx shall be used depends from one case to the other. Maybe the ClassCallMetaclass.__init__ method could handle this upon class creation, figuring out the proper xxx to use.

I am currently recompiling Sage with the new branch (thanks Jeroen), and will try to look at this later tonight.

I tchated with Florent earlier today; he was sitting next to Julien last Spring when they investigated this. He will have a look as well and try to remember and post the details.

nthiery commented 7 years ago

comment:26

Replying to @jdemeyer:

Replying to @tobihan:

we'll have to try to get the Python change reverted I guess.

I have no idea how feasible that is, but that would be the best solution. There is clearly a backwards-incompatible change being done, so that would be a good argument to revert the change.

+1

(revert or possibly ammend the change so that it would not break the backward compatibility, but still achieve the desired feature they wanted)

tobihan commented 7 years ago

comment:27

If we can revert this in the Python package in Debian in time depends on what the maintainer thinks and if he responds quickly.

It would help if the change would break another software that is already in Debian. In that case the bug would have a higher severity (RC bug) and that would force the maintainer to do something or allow us to upload a change. I already rebuilt some packages to see if they are affected (numpy, scipy, etc) but they all seem to be fine. If someone knows software that uses inheritance with cdef classes and is in Debian let me know.

jdemeyer commented 7 years ago

comment:28

Here is a more minimal breaking example:

obj.pyx:

cdef class OBJ(object):
    pass

Compile this (within Sage) with

./sage --sh -c "cython obj.pyx && gcc -Ilocal/include/python2.7 -Wall -shared -fPIC obj.c -o obj.so"

With Python 2.7.13rc1, this gives

In [1]: from obj import OBJ

In [2]: class X(OBJ, dict):
   ...:     pass
   ...: 

In [3]: X()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-a7d4f7b89654> in <module>()
----> 1 X()

TypeError: obj.OBJ.__new__(X) is not safe, use dict.__new__()

hivert commented 7 years ago

comment:29

Replying to @jdemeyer:

Here is a more minimal breaking example:

Hi there,

Here is what I understand:

When creating a class, Python has to decide which function needs to be called to allocate (__new__ method) an object. As the C-level, the function is put in a particular field called tp_new of the C-struct corresponding to the class.
In case of multiple inheritance, this is only possible if the two classes has a compatible memory layout. In this case, if one of the class has a specific extra C-attribute, its __new__ must be called (if both classes have extra attributes then they are not layout compatible).
Before the change of https://bugs.python.org/issue25731, CPython had a slow way to determine which must be called (see comment at https://hg.python.org/cpython/rev/e7062dd9085e/). When trying to optimize that, they introduced a bug, which Julien investigate with my help. As a consequence the change was reverted.
In https://hg.python.org/cpython/rev/a37cc3d926ec, they reintroduced the fast method issuing a warning if the wrong __new__ is called. As far as I understand, this means that in case of multiple inheritance with Cython code, only the first class can extend the C-struct.

Note that I have to investigate further to be sure of this last point.

nthiery commented 7 years ago

comment:30

Replying to @nthiery:

I assume you have tried to do this xxx.__new__ change in the UniqueRepresentation base class, and that did not work, right? Plausibly because which xxx shall be used depends from one case to the other. Maybe the ClassCallMetaclass.__init__ method could handle this upon class creation, figuring out the proper xxx to use.

I investigated this a bit further. First I believe we don't override object.__new__ in any of our Cython classes, and in only very few of our Python classes:

mistral-/opt/sage/src/sage>grep "def __new__" **/*.py*
combinat/shard_order.py:    def __new__(cls, p):
combinat/words/alphabet.py:    def __new__(self, alphabet=None, name=None):
combinat/words/finite_word.py:    def __new__(cls, words):
misc/classcall_metaclass.pyx:        def __new__(*args):
misc/classcall_metaclass.pyx:            ....:     def __new__(cls):
misc/sage_input.py:    def __new__(cls, cmds, expr, locals=None):
misc/six.py:        def __new__(cls, name, _, __dict__):
rings/infinity.py:    def __new__(cls, *args):
rings/qqbar.py:    def __new__(cls):
rings/qqbar.py:    def __new__(cls):
rings/qqbar.py:    def __new__(self, generator, value):
rings/qqbar.py:    def __new__(self, a, b):
rings/rational_field.py:    def __new__(cls):

Therefore, resetting cls.__new__ to be just object.__new__ for all our classes in the ClassCallMetaclass should be safe. So I added just that at the end of ClassCallMetaclass.__init__:

     self.__new__ = object.__new__

This seems to do the job for e.g. Unknown and a couple others.

However this does not seem to be quite what we want: it sounds like object.__new__ is "bound" to the class object, and I believe that's what forced Tobias to use e.g. __new__ = Functor.__new__ in his patch. Instead, what we would want to do is the analog of plain Python's

    self.__new__ = object.__new__.im_func

to make sure that self.__new__ is bound to self and not to object. I have tried to play around with what we do in a similar context in sage.structure.misc.getattr_from_other_class, but did not quite manage to. Trying to fetch object.__new__.__get__ with Py_TYPE(attribute).tp_descr_get returns null.

Florent, Jeroen, or anyone with good knowledge of the CPython API, could you have a look? At the end of the day, this is all probably emulating the proper initialization of the tp_new attribute that's been broken by Python's change. Maybe it's in fact just enough to emulate this initialization for the WithEqualityById class. Or just define a trivial WithEqualityById.__new__?

Florent: will you be at LRI tomorrow? If yes, we can have a sprint together on this.

While I am fairly optimistic on having a workaround soon, it still would be good to investigate in parallel getting this fixed / patched in Python.

Tobias: to answer your question, on my side, I haven't heard of other soft using inheritance from multiple Cython classes.

Cheers, Nicolas

jdemeyer commented 7 years ago

comment:31

Replying to @nthiery:

First I believe we don't override object.__new__ in any of our Cython classes

Not in the literal sense. However, Cython always implements a custom tp_new (which is the C version of __new__) for any cdef class. So effectively, we are overriding __new__ in every cdef class.

Therefore, resetting cls.__new__ to be just object.__new__ for all our classes in the ClassCallMetaclass should be safe.

Because of the above, I am afraid this is not true.

jdemeyer commented 7 years ago

comment:32

As I mentioned in https://bugs.python.org/issue5322, I think that breaking multiple inheritance of __new__ is a side-effect of the fix and not a fundamental feature needed to fix that bug. So there might be some hope of fixing https://bugs.python.org/issue5322 without breaking multiple inheritance of __new__.

nthiery commented 7 years ago

comment:33

Florent: for whatever it's worth (not much) I pushed some changes I experimented with using ClassCallMetaclass.__init__:

https://github.com/sagemath/sagetrac-mirror/commits/u/nthiery/22037/upgrade_to_python_2_7_13_classcall_workaround

nthiery commented 7 years ago

comment:34

Replying to @jdemeyer:

As I mentioned in https://bugs.python.org/issue5322, I think that breaking multiple inheritance of __new__ is a side-effect of the fix and not a fundamental feature needed to fix that bug. So there might be some hope of fixing https://bugs.python.org/issue5322 without breaking multiple inheritance of __new__.

That would be the ideal solution indeed!

Am I interpreting it correctly that the hunk causing trouble for us as just been reverted?

https://bugs.python.org/issue5322#msg283170

If yes, does that resolve our issue for the Debian packaging?

nthiery commented 7 years ago

comment:35

Otherwise, worst comes to worst, we could consider the following tentative workaround: making WithEqualityById into a plain Cython class. This should be easy and only result in a speed regression, presumably not critical.

Cheers,

tobihan commented 7 years ago

comment:36

Replying to @nthiery:

Replying to @jdemeyer:

As I mentioned in https://bugs.python.org/issue5322, I think that breaking multiple inheritance of __new__ is a side-effect of the fix and not a fundamental feature needed to fix that bug. So there might be some hope of fixing https://bugs.python.org/issue5322 without breaking multiple inheritance of __new__.

That would be the ideal solution indeed!

Am I interpreting it correctly that the hunk causing trouble for us as just been reverted?

https://bugs.python.org/issue5322#msg283170

If yes, does that resolve our issue for the Debian packaging?

Yes it has been reverted upstream. It probably solves the problem. The maintainer of the Python package said he hopes a new Python version will be released this weekend and in this case he will upload it quickly to Debian. If no new Python is released we don't have a statement from him but I hope he would apply a patch to revert the change then.

nthiery commented 7 years ago

comment:37

Nice! Crossing my fingers ...

nthiery commented 7 years ago

comment:38

Just for the record: in the meantime, I had made a quick experiment with WithEqualityById as plain Python class, and got confused, as I still get the same error as before:

    Unknown = UnknownClass()
    ...
    TypeError: sage.structure.sage_object.SageObject.__new__(UnknownClass) is not safe, use object.__new__()

whereas UnknowClass now has only one Cython base class; here is its mro:

(<class 'sage.misc.unknown.UnknownClass'>, <class 'sage.structure.unique_representation.UniqueRepresentation'>, <class 'sage.structure.unique_representation.CachedRepresentation'>, <class 'sage.misc.fast_methods.WithEqualityById'>, <type 'sage.structure.sage_object.SageObject'>, <type 'object'>)

tobihan commented 7 years ago

comment:39

Python 2.7.13 was released without this change that breaks Sage, and already uploaded to Debian. So this is no longer an issue, unless Python really applies this change in a major version update one day. Thanks everybody for helping on this unexpected last minute obstruction!

BTW, Sage just hit Debian experimental today and we will upload it to unstable probably Wednesday or so. Cross your fingers!

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 7 years ago

comment:40

I'd like to point people following this ticket to this thread which explains that the present ticket has some urgency. I attempted a workaround in #22089 and failed miserably.

Could you let us know what remains to be done to have this ticket completed ?

jdemeyer commented 7 years ago

comment:41

Replying to @EmmanuelCharpentier:

Could you let us know what remains to be done to have this ticket completed ?

Nothing special, just do the upgrade.

jdemeyer commented 7 years ago

comment:42

OK, I'll work on this.

jdemeyer commented 7 years ago

Changed dependencies from #19735 to none

jdemeyer commented 7 years ago

Author: Jeroen Demeyer

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -1,11 +1,3 @@
-Python 2.7.13rc1 leads to problems with sage related to `__new__`. It is likely related to this change that was included in Python:
-https://bugs.python.org/issue5322
-
-See the following discussion on sage-devel:
-https://groups.google.com/forum/#!topic/sage-devel/IbugChsM26E
-
-See also https://bugs.python.org/issue25731, where a patch at the same location was reverted as backward incompatible after causing a failure in Sage.
-
-**Tarball**: https://www.python.org/ftp/python/2.7.13/Python-2.7.13rc1.tgz
+**Tarball**: https://www.python.org/ftp/python/2.7.13/Python-2.7.13.tgz

 Note: upgrade to Python 2.7.12 done in #19735.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

5aeaf5c Upgrade to Python 2.7.13

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from ba486e1 to 5aeaf5c

embray commented 7 years ago

comment:44

Ah, I think Florent was telling me about this a couple weeks ago. Wish I had been CC'd on this--I've spent a lot of time in Python's class initialization code and probably could have helped. Let me know if I can still help on this.

(Ah, I see now the issue was fixed upstream too--funny that Armin raised this issue back in 2009 and now it gets fixed :)

sagemath / sage

Upgrade to Python 2.7.13 #22037

19735 "Upgrade to Python 2.7.12" was just closed.