Use cythonize() from cython for Sage module building.

[robertwb]

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:

• trac13031-cythonize-simple.patch
• trac13031-cythonize-version.patch
• 13031-doctest-fix-rebased.patch
• 13031-flush.patch

[ppurka]

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.

[was]

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$ 

[rohana]

Would you please make the following change:

(a) a vs. (b) b

@@ -1 @@
-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.

[rohana]

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]

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

[robertwb]

FYI, all tests passed for me.

[rohana]

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.

[rohana]

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

[jhpalmieri]

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.

[rohana]

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]

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...

[rohana]

Replying to robertwb:

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...

The grep is of some cython code that existed in 5.1.beta2 in sage/structure/coerce_dict.pxd, that is no longer present in 5.2.beta1. However, cython detected that I had a cached version of the disjoint_set module (which didn't change between versions, but imports the coerce_dict module) and incorrectly pulls the the cached version of disjoint_set.c when it actually needs to rebuild it since coerce_dict.pxd changed (same situation for the other module in the grep).

[rohana]

OK, rebased. Be nice if someone reviewed this soon.

[jdemeyer]

This probably should be rebased to #13164 and #12883.

[rohana]

Replying to jdemeyer:

This probably should be rebased to #13164 and #12883.

I'll do that once they are merged. This ticket so heavily affects module_list.py that I would rather get this reviewed on some clean development build, and then try to figure out with you what tickets I'll need to rebase against to merge this into whatever beta you feel is appropriate. Since this has been waiting review for a bit, I rather not jump the gun and start rebasing against things that haven't been merged yet.

[jdemeyer]

This seems to cause Sphinx problems:

/release/merger/sage-5.4.beta1/devel/sage/doc/en/reference/sage/modular/modform/eis_series_cython.rst:11: WARNING: error while formatting signature for sage.modular.modform.eis_series_cython.clear_mpz_globals: Could not parse cython argspec
/release/merger/sage-5.4.beta1/devel/sage/doc/en/reference/sage/modular/modform/eis_series_cython.rst:11: WARNING: error while formatting signature for sage.modular.modform.eis_series_cython.gmp_randrange: Could not parse cython argspec
/release/merger/sage-5.4.beta1/devel/sage/doc/en/reference/sage/modular/modform/eis_series_cython.rst:11: WARNING: error while formatting signature for sage.modular.modform.eis_series_cython.init_mpz_globals: Could not parse cython argspec

[rohana]

Can you please clarify what command you ran?

[jdemeyer]

Replying to rohana:

Can you please clarify what command you ran?

This is build from scratch, so it's just make doc. This is probably equivalent to removing the doc/output directory and then doing make doc.

[rohana]

apply to sage library

[rohana]

Ok, rebased on 5.4.beta1.

The issue with sphinx seems to be because of the python functions defined in sage/ext/gmp.pxi. We probably shouldn't have any python function defined in pxi files anyway (since we currently aren't checking them for coverage or anything). There are python functions in other pxi files, but they aren't causing any issues for some reason (Robert, maybe you could explain why?).

Anyway, for right now I removed one of them (that wasn't being used anywhere), and I moved the other two to sage/rings/integer.pyx since that is where the sage library looks for them.

[robertwb]

apply only this patch

[robertwb]

OK, I've changed this to only switch to use cythonize and get rid of the sphinx errors. All the other changes make this fail to apply with each new Sage release, and it's a pain to constantly re-base. Once this is in, we can fix up the module list piecemeal and do other cleanup (though I'm unconvinced writing os.path.join('sage','rings','padics','*.pyx') is really preferable to "sage/rings/padics/*.pyx") as was introduced in the latest rebase.

[rohana]

Everything looks good and works well.

[jdemeyer]

There is trouble with upgrading:

On rosemary (Linux RHEL 5.6 x86_64), after upgrading from sage-4.5.2, I get a failure during the installation of the Conway spkg:

Traceback (most recent call last):
  File "./spkg-install", line 4, in <module>
    from sage.all import save
  File "/home/buildbot/build/sage/rosemary-1/rosemary_upgrade_4.5.2/build/sage-5.7.beta3/local/lib/python2.7/site-packages/sage/all.py", line 74, in <module>
    from sage.matrix.all     import *
  File "/home/buildbot/build/sage/rosemary-1/rosemary_upgrade_4.5.2/build/sage-5.7.beta3/local/lib/python2.7/site-packages/sage/matrix/all.py", line 1, in <module>
    from matrix_space import MatrixSpace, is_MatrixSpace
  File "/home/buildbot/build/sage/rosemary-1/rosemary_upgrade_4.5.2/build/sage-5.7.beta3/local/lib/python2.7/site-packages/sage/matrix/matrix_space.py", line 33, in <module>
    import matrix
  File "matrix.pyx", line 1, in init sage.matrix.matrix (sage/matrix/matrix.c:2043)
  File "matrix2.pyx", line 1, in init sage.matrix.matrix2 (sage/matrix/matrix2.c:71233)
  File "matrix1.pyx", line 1, in init sage.matrix.matrix1 (sage/matrix/matrix1.c:13429)
  File "mutability.pxd", line 15, in init sage.matrix.matrix0 (sage/matrix/matrix0.c:29523)
ValueError: PyCapsule_GetPointer called with invalid PyCapsule object

Running ./sage -ba-force fixes this.

[jdemeyer]

Are old-style Cython modules and new-style Cython modules (made with cythonize()) compatible? If yes, the problem is probably with dependency checking (some modules should be rebuilt but aren't).

[jdemeyer]

Thanks to some sed magic, this is a list of modules rebuilt in sage-5.7.beta2 but which were not rebuilt with this ticket:

sage/combinat/words/word_datatypes.pyx
sage/ext/interactive_constructors_c.pyx
sage/games/sudoku_backtrack.pyx
sage/media/channels.pyx
sage/misc/refcount.pyx
sage/misc/search.pyx
sage/quadratic_forms/quadratic_form__evaluate.pyx
sage/rings/polynomial/symmetric_reduction.pyx
sage/structure/mutability.pyx

In the sage-5.7.beta2 log file, there is

Building modified file sage/combinat/words/word_datatypes.pyx.
Building modified file sage/ext/interactive_constructors_c.pyx.
Building modified file sage/games/sudoku_backtrack.pyx.
Building modified file sage/media/channels.pyx.
Building modified file sage/misc/refcount.pyx.
Building modified file sage/misc/search.pyx.
Building modified file sage/quadratic_forms/quadratic_form__evaluate.pyx.
Building modified file sage/rings/polynomial/symmetric_reduction.pyx.
Building modified file sage/structure/mutability.pyx.

In the log file with #13031, there is no mention of any of these files.

[jdemeyer]

Files related to mutability.pyx after building Sage with #13031:

-rwxr-xr-x 1 buildbot buildbot 52116 2013-02-04 04:32:19.000000000 -0500 ./devel/sage-main/build/lib.linux-x86_64-2.7/sage/structure/mutability.so
-rwxr-xr-x 1 buildbot buildbot 52116 2013-02-04 04:32:19.000000000 -0500 ./devel/sage-main/build/sage/structure/mutability.so
-rw-r--r-- 1 buildbot buildbot 68728 2013-02-04 04:32:19.000000000 -0500 ./devel/sage-main/build/temp.linux-x86_64-2.7/sage/structure/mutability.o
-rw-r--r-- 1 buildbot buildbot   207 2012-08-23 12:00:08.000000000 -0400 ./devel/sage-main/doc/en/reference/sage/structure/mutability.rst
-rw-r--r-- 1 buildbot buildbot 71416 2012-08-23 11:33:21.000000000 -0400 ./devel/sage-main/sage/structure/mutability.c
-rw-r--r-- 1 buildbot buildbot   529 2010-06-28 12:37:01.000000000 -0400 ./devel/sage-main/sage/structure/mutability.pxd
-rw-r--r-- 1 buildbot buildbot  2062 2012-08-23 11:22:27.000000000 -0400 ./devel/sage-main/sage/structure/mutability.pyx

[jdemeyer]

The .so is recompiled from an old Cython C file which was

/* Generated by Cython 0.12.1 on Thu Aug 23 11:33:21 2012 */

while the old code would always run cython to re-generate the .c file.

Sounds like #4797...

[jdemeyer]

Alternatively, we could also disallow upgrades from versions older than sage-4.6. On the other hand, that would probably not be sufficient as some .c files generated by newer versions of Cython would need to be recreated to account for #13896 for example.

[rohana]

So from what I can tell cythonize is in the right -- these files don't need to be rebuilt (they nor any of their dependencies have been changed). So it looks like it is an issue of using old cython generated code with new cython generated code.

The other potential fix for this particular issue is to remove sage.structure.mutablity, since nothing actually uses it (despite getting imported in a few places).

[rohana]

remove unused mutablity code

[jdemeyer]

Replying to rohana:

So from what I can tell cythonize is in the right -- these files don't need to be rebuilt (they nor any of their dependencies have been changed).

It depends what you mean. One could argue that all Cython files should implicitly depend on the Cython program. Similar how, in an autotools build system, everything depends on config.status (the output of ./configure) or similar how SCons and ccache use the compiler executable as extra dependency of all C files.

[jdemeyer]

Did you fix the issue of C files not being regenerated by a Cython upgrade? Because whether it's the "fault" of Cython or not, it needs to be fixed.

[rohana]

Replying to jdemeyer:

Did you fix the issue of C files not being regenerated by a Cython upgrade?

No, I removed the single offending orphaned code, all the other code that is not regenerated causes no issues (since they are they are isolated from all of the modified files). The proper way to have this resolved is to have Cython's dependency checking not just check the timestamp of a file but also what version of Cython was used to generate the C file -- but I would rather not have this ticket held up on a future version of Cython.

From what I can tell, the current DependancyTree only works because it is finding false positives, not because it is checking the Cython version of generated files. So until I find the time to look at fixing this in Cython, I think the correct resolution is to implement #4797.

[robertwb]

OK, I've added a bit of code to cache and check the version.

[robertwb]

Apply only trac13031-cythonize-simple-5.8.beta4.patch trac13031-mutablity.patch trac13031-cythonize-version.patch

[jdemeyer]

Replying to rohana:

all the other code that is not regenerated causes no issues

Really???????????????????????????????? What about the bug that Cython-0.17.4 fixed?

[jdemeyer]

Replying to rohana:

From what I can tell, the current DependancyTree only works because it is finding false positives, not because it is checking the Cython version of generated files.

True, but at least it works.

[rohana]

Ok, Robert's patch looks good to me.

[jdemeyer]

This needs to be rebased to #13432.

[rohana]

apply to sage library

[rohana]

I thought I had rebased this, oh well, it should now apply cleanly on top of #13432.

[jdemeyer]

Traceback (most recent call last):
  File "./spkg-install", line 4, in <module>
    from sage.all import save
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/all.py", line 80, in <module>
    from sage.misc.all       import *         # takes a while
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/misc/all.py", line 83, in <module>
    from functional import (additive_order,
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/misc/functional.py", line 36, in <module>
    from sage.rings.complex_double import CDF
  File "complex_double.pyx", line 91, in init sage.rings.complex_double (sage/rings/complex_double.c:16796)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/rings/complex_field.py", line 110, in ComplexField
    C = ComplexField_class(prec)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/rings/complex_field.py", line 204, in __init__
    self._populate_coercion_lists_(coerce_list=[complex_number.RRtoCC(self._real_field(), self)])
  File "complex_number.pyx", line 2431, in sage.rings.complex_number.RRtoCC.__init__ (sage/rings/complex_number.c:15890)
  File "map.pyx", line 125, in sage.categories.map.Map.__init__ (sage/categories/map.c:2478)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/categories/homset.py", line 263, in Hom
    H = category.hom_category().parent_class(X, Y, category = category)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/categories/rings.py", line 636, in __new__
    from sage.rings.homset import RingHomset
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/rings/homset.py", line 17, in <module>
    import morphism
  File "morphism.pyx", line 985, in init sage.rings.morphism (sage/rings/morphism.c:12304)
  File "/release/merger/sage-5.9.beta1/local/lib/python2.7/site-packages/sage/structure/all.py", line 49, in <module>
    from mutability  import Mutability
ImportError: No module named mutability

[robertwb]

Apply only trac13031-cythonize-simple.patch and trac13031-cythonize-version.patch

We can delete mutability in a separate CL, now that we have Cython version tracking.

[jdemeyer]

sage -t devel/sage/sage/misc/dev_tools.py
**********************************************************************
File "devel/sage/sage/misc/dev_tools.py", line 145, in sage.misc.dev_tools.import_statements
Failed example:
    import_statements(ZZ)
Expected:
      ** Warning **: several names for that object: ZZ, Z
    from sage.rings.integer_ring import ZZ
Got:
      ** Warning **: several names for that object: Z, ZZ
    from sage.rings.integer_ring import Z
**********************************************************************
File "devel/sage/sage/misc/dev_tools.py", line 158, in sage.misc.dev_tools.import_statements
Failed example:
    import_statements(ZZ, verbose=False)
Expected:
    from sage.rings.integer_ring import ZZ
Got:
    from sage.rings.integer_ring import Z
**********************************************************************

[jdemeyer]

I think you are missing some sys.stdout.flush() statements in cythonize(), as the message

Cythonizing sage/categories/category_singleton.pyx

might come after

gcc -fno-strict-aliasing -std=gnu99 -D_XPG6 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -I/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-5.9.beta1/local/include -I/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-5.9.beta1/local/include/csage -I/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-5.9.beta1/devel/sage/sage/ext -I/export/home/buildbot/build/sage/hawk-1/hawk_full/build/sage-5.9.beta1/local/include/python2.7 -c sage/categories/category_singleton.c -o build/temp.solaris-2.11-i86pc.32bit-2.7/sage/categories/category_singleton.o -w

which is very confusing, especially when errors occur.

(there is nothing special about this file, it's just an example)

[robertwb]

 https://github.com/cython/cython/commit/aeb34fe0046522bad68205129b7a5a0f6a56f0ab

[robertwb]

Granted, there's no reason (other than luck) that we couldn't have hit this before just as well.

[jdemeyer]

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

[jdemeyer]

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

[jdemeyer]

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]

I'm on it.

[jdemeyer]

Any progress?

[robertwb]

The fix will be in Cython 0.19.

[robertwb]

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

[jdemeyer]

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

[jdemeyer]

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

[jdemeyer]

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

[robertwb]

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

[jdemeyer]

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]

There are still serious problems with dependency checking: #14544

[leif]

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.

[leif]

Replying to leif:

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'))

[leif]

Replying to leif:

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.)

[leif]

Replying to leif:

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

sage-clone

@@ -53 +53 @@
 
 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)