Opened 4 years ago

Last modified 4 years ago

#22037 closed enhancement

Upgrade to Python 2.7.13 — at Version 20

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-7.5
Component: packages: standard Keywords:
Cc: SimonKing, hivert, slelievre, charpent Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/nthiery/upgrade_to_python_2_7_13 (Commits) Commit: b7bd4597e18af490dd9e6225e47b16d5ec8d79dc
Dependencies: #19735 Stopgaps:

Description (last modified by jdemeyer)

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

Note: upgrade to Python 2.7.12 done in #19735.

Change History (21)

comment:1 Changed 4 years ago by thansen

  • Description modified (diff)

comment:2 Changed 4 years ago by thansen

  • Description modified (diff)

comment:3 Changed 4 years ago by jdemeyer

  • Description modified (diff)

Changed 4 years ago by thansen

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

comment:4 follow-ups: Changed 4 years ago by thansen

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.

Last edited 4 years ago by thansen (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 4 years ago by nthiery

Replying to thansen:

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

comment:6 Changed 4 years ago by nthiery

  • Cc SimonKing added

comment:7 Changed 4 years ago by nthiery

  • Description modified (diff)

comment:8 Changed 4 years ago by thansen

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

comment:9 in reply to: ↑ 4 Changed 4 years ago by nthiery

Replying to thansen:

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

comment:10 Changed 4 years ago by nthiery

  • Branch set to u/nthiery/upgrade_to_python_2_7_13

comment:11 Changed 4 years ago by nthiery

  • Commit set to b7bd4597e18af490dd9e6225e47b16d5ec8d79dc

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:

896831422037: switch order of the bases of the class UniqueRepresentation
b7bd45922037: more robust backward-compatibility stub class CartanType_simple_finite
Last edited 4 years ago by nthiery (previous) (diff)

comment:12 Changed 4 years ago by thansen

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.

comment:13 follow-up: Changed 4 years ago by nthiery

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,

comment:14 Changed 4 years ago by thansen

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

comment:15 Changed 4 years ago by nthiery

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!

comment:16 Changed 4 years ago by nthiery

  • Cc hivert added

comment:17 Changed 4 years ago by slelievre

  • Cc slelievre added
  • Description modified (diff)

#19735 "Upgrade to Python 2.7.12" was just closed.

comment:18 in reply to: ↑ 13 Changed 4 years ago by jdemeyer

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.

comment:19 Changed 4 years ago by thansen

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:

comment:20 Changed 4 years ago by jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.