Use cythonize() from cython for Sage module building. #13031

robertwb · 2012-05-27T12:02:04Z

Cython's cythonize is now robust enough to replace our custom dependency logic, with a few extra features such as * syntax for cython extensions and cycache (still needs work).

Apply to the Sage library:

Depends on #13029
Depends on #13432

CC: @jdemeyer @roed314 @ohanar @ppurka @kini @mwhansen

Component: build

Keywords: sd40.5

Author: Robert Bradshaw, R. Andrew Ohana

Reviewer: Jeroen Demeyer, R. Andrew Ohana

Merged: sage-5.9.beta4

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

robertwb · 2012-05-27T12:02:21Z

Attachment: 13031-cythonize.patch.gz

ppurka · 2012-05-27T14:07:14Z

comment:2

Is the patch based against sage-5.1beta0? I upgraded a sage installation from sage-5.0rc0 to sage-5.1beta0 and it fails to apply there. I will try a sage-5.1beta0 tarball sometime later.

mwhansen · 2012-05-27T23:19:06Z

Changed keywords from none to sd40.5

williamstein · 2012-05-27T23:35:53Z

comment:5

Changing to needs work since -- as mentioned above -- this doesn't apply to sage-5.1.beta0:


adding 13031-cythonize.patch to series file
applying 13031-cythonize.patch
patching file module_list.py
Hunk #1 FAILED at 144
Hunk #3 FAILED at 223
Hunk #13 FAILED at 834
Hunk #16 FAILED at 1475
Hunk #20 succeeded at 1698 with fuzz 2 (offset 75 lines).
Hunk #21 succeeded at 1712 with fuzz 2 (offset 79 lines).
4 out of 22 hunks FAILED -- saving rejects to file module_list.py.rej
patching file setup.py
Hunk #1 succeeded at 17 with fuzz 2 (offset 0 lines).
Hunk #3 FAILED at 513
1 out of 3 hunks FAILED -- saving rejects to file setup.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 13031-cythonize.patch
wstein@sage:/tmp/wstein/sage-5.1.beta0-boxen-x86_64-Linux/devel/sage/sage$

ohanar · 2012-05-28T02:38:16Z

comment:6

Would you please make the following change:

--- a
+++ b
@@ -1,1 +1,5 @@
-ext_modules = cythonize(ext_modules, exclude=exclude_modules, nthreads=nthreads, cache=os.path.join(DOT_SAGE, 'cythoncache')))
+if 'CYCACHE_DIR' in os.environ:
+    CYCACHE_DIR = os.environ['CYCACHE_DIR']
+else:
+    CYCACHE_DIR = os.path.join(DOT_SAGE,'cycache')
+ext_modules = cythonize(ext_modules, exclude=exclude_modules, nthreads=nthreads, cache=CYCACHE_DIR))

This way we can easily specify the cache directory separately from DOT_SAGE.

ohanar · 2012-05-29T09:11:00Z

Attachment: 13031-cythonize-5.1.beta1.patch.gz

ohanar · 2012-06-07T11:18:07Z

Author: Robert Bradshaw, R. Andrew Ohana

ohanar · 2012-06-07T11:18:07Z

comment:8

Attachment: 13031-cythonize-5.1.beta2.patch.gz

I attached an updated patch that

is rebased on the final 5.1.beta2
fixes a ZeroDivisionError that was preventing documentation from building
more throughly cleans up module_list.py with the use of *s
does other clean up with module_list.py since this patch will constantly need to be rebased with each development release (may as well clean up everything while we are at it)

That said, there is still some issue with Cython's cythonize, even using the old module_list.py you encounter the issue:

sage: cython('a = 5')
sage: a
Traceback (most recent call last):
...
NameError: name 'a' is not defined

You'll find that tons of doctests fail, and frequently they are having NameErrors like this (although they aren't necessarily calling Sage's cython function).

robertwb · 2012-06-23T09:24:15Z

comment:9

Thanks for looking at this. I fixed the patch, Sage relies on a deprecated "feature" of globals(). Running all tests...

robertwb · 2012-06-23T17:27:43Z

comment:10

FYI, all tests passed for me.

ohanar · 2012-06-25T03:17:42Z

comment:11

Ok, looks good to me and works well with 5.1.beta2, so I'm going to mark this back as needing review.

As expected, it needs to be rebased again on more recent betas, but since the changes to module_list.py is so massive, I'm not going to bother with rebasing until Jeroen decides which beta he would like to merge it in.

For whoever reviews this, please use 5.1.beta2 as a basis.

ohanar · 2012-07-01T10:12:05Z

comment:12

The new patch fixes an issue with DOT_SAGE not being defined in setup.py.

jhpalmieri · 2012-07-01T15:28:32Z

comment:13

Rather than code like this:

if not os.path.exists(cache_dir): 
   os.mkdir(cache_dir)

you should use sage.misc.misc.sage_makedirs (or copy-paste the code from there). A try-except block is safer than testing whether than the directory exists first.

ohanar · 2012-07-14T05:04:44Z

comment:14

I just rebased on 5.2.beta1 since the dependency was merged into that release. However, in the process I discovered that cycache is not ready for primetime yet:

$ grep 'cdef class MonoDict:' sage -R
sage/sets/disjoint_set.c: * cdef class MonoDict:             # <<<<<<<<<<<<<<
sage/matrix/matrix_modn_dense_float.cpp: * cdef class MonoDict:             # <<<<<<<<<<<<<<

Neither of these modules changed between 5.1.beta2 and 5.2.beta1, but their dependencies changed, so there is some issue with the hashing that needs to be resolved.

For now I've disabled cycache, since it will be trivial to re-add once this ticket gets merged.

robertwb · 2012-07-14T08:26:55Z

comment:15

Yes, I think it's perfectly fine to disable cycache for now to get this in (as it will then be much easier to work on). Could you expound on what the issue was though? I'm not quite following your grep is trying to show...

jdemeyer · 2013-03-28T19:39:34Z

comment:55

The file sage/schemes/generic/notes/divisor_stein-joyner.txt is removed in #13062, so attachment: 13031-doctest-fix.patch needs to be rebased.

robertwb · 2013-03-28T19:49:56Z

comment:56

Attachment: 13031-doctest-fix-rebased.patch.gz

jdemeyer · 2013-04-01T10:39:45Z

Merged: sage-5.9.beta3

jdemeyer · 2013-04-02T12:08:59Z

Changed merged from sage-5.9.beta3 to sage-5.9.beta4

jdemeyer · 2013-04-05T10:01:04Z

comment:59

Robert: any plans to add the version checking upstream to cythonize()?

jdemeyer · 2013-04-15T14:34:22Z

comment:60

I just found some important bug: it seems the dependencies on the files in c_lib/ are no longer correctly handled.

$ touch devel/sage/c_lib/include/interrupt.h && ./sage -b

should rebuild all files depending on interrupt.h but this doesn't work anymore.

In sage-5.9.beta3, one gets instead lots of lines like

Building sage/libs/ecl.pyx because it depends on /mazur/release/merger/sage-5.9.beta3/local/include/csage/interrupt.h.

robertwb · 2013-04-16T03:51:39Z

comment:61

I'm on it.

jdemeyer · 2013-04-19T08:56:32Z

comment:62

Any progress?

robertwb · 2013-04-19T09:29:01Z

comment:63

The fix will be in Cython 0.19.

robertwb · 2013-04-19T09:29:18Z

comment:64

(We could backport it if that takes too long.)

jdemeyer · 2013-04-19T10:36:39Z

comment:65

Thanks. Anyway, good to know it's a Cython bug, not Sage bug.

jdemeyer · 2013-04-19T14:55:59Z

comment:66

Proposal: unmerge this for sage-5.9, merge in again in sage-5.10 together with #14452.

jdemeyer · 2013-04-19T14:56:36Z

comment:67

(or possibly merge #14452 in sage-5.9.rc0)

robertwb · 2013-04-19T17:23:27Z

comment:68

Alternative proposal: cython-0.18.p1. I really would not like to see this unmerged after it finally got in.

jdemeyer · 2013-04-19T18:45:25Z

comment:69

Given that cython-0.19 is released today and we have a workaround for the docbuilder crash at #14452, why not cython-0.19?

jdemeyer · 2013-05-07T12:53:37Z

comment:70

There are still serious problems with dependency checking: #14544

nexttime · 2013-05-11T17:18:33Z

comment:71

Replying to @jdemeyer:

There are still serious problems with dependency checking: #14544

There are also reports on sage-devel that sage -clone was broken in 5.9 (and later), now rebuilding the whole Sage library. Not sure what exactly introduced that though.

nexttime · 2013-05-11T19:31:12Z

comment:72

Replying to @nexttime:

There are also reports on sage-devel that sage -clone was broken in 5.9 (and later), now rebuilding the whole Sage library. Not sure what exactly introduced that though.

Looks like just the fact that sage-clone doesn't copy $SAGE_SRC/.cython_version causes this...

(where $SAGE_SRC is [meanwhile] $SAGE_ROOT/devel/sage)

Also (but this doesn't seem to be the cause), sage-clone does not (or no longer, presumably at least since a while) copy Cython-generated header files (*.h), as these do not contain a comment on the first line stating that they were generated by Cython, so the following doesn't work for them:

print "Copying over all Cython auto-generated .c, .cpp and .h files..."
def cpdir(src, dest):
    if not os.path.isdir(dest):
        return
    for F in os.listdir(src):
        if os.path.isdir(src + '/' + F):
            cpdir(src + '/' + F, dest + '/' + F)
        else:
            ext = os.path.splitext(F)[-1]
            if ext in ['.h', '.c', '.cpp']:
                if 'Cython' in open(src + '/' + F).readline():
                    os.link(src + '/' + F, dest + '/' +F)
                    os.utime(dest + '/' +F, None)

cpdir(os.path.abspath('sage/sage'), os.path.abspath(branch + '/sage'))

nexttime · 2013-05-11T20:13:47Z

comment:73

Replying to @nexttime:

Also (but this doesn't seem to be the cause), sage-clone does not (or no longer, presumably at least since a while) copy Cython-generated header files (*.h), as these do not contain a comment on the first line stating that they were generated by Cython, so the following doesn't work for them:

print "Copying over all Cython auto-generated .c, .cpp and .h files..."
def cpdir(src, dest):
    if not os.path.isdir(dest):
        return
    for F in os.listdir(src):
        if os.path.isdir(src + '/' + F):
            cpdir(src + '/' + F, dest + '/' + F)
        else:
            ext = os.path.splitext(F)[-1]
            if ext in ['.h', '.c', '.cpp']:
                if 'Cython' in open(src + '/' + F).readline():
                    os.link(src + '/' + F, dest + '/' +F)
                    os.utime(dest + '/' +F, None)

cpdir(os.path.abspath('sage/sage'), os.path.abspath(branch + '/sage'))

Furthermore, the above code doesn't copy the Cython-generated files from devel/sage/ext/interpreters/, so these also get rebuilt, but that goes back to at least Sage 5.8 (unless I'm missing something).

If I add copying .cython_version to sage-clone, besides the above-mentioned re-"cythonizing"[sic!], many (if not all?) Python modules get re-byte-compiled, too, for whatever reason... (This doesn't seem to be related to Cython, but maybe I'm just missing something here as well.)

nexttime · 2013-05-11T20:54:10Z

comment:74

Replying to @nexttime:

If I add copying .cython_version to sage-clone ...

diff --git a/sage-clone b/sage-clone
--- a/sage-clone
+++ b/sage-clone
@@ -53,6 +53,10 @@
 
 cpdir(os.path.abspath('sage/sage'), os.path.abspath(branch + '/sage'))
 
+if os.path.isfile('sage/.cython_version'):
+    print "Copying over hidden Cython version file..."
+    os.link('sage/.cython_version', branch+'/.cython_version')
+
 def copy_dtree(src_dir, dest_dir):
     src_root = os.path.abspath(src_dir)
     dest_root = os.path.abspath(dest_dir)

robertwb added this to the sage-5.8 milestone May 27, 2012

robertwb added c: build labels May 27, 2012

robertwb assigned sagetrac-GeorgSWeber May 27, 2012

robertwb added the s: needs review label May 27, 2012

williamstein added s: needs work and removed s: needs review labels May 27, 2012

robertwb added s: needs review and removed s: needs work labels May 29, 2012

ohanar added s: needs work and removed s: needs review labels Jun 7, 2012

ohanar added s: needs review and removed s: needs work labels Jun 25, 2012

This comment has been minimized.

Sign in to view

robertwb added s: needs review and removed s: needs work labels Mar 19, 2013

ohanar added s: positive review and removed s: needs review labels Mar 28, 2013

This comment has been minimized.

Sign in to view

jdemeyer removed the s: positive review label Apr 1, 2013

jdemeyer closed this as completed Apr 1, 2013

jhpalmieri mentioned this issue Jun 13, 2013

sage --clone: do not rebuild the entire Sage library, and do not rebuild the docs #13245

Closed

jdemeyer mentioned this issue May 17, 2013

Cython dependency checking broken #14544

Closed

vbraun mentioned this issue Jun 3, 2013

Cython debug symbols and sage --gdb #14649

Closed

Use cythonize() from cython for Sage module building. #13031

Use cythonize() from cython for Sage module building. #13031

Comments

robertwb commented May 27, 2012

robertwb commented May 27, 2012

ppurka commented May 27, 2012

mwhansen commented May 27, 2012

williamstein commented May 27, 2012

ohanar commented May 28, 2012

ohanar commented May 29, 2012

ohanar commented Jun 7, 2012

ohanar commented Jun 7, 2012

robertwb commented Jun 23, 2012

robertwb commented Jun 23, 2012

ohanar commented Jun 25, 2012

ohanar commented Jul 1, 2012

jhpalmieri commented Jul 1, 2012

This comment has been minimized.

ohanar commented Jul 14, 2012

robertwb commented Jul 14, 2012

jdemeyer commented Mar 28, 2013

This comment has been minimized.

robertwb commented Mar 28, 2013

jdemeyer commented Apr 1, 2013

jdemeyer commented Apr 2, 2013

jdemeyer commented Apr 5, 2013

jdemeyer commented Apr 15, 2013

robertwb commented Apr 16, 2013

jdemeyer commented Apr 19, 2013

robertwb commented Apr 19, 2013

robertwb commented Apr 19, 2013

jdemeyer commented Apr 19, 2013

jdemeyer commented Apr 19, 2013

jdemeyer commented Apr 19, 2013

robertwb commented Apr 19, 2013

jdemeyer commented Apr 19, 2013

jdemeyer commented May 7, 2013

nexttime mannequin commented May 11, 2013

nexttime mannequin commented May 11, 2013

nexttime mannequin commented May 11, 2013

nexttime mannequin commented May 11, 2013