Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#18027 closed enhancement (fixed)

Move interrupts to Cython

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.6
Component: cython Keywords:
Cc: jpflori Merged in:
Authors: Jeroen Demeyer Reviewers: Jean-Pierre Flori
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: 6d738d6 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This needs two upstream patches, one which is accepted in master and api_mangle.patch

We also clean up where possible. In particular:

  1. drop support for i386 CPUs without MMX.
  2. remove set_sage_signal_handler_message() which is no longer used and not needed since sig_error().

Change History (44)

comment:1 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/move_interrupts_to_cython

comment:2 Changed 5 years ago by git

  • Commit set to 26c3b8bb5553853161550a35e330860e740f1ecd

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ff8b9fdDisable broken Cython caching
6b95de6Merge commit 'ce992d2ac5a68a4dd7328dc088f68007e4b5e4cd'; commit 'ff8b9fd18562be12020ce35477e05044374d5683' into HEAD
9727fd0Add upstreamed Cython patch
26c3b8bMove interrupts to Cython

comment:3 Changed 5 years ago by jdemeyer

  • Dependencies changed from #17851, #18007 to #17851, #18007, #18030

comment:4 Changed 5 years ago by git

  • Commit changed from 26c3b8bb5553853161550a35e330860e740f1ecd to 2e3c1d9d0f97bce616d5831f7b29a79ffc7b2408

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9aadb70Clean-up stdsage.pxi includes
5137c4bDirectly cimport PY_NEW
bd606a0Merge commit 'ff8b9fd18562be12020ce35477e05044374d5683' into HEAD
2e3c1d9Move interrupts to Cython

comment:5 Changed 5 years ago by git

  • Commit changed from 2e3c1d9d0f97bce616d5831f7b29a79ffc7b2408 to 5fd2ab5d6a612693d95c59adcc41a29926f742b8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5fd2ab5Move interrupts to Cython

comment:6 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:7 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:8 Changed 5 years ago by git

  • Commit changed from 5fd2ab5d6a612693d95c59adcc41a29926f742b8 to 72c18cddc5de380e52b8d3fe1ad7cdf1cf329b65

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

72c18cdMove interrupts to Cython

comment:9 Changed 5 years ago by git

  • Commit changed from 72c18cddc5de380e52b8d3fe1ad7cdf1cf329b65 to 82ecadac9a24d222e017a1064732a72d83e18edd

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

82ecadaMove interrupts to Cython

comment:10 Changed 5 years ago by jdemeyer

  • Dependencies changed from #17851, #18007, #18030 to #17851, #18007, #18030, #18037

comment:11 Changed 5 years ago by git

  • Commit changed from 82ecadac9a24d222e017a1064732a72d83e18edd to f5efcfc0484b66e55c788691cbfef86c05bb8285

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b8ed24aUse alarm() instead of interrupt_after_delay()
43d96bcMerge dependencies
f5efcfcMove interrupts to Cython

comment:12 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 5 years ago by git

  • Commit changed from f5efcfc0484b66e55c788691cbfef86c05bb8285 to a1c8ae2b41585186b0859da8fa6b3931ccdf01ee

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a1c8ae2Move interrupts to Cython

comment:14 Changed 5 years ago by jdemeyer

  • Status changed from new to needs_review

comment:15 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 5 years ago by jdemeyer

(NOTE: despite the red link in the Trac interface, the branch merges cleanly)

comment:17 Changed 5 years ago by git

  • Commit changed from a1c8ae2b41585186b0859da8fa6b3931ccdf01ee to 468332c1763303337312354da81762c3c3296a95

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ba7c496Merge commit 'c98c51a982ee78488a5ff2b01ffe5dfb996ef969' into t/18030/ticket/18030
8d18d58Add includes to cryptominisat.pyx
71c60acMerge commit 'e56aeb2c9a190813096a0f7db1683d3d5bc774d1' into t/18030/ticket/18030
b55b277Add missing includes
00db4b2Merge tag '6.7.beta0' into t/18030/ticket/18030
468332cMove interrupts to Cython

comment:18 Changed 5 years ago by jdemeyer

  • Dependencies changed from #17851, #18007, #18030, #18037 to #18030

comment:19 Changed 5 years ago by jpflori

  • Cc jpflori added

comment:20 Changed 5 years ago by git

  • Commit changed from 468332c1763303337312354da81762c3c3296a95 to bd877b899103fa693266a1a55140d1d5454a2d45

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bd877b8Move interrupts to Cython

comment:21 Changed 5 years ago by jdemeyer

Rebased to 6.7.beta2.

comment:22 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I'm simplifying the changes to src/module_list.py and src/setup.py

comment:23 Changed 5 years ago by git

  • Commit changed from bd877b899103fa693266a1a55140d1d5454a2d45 to 68c43afcf922bb5cd776623a0222d289b4d0223e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

68c43afMove interrupts to Cython

comment:24 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:25 Changed 5 years ago by jdemeyer

  • Dependencies #18030 deleted

comment:26 Changed 5 years ago by git

  • Commit changed from 68c43afcf922bb5cd776623a0222d289b4d0223e to 6d738d6e869b869ccaf68a5db6a166542647dd7d

Branch pushed to git repo; I updated commit sha1. New commits:

6d738d6Merge tag '6.7.beta3' into HEAD

comment:27 follow-up: Changed 5 years ago by jpflori

Do you have any other reason to remove x86 CPUs without MMX support execpt that it is very old? I guess some other parts of sage might not work on such CPU anyway, do you have any hint? I at least remember that our ATLAS base configuration requires an FPU or even MMX.

comment:28 in reply to: ↑ 27 Changed 5 years ago by jdemeyer

Replying to jpflori:

Do you have any other reason to remove x86 CPUs without MMX support execpt that it is very old?

The reason is to simplify the code.

I guess some other parts of sage might not work on such CPU anyway, do you have any hint?

No idea...

comment:29 follow-up: Changed 5 years ago by jpflori

I think it's dangerous to use stuff from the

#include "build/cythonized/sage/ext/interrupt/interrupt_api.h"

On top of that, it occurs in a file meant to be included itsef as soon as one wants to use signal related stuff.

comment:30 in reply to: ↑ 29 Changed 5 years ago by jdemeyer

Replying to jpflori:

I think it's dangerous to use stuff from the

#include "build/cythonized/sage/ext/interrupt/interrupt_api.h"

Why dangerous? These _api.h files are a standard and documented Cython mechanism to use Cython functions from C code: http://docs.cython.org/src/userguide/external_C_code.html#c-api-declarations

comment:31 Changed 5 years ago by jdemeyer

To explain the usage of interrupt_api.h better: the global variable _signals (and some functions like _sig_on_interrupt_received) which used to be implemented in c_lib are now implemented in Cython. For technical reasons, sig_on() MUST be a macro and therefore it can only reasonably be implemented in a .h file. But sig_on() needs access to _signals. So we need a mechanism to access the Cython global variable _signals in the C header macros.h. We use the standard cdef api mechanism for this.

comment:32 follow-up: Changed 5 years ago by jpflori

My only issue is the place where this header lives: build/. What if I decide to empty this directory and then recompile a file including interrupts.pxi?

comment:33 in reply to: ↑ 32 Changed 5 years ago by jdemeyer

Replying to jpflori:

My only issue is the place where this header lives: build/. What if I decide to empty this directory and then recompile a file including interrupts.pxi?

Cython generates the file interrupt_api.h. If you empty the build directory, the file will be regenerated just like all the other files in the build directory.

comment:34 follow-up: Changed 5 years ago by jpflori

Ah, in that case it is fine. I for no reason though it was only generated when interrupt.pyx was cythonized.

comment:35 in reply to: ↑ 34 Changed 5 years ago by jdemeyer

Replying to jpflori:

I for no reason though it was only generated when interrupt.pyx was cythonized.

You are right, it is only generated when interrupt.pyx is Cythonized.

That's not a problem however: if you delete the build directory, then interrupt.pyx will be Cythonized and the file will be generated.

comment:36 follow-up: Changed 5 years ago by jpflori

Yes indeed, and I don't like this Cython behavior. You have to keep the build directory alive to avoid rebuilding everything.

Anyway, I'll leave with it for this ticket.

comment:37 Changed 5 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to positive_review

comment:38 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/move_interrupts_to_cython to 6d738d6e869b869ccaf68a5db6a166542647dd7d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:39 in reply to: ↑ 36 Changed 5 years ago by jdemeyer

  • Commit 6d738d6e869b869ccaf68a5db6a166542647dd7d deleted

Replying to jpflori:

Yes indeed, and I don't like this Cython behavior. You have to keep the build directory alive to avoid rebuilding everything.

I really don't understand your problem.

Deleting the src/build directory in Sage is the same as make clean in a typical software package. And when you do make clean, you have to rebuild everything if you do make later.

And when you don't delete the build directory, Cython+distutils has proper dependency checking: only needed files are rebuilt.

comment:40 Changed 5 years ago by fbissey

I'll surely open a follow up ticket about that build directory question once I have figured out to deal with it. I am not completely sure you have totally broken all "global" sage install and whether or not making those install carry at least a fragment of the build folder will fix it properly. Who else needs to have their build folder kept? It joins a basket of issues that I have duct-taped in sage-on-gentoo for years to have doctest and spyx files to work properly. If I had spent more time on fixing the installation of sage headers and various auxiliary files rather than the duct tape we wouldn't be there today.

That's a sample

    1 item had failures:
       1 of   4 in sage.doctest.tests.sig_on
        [2 tests, 1 failure, ...]
    ----------------------------------------------------------------------
    sage -t --warn-long 0.0 sig_on.rst  # 1 doctest failed
    ----------------------------------------------------------------------
    ...
    1
Got:
    Running doctests with ID 2015-05-24-22-01-48-66399ec6.
    Doctesting 1 file.
    sage -t --warn-long 0.0 sig_on.rst
    **********************************************************************
    File "sig_on.rst", line 3, in sage.doctest.tests.sig_on
    Failed example:
        cython('sig_on()')
    Exception raised:
        Traceback (most recent call last):
          File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
            self.compile_and_execute(example, compiler, test.globs)
          File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
            exec(compiled, globs)
          File "<doctest sage.doctest.tests.sig_on[0]>", line 1, in <module>
            cython('sig_on()')
          File "sage/misc/cython_c.pyx", line 63, in sage.misc.cython_c.cython (build/cythonized/sage/misc/cython_c.c:949)
            sage.server.support.cython_import_all(tmpfile, globals(),
          File "/usr/lib64/python2.7/site-packages/sage/server/support.py", line 85, in cython_import_all
            create_local_c_file=create_local_c_file)
          File "/usr/lib64/python2.7/site-packages/sage/server/support.py", line 62, in cython_import
            **kwds)
          File "/usr/lib64/python2.7/site-packages/sage/misc/cython.py", line 519, in cython
            raise RuntimeError("Error compiling {}:\n{}\n{}".format(filename, log, err))
        RuntimeError: Error compiling /home/fbissey/.sage/temp/QCD-nzi3/10231/tmp_FdPxlK.spyx:
        running build
        running build_ext
        building '_home_fbissey__sage_temp_QCD_nzi3_10231_tmp_FdPxlK_spyx_0' extension
        creating build
        creating build/temp.linux-x86_64-2.7
        x86_64-pc-linux-gnu-gcc -pthread -fPIC -I/usr/include/csage -I/usr/include -I/usr/include/python2.7 -I/usr/lib/python/site-packages/numpy/core/include -I/usr/share/sage/src/sage/ext -I/usr/share/sage/src -I/usr/share/sage/src/sage/gsl -I/home/fbissey/.sage/temp/QCD-nzi3/10231 -I/usr/include/python2.7 -c _home_fbissey__sage_temp_QCD_nzi3_10231_tmp_FdPxlK_spyx_0.c -o build/temp.linux-x86_64-2.7/_home_fbissey__sage_temp_QCD_nzi3_10231_tmp_FdPxlK_spyx_0.o -w -O2
    <BLANKLINE>
        In file included from _home_fbissey__sage_temp_QCD_nzi3_10231_tmp_FdPxlK_spyx_0.c:248:0:
        /usr/share/sage/src/sage/ext/interrupt/pxi.h:7:63: fatal error: build/cythonized/sage/ext/interrupt/interrupt_api.h: No such file or directory
         #include "build/cythonized/sage/ext/interrupt/interrupt_api.h"
                                                                       ^
        compilation terminated.
        error: command 'x86_64-pc-linux-gnu-gcc' failed with exit status 1
    <BLANKLINE>
    **********************************************************************
    1 item had failures:
       1 of   4 in sage.doctest.tests.sig_on
        [2 tests, 1 failure, 0.86 s]
    ----------------------------------------------------------------------
    sage -t --warn-long 0.0 sig_on.rst  # 1 doctest failed
    ----------------------------------------------------------------------
    Total time for all tests: 0.9 seconds
        cpu time: 0.0 seconds
        cumulative wall time: 0.9 seconds
    1
**********************************************************************
1 item had failures:
   1 of  45 in sage.doctest.test
    [44 tests, 1 failure, 98.40 s]
----------------------------------------------------------------------
sage -t --long /usr/share/sage/src/sage/doctest/test.py  # 1 doctest failed

comment:41 Changed 5 years ago by jdemeyer

I understand your problem now: you need the build folder for cython() to work properly (why didn't you just tell me this?)

comment:42 Changed 5 years ago by fbissey

To be honest, while I saw the ticket yesterday and suspected problems, I only found the extent a few hours ago. I cooled off a little bit before commenting, possibly not enough.

Last edited 5 years ago by fbissey (previous) (diff)

comment:43 follow-up: Changed 5 years ago by fbissey

But it is a symptom for a larger problem sage keep a lot of stuff in the sources that should really be installed. Headers and a few c/cpp files here and there. I had that conversation with Burcin a long time ago. I have been going on with shipping a copy of the sources in sage-on-gentoo and I probably could try to ship that single file but it should be just a stop gap to better solution.

comment:44 in reply to: ↑ 43 Changed 5 years ago by jdemeyer

Replying to fbissey:

But it is a symptom for a larger problem sage keep a lot of stuff in the sources that should really be installed. Headers and a few c/cpp files here and there.

And .pxd/.pxi files also I guess...

It wouldn't be too hard to install all needed files, it's just that somebody has to do it.

Note: See TracTickets for help on using tickets.