#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
This needs two upstream patches, one which is accepted in master and api_mangle.patch
We also clean up where possible. In particular:
- drop support for i386 CPUs without MMX.
- remove
set_sage_signal_handler_message()
which is no longer used and not needed sincesig_error()
.
Change History (44)
comment:1 Changed 6 years ago by
- Branch set to u/jdemeyer/move_interrupts_to_cython
comment:2 Changed 6 years ago by
- Commit set to 26c3b8bb5553853161550a35e330860e740f1ecd
comment:3 Changed 6 years ago by
- Dependencies changed from #17851, #18007 to #17851, #18007, #18030
comment:4 Changed 6 years ago by
- Commit changed from 26c3b8bb5553853161550a35e330860e740f1ecd to 2e3c1d9d0f97bce616d5831f7b29a79ffc7b2408
comment:5 Changed 6 years ago by
- Commit changed from 2e3c1d9d0f97bce616d5831f7b29a79ffc7b2408 to 5fd2ab5d6a612693d95c59adcc41a29926f742b8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5fd2ab5 | Move interrupts to Cython
|
comment:6 Changed 6 years ago by
- Description modified (diff)
- Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:7 Changed 6 years ago by
- Description modified (diff)
comment:8 Changed 6 years ago by
- Commit changed from 5fd2ab5d6a612693d95c59adcc41a29926f742b8 to 72c18cddc5de380e52b8d3fe1ad7cdf1cf329b65
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
72c18cd | Move interrupts to Cython
|
comment:9 Changed 6 years ago by
- Commit changed from 72c18cddc5de380e52b8d3fe1ad7cdf1cf329b65 to 82ecadac9a24d222e017a1064732a72d83e18edd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
82ecada | Move interrupts to Cython
|
comment:10 Changed 6 years ago by
- Dependencies changed from #17851, #18007, #18030 to #17851, #18007, #18030, #18037
comment:11 Changed 6 years ago by
- Commit changed from 82ecadac9a24d222e017a1064732a72d83e18edd to f5efcfc0484b66e55c788691cbfef86c05bb8285
comment:12 Changed 6 years ago by
- Description modified (diff)
comment:13 Changed 6 years ago by
- Commit changed from f5efcfc0484b66e55c788691cbfef86c05bb8285 to a1c8ae2b41585186b0859da8fa6b3931ccdf01ee
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a1c8ae2 | Move interrupts to Cython
|
comment:14 Changed 6 years ago by
- Status changed from new to needs_review
comment:15 Changed 6 years ago by
- Description modified (diff)
comment:16 Changed 6 years ago by
(NOTE: despite the red link in the Trac interface, the branch merges cleanly)
comment:17 Changed 6 years ago by
- Commit changed from a1c8ae2b41585186b0859da8fa6b3931ccdf01ee to 468332c1763303337312354da81762c3c3296a95
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ba7c496 | Merge commit 'c98c51a982ee78488a5ff2b01ffe5dfb996ef969' into t/18030/ticket/18030
|
8d18d58 | Add includes to cryptominisat.pyx
|
71c60ac | Merge commit 'e56aeb2c9a190813096a0f7db1683d3d5bc774d1' into t/18030/ticket/18030
|
b55b277 | Add missing includes
|
00db4b2 | Merge tag '6.7.beta0' into t/18030/ticket/18030
|
468332c | Move interrupts to Cython
|
comment:18 Changed 6 years ago by
- Dependencies changed from #17851, #18007, #18030, #18037 to #18030
comment:19 Changed 6 years ago by
- Cc jpflori added
comment:20 Changed 6 years ago by
- Commit changed from 468332c1763303337312354da81762c3c3296a95 to bd877b899103fa693266a1a55140d1d5454a2d45
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bd877b8 | Move interrupts to Cython
|
comment:21 Changed 6 years ago by
Rebased to 6.7.beta2.
comment:22 Changed 6 years ago by
- 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 6 years ago by
- Commit changed from bd877b899103fa693266a1a55140d1d5454a2d45 to 68c43afcf922bb5cd776623a0222d289b4d0223e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
68c43af | Move interrupts to Cython
|
comment:24 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:25 Changed 6 years ago by
- Dependencies #18030 deleted
comment:26 Changed 6 years ago by
- Commit changed from 68c43afcf922bb5cd776623a0222d289b4d0223e to 6d738d6e869b869ccaf68a5db6a166542647dd7d
Branch pushed to git repo; I updated commit sha1. New commits:
6d738d6 | Merge tag '6.7.beta3' into HEAD
|
comment:27 follow-up: ↓ 28 Changed 6 years ago by
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 6 years ago by
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: ↓ 30 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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: ↓ 33 Changed 6 years ago by
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 6 years ago by
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 includinginterrupts.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: ↓ 35 Changed 6 years ago by
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 6 years ago by
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: ↓ 39 Changed 6 years ago by
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 6 years ago by
- Reviewers set to Jean-Pierre Flori
- Status changed from needs_review to positive_review
comment:38 Changed 6 years ago by
- 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 6 years ago by
- 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 6 years ago by
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 6 years ago by
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 6 years ago by
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.
comment:43 follow-up: ↓ 44 Changed 6 years ago by
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 6 years ago by
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.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Disable broken Cython caching
Merge commit 'ce992d2ac5a68a4dd7328dc088f68007e4b5e4cd'; commit 'ff8b9fd18562be12020ce35477e05044374d5683' into HEAD
Add upstreamed Cython patch
Move interrupts to Cython