Python 3 fixes to Cython

[fchapoton]

1. Fix running the Cython testsuite on Python 3.

2. Fix the issue that unbound methods are unhashable:

sage: hash(Graph.is_long_hole_free)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-41-8a5746117695> in <module>()
----> 1 hash(getattr(Graph, 'is_long_hole_free'))

TypeError: unhashable type: 'instancemethod'

Upstream: cython/cython#2105

Upstream: Fixed upstream, but not in a stable release.

CC: @embray

Component: python3

Author: Jeroen Demeyer

Branch: 7187fb9

Reviewer: Frédéric Chapoton, François Bissey

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

[fchapoton]

comment:2

If we compare the two lines

from sage.graphs.weakly_chordal import is_long_hole_free, is_long_antihole_free, is_weakly_chordal
from sage.graphs.asteroidal_triples import is_asteroidal_triple_free

Both are imported from a pyx file, only the first is problematic.
Comparing:

sage: getattr(Graph, "is_asteroidal_triple_free")
<built-in function is_asteroidal_triple_free>
sage: getattr(Graph, "is_long_hole_free")
<instancemethod is_long_hole_free at 0x7fed5c5758b8>

and the second one is not hashable. Both come from a "def". What is the difference ?

[fchapoton]

comment:3

Jeoren, any idea as a cython expert of what could explain the difference in the previous comment ?

[jdemeyer]

comment:4

The difference is that weakly_chordal.pyx is compiled with # cython: binding=True (see the first line of that file).

But really, is_asteroidal_triple_free is broken. It doesn't work as a method:

sage: Graph(5).is_asteroidal_triple_free()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-11-79cc7f0a39a3> in <module>()
----> 1 Graph(Integer(5)).is_asteroidal_triple_free()

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/graphs/asteroidal_triples.pyx in sage.graphs.asteroidal_triples.is_asteroidal_triple_free (build/cythonized/sage/graphs/asteroidal_triples.c:6495)()
     80 from sage.ext.memory_allocator cimport MemoryAllocator
     81 
---> 82 def is_asteroidal_triple_free(G, certificate=False):
     83     """
     84     Test if the input graph is asteroidal triple-free

TypeError: is_asteroidal_triple_free() takes at least 1 positional argument (0 given)

So don't take is_asteroidal_triple_free as example.

[jdemeyer]

comment:5

In Python 3, an unbound method is just the function:

In [18]: class X:
    ...:     def meth(self): return self

In [19]: X.meth
Out[19]: <function __main__.X.meth>

In [20]: type(X.meth)
Out[20]: function

So the fact that Cython gives an instancemethod instead of a function might be considered a Cython bug. I'll think about it later.

[jdemeyer]

Upstream: Not yet reported upstream; Will do shortly.

[jdemeyer]

Dependencies: #21509

[jdemeyer]

Branch: u/jdemeyer/py3__fix_fully_the_thematic_index_of_methods_in_graphs

[sagetrac-git]

Commit: 786fed7

[sagetrac-git]

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

786fed7

Fix unbound methods in Python 3

[jdemeyer]

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

[jdemeyer]

Author: Jeroen Demeyer

[sagetrac-git]

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

d13c9bd	Fix running Cython testsuite on Python 3
bc997e7	Fix unbound methods in Python 3

[sagetrac-git]

Changed commit from 786fed7 to bc997e7

[sagetrac-git]

Changed commit from bc997e7 to 86e7f17

[sagetrac-git]

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

86e7f17

Fix unbound methods in Python 3

[jdemeyer]

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

[jdemeyer]

comment:14

To potential reviewers: the dependency on #21509 is just because both tickets add a patch to Cython. If it's hard to review #21509, I could reorganize things. For example, I could put both patches on this ticket and reverse the dependency. Just let me know which pieces of #21509 and #24728 you could give positive review to.

[jdemeyer]

comment:15

#21509 is stuck for now.

Regardless, the commits from this ticket can still be reviewed. If this ticket gets positive review, I will reorganize the dependency to make this ticket independent from #21509.

[fchapoton]

comment:16

Ok, I am ready to give a positive review here. Please disentangle from #21509.

[jdemeyer]

Changed dependencies from #21509 to none

[sagetrac-git]

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

f6621e6	Fix https://github.com/cython/cython/pull/2095
75dbc84	Fix running Cython testsuite on Python 3
7187fb9	Fix unbound methods in Python 3

[sagetrac-git]

Changed commit from 86e7f17 to 7187fb9

[jdemeyer]

Reviewer: Frédéric Chapoton, François Bissey

[vbraun]

Changed branch from u/jdemeyer/py3__fix_fully_the_thematic_index_of_methods_in_graphs to 7187fb9

[fchapoton]

Changed commit from 7187fb9 to none

[fchapoton]

comment:21

Sadly, this still does not fix the issue number 2 in the description..

So, vanilla sage still does not start with python3..

[jdemeyer]

comment:22

I think you need ./sage -f sagelib

[fchapoton]

comment:23

Ho, is this stronger than make build ? ok, I will try.

[jdemeyer]

comment:24

Similar to other packages inside Sage-the-distribution, ./sage -f forces a complete rebuild of that package, regardless of what was installed before. So ./sage -f sagelib forces a complete rebuild of the Sage library. In particular, in this ticket we need to run Cython again on the affected file.

make build simply builds all of Sage-the-distribution (except doc), but it won't reinstall anything which is already installed.

[fchapoton]

comment:25

Thanks, it worked. So indeed now sage+python3 does start !

[jdemeyer]

comment:26

Replying to @fchapoton:

Thanks, it worked. So indeed now sage+python3 does start !

So I guess we should update the build-python3 target (and actually start using it!)