Use cythonize() from cython for Sage module building.

robertwb commented 12 years ago

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 commented 12 years ago

Attachment: 13031-cythonize.patch.gz

ppurka commented 12 years ago

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 commented 12 years ago

Changed keywords from none to sd40.5

williamstein commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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

ohanar commented 12 years ago

Author: Robert Bradshaw, R. Andrew Ohana

ohanar commented 12 years ago

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 commented 12 years ago

comment:9

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

robertwb commented 12 years ago

comment:10

FYI, all tests passed for me.

ohanar commented 12 years ago

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 commented 12 years ago

comment:12

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

jhpalmieri commented 12 years ago

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 commented 12 years ago

Description changed:

--- 
+++ 
@@ -1 +1,4 @@
+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:
+* [attachment: trac13031.patch](https://github.com/sagemath/sage-prod/files/10655647/trac13031.patch.gz)

ohanar commented 12 years ago

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 commented 12 years ago

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

ohanar commented 12 years ago

comment:16

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

ohanar commented 12 years ago

comment:17

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

jdemeyer commented 12 years ago

comment:18

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

ohanar commented 12 years ago

comment:19

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 commented 12 years ago

comment:20

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

jdemeyer commented 12 years ago

Work Issues: Sphinx

ohanar commented 12 years ago

comment:21

Can you please clarify what command you ran?

jdemeyer commented 12 years ago

comment:22

Replying to @ohanar:

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.

ohanar commented 12 years ago

apply to sage library

ohanar commented 12 years ago

comment:23

Attachment: trac13031.patch.gz

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 commented 11 years ago

Attachment: 13031-cythonize-simple.patch.gz

apply only this patch

robertwb commented 11 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
 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:
-* [attachment: trac13031.patch](https://github.com/sagemath/sage-prod/files/10655647/trac13031.patch.gz)
+* [attachment: 13031-cythonize-simple.patch](https://github.com/sagemath/sage-prod/files/10655648/13031-cythonize-simple.patch.gz)

robertwb commented 11 years ago

comment:24

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.

ohanar commented 11 years ago

Reviewer: Jeroen Demeyer, R. Andrew Ohana

ohanar commented 11 years ago

Changed work issues from Sphinx to none

ohanar commented 11 years ago

comment:25

Everything looks good and works well.

jdemeyer commented 11 years ago

comment:26

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 commented 11 years ago

comment:27

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 commented 11 years ago

comment:28

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 commented 11 years ago

comment:29

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 commented 11 years ago

comment:30

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 commented 11 years ago

comment:31

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.

ohanar commented 11 years ago

comment:32

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

ohanar commented 11 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,5 @@
 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:
-* [attachment: 13031-cythonize-simple.patch](https://github.com/sagemath/sage-prod/files/10655648/13031-cythonize-simple.patch.gz)
+* [attachment: trac13031-cythonize-simple.patch](https://github.com/sagemath/sage-prod/files/10655652/trac13031-cythonize-simple.patch.gz)
+* [attachment: trac13031-mutability.patch](https://github.com/sagemath/sage/files/ticket13031/trac13031-mutability.patch.gz)

ohanar commented 11 years ago

remove unused mutablity code

ohanar commented 11 years ago

comment:34

Attachment: trac13031-mutablity.patch.gz

ohanar commented 11 years ago

Description changed:

--- 
+++ 
@@ -2,4 +2,4 @@

 Apply:
 * [attachment: trac13031-cythonize-simple.patch](https://github.com/sagemath/sage-prod/files/10655652/trac13031-cythonize-simple.patch.gz)
-* [attachment: trac13031-mutability.patch](https://github.com/sagemath/sage/files/ticket13031/trac13031-mutability.patch.gz)
+* [attachment: trac13031-mutablity.patch](https://github.com/sagemath/sage-prod/files/10655649/trac13031-mutablity.patch.gz)

jdemeyer commented 11 years ago

comment:35

Replying to @ohanar:

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 commented 11 years ago

comment:36

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.

ohanar commented 11 years ago

comment:37

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 commented 11 years ago

Attachment: trac13031-cythonize-simple-5.8.beta4.patch.gz

robertwb commented 11 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,6 @@
 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:
-* [attachment: trac13031-cythonize-simple.patch](https://github.com/sagemath/sage-prod/files/10655652/trac13031-cythonize-simple.patch.gz)
+* [attachment: trac13031-cythonize-simple-5.8.beta4.patch](https://github.com/sagemath/sage-prod/files/10655650/trac13031-cythonize-simple-5.8.beta4.patch.gz)
 * [attachment: trac13031-mutablity.patch](https://github.com/sagemath/sage-prod/files/10655649/trac13031-mutablity.patch.gz)
+* [attachment: trac13031-version.patch](https://github.com/sagemath/sage/files/ticket13031/trac13031-version.patch.gz)

robertwb commented 11 years ago

comment:38

Attachment: trac13031-cythonize-version.patch.gz

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

robertwb commented 11 years ago

comment:39

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

sagemath / sage

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