Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#11680 closed enhancement (fixed)

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

Reported by: was Owned by: jason
Priority: minor Milestone: sage-4.7.2
Component: misc Keywords: sd32
Cc: robertwb Merged in: sage-4.7.2.alpha3
Authors: Martin Albrecht Reviewers: William Stein, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

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.

Attachments (2)

test.spyx (837 bytes) - added by malb 3 years ago.
test file to demonstrate behaviour
trac_11680.patch (8.5 KB) - added by malb 3 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 3 years ago by leif

  • Cc robertwb added

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

comment:2 follow-up: Changed 3 years ago by was

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

comment:3 in reply to: ↑ 2 Changed 3 years ago by 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?

comment:4 follow-up: Changed 3 years ago by 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.

comment:5 in reply to: ↑ 4 Changed 3 years ago by 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.)

comment:6 Changed 3 years ago by 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?

Changed 3 years ago by malb

test file to demonstrate behaviour

comment:7 Changed 3 years ago by malb

  • Authors set to Martin Albrecht
  • Status changed from new to needs_review

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.

comment:8 follow-up: Changed 3 years ago by 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... ;-)

comment:9 follow-up: Changed 3 years ago by was

  1. The code looks great!
  1. There are no tests at all of the new functionality. Add tests somehow.
  1. TODO Later: Make it so typing cython? results in one seeing documentation for all pragma's. This is now #11712.

comment:10 in reply to: ↑ 9 Changed 3 years ago by malb

Replying to was:

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

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

comment:11 in reply to: ↑ 8 Changed 3 years ago by 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 ;-)

comment:12 Changed 3 years ago by 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.

comment:13 Changed 3 years ago by 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.

Changed 3 years ago by malb

comment:14 follow-up: Changed 3 years ago by malb

I am happy with this version if you are.

comment:15 in reply to: ↑ 14 Changed 3 years ago by 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.

comment:16 Changed 3 years ago by was

  • Status changed from needs_review to positive_review

comment:17 Changed 3 years ago by was

  • Keywords sd32 added

comment:18 Changed 3 years ago by 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

comment:19 follow-up: Changed 3 years ago by 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?

comment:20 in reply to: ↑ 19 Changed 3 years ago by leif

  • Reviewers set to William Stein, Leif Leonhardy

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

comment:21 Changed 3 years ago by leif

  • Description modified (diff)

comment:22 Changed 3 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:23 Changed 3 years ago by 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

    diff --git a/sage/misc/cython.py b/sage/misc/cython.py
    a b AUTHORS: 
    1717 
    1818import os, sys, platform 
    1919 
    20 from misc import SPYX_TMP, SAGE_ROOT, SAGE_LOCAL 
     20from misc import SPYX_TMP, SAGE_ROOT, SAGE_LOCAL, SAGE_TMP 
    2121from sage.misc.misc import UNAME 
    2222 
    2323def cblas(): 
    def compile_and_load(code): 
    627627    file = tmp_filename() + ".pyx" 
    628628    open(file,'w').write(code) 
    629629    from sage.server.support import cython_import 
     630    os.chdir(SAGE_TMP) 
    630631    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.

comment:24 Changed 3 years ago by 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).

comment:25 follow-up: Changed 3 years ago by 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.

comment:26 in reply to: ↑ 25 Changed 3 years ago by 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

    diff --git a/sage/misc/cython.py b/sage/misc/cython.py
    a b  
    627627    file = tmp_filename() + ".pyx" 
    628628    open(file,'w').write(code) 
    629629    from sage.server.support import cython_import 
    630     return cython_import(file) 
     630    # return cython_import(file, create_local_c_file=False, annotate=False) 
     631    return cython_import(file, create_local_c_file=False) 
    631632 
    632633 
    633634TESTS = { 

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.

comment:27 follow-up: Changed 3 years ago by 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.

comment:28 in reply to: ↑ 27 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.