support extra_compile_args (e.g., C99) when loading/attaching .pyx (cython) files, and when using %cython in the notebook

[was]

Right now, the following file foo.pyx cannot be just loaded into Sage:

from sage.rings.rational cimport Rational
from sage.rings.polynomial.polynomial_rational_flint cimport Polynomial_rational_flint
from sage.libs.flint.fmpq_poly cimport (fmpq_poly_get_coeff_mpq, fmpq_poly_set_coeff_mpq,
                                        fmpq_poly_length)

def evaluate_at_power_of_gen(Polynomial_rational_flint f, unsigned long n):
    assert n >= 1
    cdef Polynomial_rational_flint res = f._new()
    cdef unsigned long k
    cdef Rational z = Rational(0)
    for k in range(fmpq_poly_length(f.__poly)):
        fmpq_poly_get_coeff_mpq(z.value, f.__poly, k)
        fmpq_poly_set_coeff_mpq(res.__poly, n*k, z.value)
    return res

The main reason is that there is no way to tell Sage (i.e., the file cython.py) that the code needs to have the extra compile flag:

              extra_compile_args = ['-std=c99'],

Currently devel/sage/sage/misc/cython.py supports "clang", "clib", and "cinclude" pragmas. But none of these let us add an extra_compile_arg or use C99.

---

Apply trac_11680.patch to the Sage library.

[leif]

So what do you propose?

Since c99 is actually also a program (and could be considered a language of its own), I'd suggest to both support

#clang c99

and some other way to specify "flags" or compile-time options, perhaps also to the linker.

AFAIK there's currently not even a way to specify cython options in a .pyx file.

Allowing / using strings that get passed literally to the compiler driver (and perhaps also directly to the linker) is very flexible, but the Cython files may of course get less portable (which is perhaps a minor issue for the Sage library, at least at the moment).

---

What Cython needs, too, is some kind of conditional compilation (à la #ifdef etc.) anyway... ;-)

[was]

leif -- I have no proposal, I just want *something*, anything, that actually works, so I can get back to work.

[leif]

Replying to was:

leif -- I have no proposal, I just want *something*, anything, that actually works, so I can get back to work.

Do you need or imagine any other "generic" options (other than for compiling as C99 code) which you'd rather like to specify "non-verbatim", i.e., without using a string to be directly passed to e.g. gcc?

[leif]

In addition to

#clang C99

I could imagine having

#coptions keyword1 keyword2 keyword3

which are then mapped to the appropriate flags and passed to their respective component (cython, the C or C++ compiler, or the linker).

This should be quite flexible, since changes to the flags effectively used (e.g for different compilers or compiler versions) would be limited to sage.misc.cython*.

I don't know if we'd in addition need something like

#cpragma "CFLAGS" "-D_XPG6 --some-very-rare-and-specific-options another_argument"
#cpragma "LDFLAGS" "-L/unusual/location"

etc.

[leif]

Replying to leif:

I don't know if we'd in addition need something like

#cpragma "CFLAGS" "-D_XPG6 --some-very-rare-and-specific-options another_argument"
#cpragma "LDFLAGS" "-L/unusual/location"

etc.

We could even extend this to conditionals (e.g. depending on the operating system):

#cpragma "CFLAGS" SunOS, HP-UX ? "-D_XPG6" : ""

where SunOS etc. are arbitrary predefined predicates.

(Using && and || there would perhaps be more appropriate, besides the ternary ? : expression. ! for negation shouldn't be necessary, though also desirable.)

[leif]

Just noticed sage/misc/cython.py is more complicated than I expected (and not fully up-to-date, e.g. w.r.t. cython's --cplus option, which isn't used yet), but adding at least support for C99 should be easy.

Will give it a try...

Any nice Cython examples to test with?

[malb]

test file to demonstrate behaviour

[malb]

Hi, I've addeded cflags which are just passed to extra_compile_args. I only fixed enough things in cython.py such that the attached test.spyx works, e.g. I didn't fix adding --cplus etc.

[leif]

Hmmm, the naming isn't very clear.

I'd use some plural form for cfile, i.e. cfiles, or something else, e.g. csources, which clearly indicates that these are additional, "arbitrary" source files to also be compiled into the extension module.

cargs is similarly ambiguous, as the c refers to Cython, not C, but the "args" are passed to the C or C++ compiler rather than to cython itself.

(I admit that clib and cinclude could perhaps have better names, or at least the plural form, as well; especially for the latter it isn't clear that its parameters are or can be actually search paths.)

For compiling C99, I still prefer having #clang C99 (case-insensitive for all languages btw.).

P.S.: Now I'm happy I didn't also work on this yesterday... ;-)

[was]

1. The code looks great!

2. There are no tests at all of the new functionality. Add tests somehow.

3. TODO Later: Make it so typing cython? results in one seeing documentation for all pragma's. This is now #11712.

[malb]

Replying to was:

2. There are no tests at all of the new functionality. Add tests somehow.

Done, I added only one test, though. It seems sufficient.

[malb]

Replying to leif:

Hmmm, the naming isn't very clear.

Agreed.

I'd use some plural form for cfile, i.e. cfiles, or something else, e.g. csources, which clearly indicates that these are additional, "arbitrary" source files to also be compiled into the extension module.

Those weren't added in this ticket.

cargs is similarly ambiguous, as the c refers to Cython, not C, but the "args" are passed to the C or C++ compiler rather than to cython itself.

Yep, the whole cprefix thingy is ambiguous. Perhaps there should be another ticket where the whole system is overhauled?

(I admit that clib and cinclude could perhaps have better names, or at least the plural form, as well; especially for the latter it isn't clear that its parameters are or can be actually search paths.)

These weren't added in this ticket.

For compiling C99, I still prefer having #clang C99 (case-insensitive for all languages btw.).

I have no preference either way. GCC treats it as an option. Hence, I think it's okay to have it as part of cargs.

P.S.: Now I'm happy I didn't also work on this yesterday... ;-)

But you could work on a more general ticket tomorrow ;-)

[was]

I was refereeing the patch and fixed a doctest failure in it (?). Then I decided I wanted that .pyx file in my original input in a doctest, and setup infrastructure to do that, and did it. So the new patch (which is merged with yours, with both our names) is longer than your original patch. Since I'm happy with yours, if you (=malb) can review this (especially my new code), then it is reviewed.

[malb]

Patch looks good but I wonder whether we should support replacing <SAGE_ROOT> with SAGE_ROOT not only in tests but in general. It'd make pyx files more portable.

[malb]

I am happy with this version if you are.

[was]

Replying to malb:

I am happy with this version if you are.

Yep. Positive review :-)

Regarding $SAGE_ROOT, let's do that for sure, but as another ticket.

[robertwb]

When #11761 gets approved, we can move using # distutils: language = c++ which is understood by Cython and can be used to specify any Extension options. See  http://wiki.cython.org/enhancements/distutils_preprocessing

[malb]

Shouldn't we make this ticket depend on #11761 then and adapt it accordingly? It doesn't seem to be a good idea to change the syntax that often?

[leif]

Replying to malb:

Shouldn't we make this ticket depend on #11761 then and adapt it accordingly? It doesn't seem to be a good idea to change the syntax that often?

IMHO not until #11761 has positive review. (Otherwise we'd need a negative dependence, invalidating this ticket when #11761 gets merged.)

You can open a follow-up ticket based on #11761 though, changing the syntax used here.

Or change the syntax used here, and don't make this ticket depend on Cython 0.15... ;-)

[jhpalmieri]

The patch here seems to be leaving some .c and .html files lying around during doctests. I think we can fix this with this:

sage/misc/cython.py

@@ -17 +17 @@
 
 import os, sys, platform
 
-from misc import SPYX_TMP, SAGE_ROOT, SAGE_LOCAL
+from misc import SPYX_TMP, SAGE_ROOT, SAGE_LOCAL, SAGE_TMP
 from sage.misc.misc import UNAME
 
 def cblas():
@@ -627 +627 @@
     file = tmp_filename() + ".pyx"
     open(file,'w').write(code)
     from sage.server.support import cython_import
+    os.chdir(SAGE_TMP)
     return cython_import(file)

Is this a good idea? Can anyone confirm that the patch here is responsible for those files? If so, we should open up a follow-up ticket.

[leif]

Revisiting this, I don't think the TESTS dictionary and the import_test() function should be part of the module itself at all, as they provide no functionality. (I actually reviewed an earlier version of the patch.)

They should either be part of the docstring as an -- of course longer -- example, or somewhere in sage/tests/.

W.r.t. the temporary files:

I think it is a more general problem that Cython apparently puts temporary files into the current working directory, just mangling the original path, and, secondly, not deleting them afterwards.

I'm pretty sure I've seen similar files sporadically occurring without ever getting deleted in previous releases (i.e., e.g. last year).

[leif]

P.S.: But John's suggestion is IMHO -- perhaps as a temporary fix only -- ok here (in that it should fix the issue in an easy way), since using sage.server.support.cython_import() for this purpose is simply an abuse of that function... ;-)

Maybe we should also use create_local_c_file=False; I guess that also suppresses the HTML counterpart.

[leif]

Replying to leif:

Maybe we should also use create_local_c_file=False; I guess that also suppresses the HTML counterpart.

FWIW, the following fixes the issue for me, without changing the current working directory:

sage/misc/cython.py

@@ -627 +627 @@
     file = tmp_filename() + ".pyx"
     open(file,'w').write(code)
     from sage.server.support import cython_import
-    return cython_import(file)
+    # return cython_import(file, create_local_c_file=False, annotate=False)
+    return cython_import(file, create_local_c_file=False)
 
 
 TESTS = {

The line I've commented out does not work, since cython_import() doesn't propagate the annotate keyword to sage.misc.cython.cython() (i.e., it doesn't take such a keyword argument), which is perhaps a bug by itself.

However, using just create_local_c_file=False is sufficient.

[jhpalmieri]

We definitely want create_local_c_file=False, not just annotate=False, because there are c files being created as well. I've created #11887 to deal with this.

[leif]

Replying to jhpalmieri:

We definitely want create_local_c_file=False, not just annotate=False

I meant both of course, but disabling the former apparently also disables the latter.