Opened 6 years ago
Closed 6 years ago
#16901 closed defect (fixed)
cythonized function in combinatorial designs must be interruptible
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  combinatorial designs  Keywords:  
Cc:  ncohen  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Nathann Cohen 
Report Upstream:  N/A  Work issues:  
Branch:  6097260 (Commits)  Commit:  6097260461d3d20653d3406db362b4e9c9a8b40c 
Dependencies:  #16879  Stopgaps: 
Description
Right now, we can not interrupt MOLS_table
... this is a consequence of the intensive cythonization.
sage: sage: from sage.combinat.designs.latin_squares import MOLS_table sage: MOLS_table(10000) 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 ________________________________________________________________________________ 0 +oo +oo 1 2 3 4 1 6 7 8 2 10 5 12 4 4 15 16 5 18 20 4 5 3 22 7 24 4 26 5 28 4 30 31 5 4 5 8 36 4 5 40 7 40 5 42 5 6 4 46 8 48 6 5 5 52 5 6 7 7 5 58 ^CException KeyboardInterrupt: KeyboardInterrupt() in 'sage.combinat.designs.orthogonal_arrays_find_recursive.is_available' ignored 360 7 360 6 7 7 7 6 366 15 15 7 15 7 372 7 15 7 13 7 378 380 7 12 7 382 15 15 7 15 7 388 7 16 7 7 7 7 8 396 7 7 400 15 400 7 15 11 8 7 15 8 408 7 13 8 12 10 9 18 15 7 418 ^CException KeyboardInterrupt: KeyboardInterrupt() in 'sage.combinat.designs.orthogonal_arrays_find_recursive.is_available' ignored ...
We need more sig_on()
, sig_off()
.
Change History (30)
comment:1 Changed 6 years ago by
 Branch set to u/vdelecroix/16901
 Commit set to 00f4fdb067d61ec76b87efb0f250a4937bda9046
comment:2 Changed 6 years ago by
 Dependencies set to #16879
comment:3 Changed 6 years ago by
 Status changed from new to needs_review
comment:4 Changed 6 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_review to positive_review
Yo !
Seems to do the job, thanks !
 return orthogonal_array(k,n,existence=True) + ans = orthogonal_array(k,n,existence=True) + return ans
Is return orthogonal_array(k,n,existence=True)
declared politically incorrect now ? :P
Nathann
P.S. : the commit's ticket contains the wrong number but we probably won't base anything on this branch so it is not a problem.
comment:5 Changed 6 years ago by
 Status changed from positive_review to needs_work
The messages in the ticket description
^CException KeyboardInterrupt: KeyboardInterrupt() in 'sage.combinat.designs.orthogonal_arrays_find_recursive.is_available' ignored
show that the issue is not just sig_on()
/sig_off()
but also missing except
clauses in cdef
functions, see also #16233 for how to fix this.
Also, your codes mixes Cython and Python, so I would go with sig_check() in the inner loops which is much safer than sig_on()
and sig_off()
.
comment:6 Changed 6 years ago by
Perhaps the warning at http://www.sagemath.org/doc/developer/coding_in_cython.html#usingsigonandsigoff is not clear enough, but really the code inside sig_on() should be pure C or Cython code.
comment:7 followup: ↓ 8 Changed 6 years ago by
This ticket inspired me to create #16902.
comment:8 in reply to: ↑ 7 ; followup: ↓ 9 Changed 6 years ago by
Replying to jdemeyer:
This ticket inspired me to create #16902.
Thanks, I am carefully reading it!
By the way, I got something very strange here. find_recursive_construction
is a cached function (with the decorator in sage.misc.cachefunc
). It appears that after some of the interruption, the CachedFunction
class suddenly decided that the function now takes 3 arguments instead of 2... and trying to call it with 3 values instead of 2 I just got an unhandled SIGSEGV.
Might be something subtle going on with exception and cache but I do not know why. I can describe the case in more details.
Vincent
comment:9 in reply to: ↑ 8 Changed 6 years ago by
Replying to vdelecroix:
By the way, I got something very strange here.
find_recursive_construction
is a cached function (with the decorator insage.misc.cachefunc
). It appears that after some of the interruption, theCachedFunction
class suddenly decided that the function now takes 3 arguments instead of 2... and trying to call it with 3 values instead of 2 I just got an unhandled SIGSEGV.Might be something subtle going on with exception and cache but I do not know why. I can describe the case in more details.
Personally, I would not bother too much with this. Interrupting Python (as opposed to Cython) code can give all kinds of undefined behaviour...
comment:10 Changed 6 years ago by
 Commit changed from 00f4fdb067d61ec76b87efb0f250a4937bda9046 to a54ef1458ec570aa3680507aa25963c0e8471468
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a54ef14  trac #16901: make cython code in OA interruptible

comment:11 Changed 6 years ago by
 Commit changed from a54ef1458ec570aa3680507aa25963c0e8471468 to 4917f8563795f8976910c7d798ae7909b3a0396d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4917f85  trac #16901: make cython code in OA interruptible

comment:12 Changed 6 years ago by
 Status changed from needs_work to needs_review
All right, very simple one line change which makes us loose only 0.02 secs for the generation of the 1800 first MOLS.
Vincent
comment:13 followup: ↓ 14 Changed 6 years ago by
Okayyyyyyyy.. This being said, why did you change the bint to int ?
Nathann
comment:14 in reply to: ↑ 13 ; followup: ↓ 15 Changed 6 years ago by
Replying to ncohen:
Okayyyyyyyy.. This being said, why did you change the bint to int ?
The exception is propagated with the return value 1
, I think it is cleaner to make it an int.
Anyway, the choice int/bint
is just a compiler instruction for Cython. There is no gain of having it a bint
type here since it is always used in C code. The purpose of the bint type is to mix well Python booleans with C ints. One difference (that I learn few seconds ago) is in
def f(b): int x = b bint y = b
The first conversion will involves b.__int__
while the second b.__nonzero__
.
Vincent
comment:15 in reply to: ↑ 14 ; followup: ↓ 16 Changed 6 years ago by
 Status changed from needs_review to positive_review
Yo !
The exception is propagated with the return value
1
, I think it is cleaner to make it an int.
Oh right, right.
Anyway, the choice
int/bint
is just a compiler instruction for Cython. There is no gain of having it abint
type here since it is always used in C code.
Of course, but there if there is no reason to change that into an 'int' it was better to keep it. And that's a good reason to change it. Good to go !
The purpose of the bint type is to mix well Python booleans with C ints. One difference (that I learn few seconds ago) is in
def f(b): int x = b bint y = bThe first conversion will involves
b.__int__
while the secondb.__nonzero__
.
I hate Python.
Nathann
comment:16 in reply to: ↑ 15 ; followup: ↓ 17 Changed 6 years ago by
Replying to ncohen:
The purpose of the bint type is to mix well Python booleans with C ints. One difference (that I learn few seconds ago) is in
def f(b): int x = b bint y = bThe first conversion will involves
b.__int__
while the secondb.__nonzero__
.I hate Python.
Let me see. You want strongly typed, checked at compilation code... hum, use Haskell!
Vincent
comment:17 in reply to: ↑ 16 Changed 6 years ago by
Why is everybody trying to use logic to prove that I must like this or that thing ?
I hate Python AND I hate Haskell. Though I never used Haskell, of course.
Nathann
comment:18 Changed 6 years ago by
Doesn't work. Also, use bint for what would be a boolean in Python and int for integers. E.g. simplifies debugging if you can just switch the method to cpdef and you get a Python boolean instead of some integer.
sage t long warnlong 43.5 src/sage/combinat/designs/orthogonal_arrays_find_recursive.pyx # 13 doctests failed sage t long warnlong 43.5 src/sage/combinat/designs/orthogonal_arrays_build_recursive.py # 3 doctests failed sage t long warnlong 43.5 src/sage/combinat/designs/orthogonal_arrays.py # 11 doctests failed
comment:19 Changed 6 years ago by
 Status changed from positive_review to needs_work
comment:20 Changed 6 years ago by
 Commit changed from 4917f8563795f8976910c7d798ae7909b3a0396d to e5343b297f02c90ad2100e9563b8fc9d372d6b3a
Branch pushed to git repo; I updated commit sha1. New commits:
e5343b2  trac #16901: being careful with return value

comment:21 Changed 6 years ago by
 Status changed from needs_work to needs_review
Hum.. I wonder what happened!
Vincent
comment:22 Changed 6 years ago by
I don't get what you did in your latest commit: you change the first "return" to give a boolean, yet in the second one you change a 'True' into a '1' O_o
Nathann
comment:23 Changed 6 years ago by
The function is returned an int
. So anyway Cython will convert True
to 1. If you prefer 1 > True
and 0 > False
fell free to do it.
Vincent
comment:24 followup: ↓ 25 Changed 6 years ago by
Why did you change it in the first place ? O_o
comment:25 in reply to: ↑ 24 Changed 6 years ago by
comment:26 Changed 6 years ago by
With different types of output ? O_o
Nathann
comment:27 Changed 6 years ago by
 Commit changed from e5343b297f02c90ad2100e9563b8fc9d372d6b3a to 6097260461d3d20653d3406db362b4e9c9a8b40c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6097260  trac #16901: make cython code in OA interruptible

comment:28 Changed 6 years ago by
done.
Vincent
comment:29 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:30 Changed 6 years ago by
 Branch changed from u/vdelecroix/16901 to 6097260461d3d20653d3406db362b4e9c9a8b40c
 Resolution set to fixed
 Status changed from positive_review to closed
It is better with the branch applied as it becomes interruptible. Nevertheless it is not ideal since it may happen that after an interruption you are not able to call
MOLS_table
anymore... got strange error likeVincent
Last 10 new commits:
trac #16879: orthogonal_array_recursive.py > pyx
trac #16879: a is_available function in orthogonal_arrays_recursive
trac #16879: more speed up
trac #16879: rename orthogonal_arrays_recursive to orthogonal_arrays_find_recursive
trac #16879: Move constructions from orthogonal_arrays_find to orthogonal_arrays_build (this, and only this)
trac #16879: Fix the import statements
trac #16879: Fix the doc
trac #16879: speed up
trac #16879: Trivial stuff
trac #16879: make cython code in OA interruptible