Opened 5 years ago

Closed 5 years ago

#16901 closed defect (fixed)

cythonized function in combinatorial designs must be interruptible

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-6.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 5 years ago by vdelecroix

  • Branch set to u/vdelecroix/16901
  • Commit set to 00f4fdb067d61ec76b87efb0f250a4937bda9046

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 like

TypeError                                 Traceback (most recent call last)
<ipython-input-4-3880577162c6> in <module>()
----> 1 MOLS_table(Integer(10000))

/opt/sage_flatsurf/local/lib/python2.7/site-packages/sage/combinat/designs/latin_squares.pyc in MOLS_table(number_of_lines, compare)
    513         if i%20==0:
    514             print "\n%3d|"%i,
--> 515         k = mutually_orthogonal_latin_squares(None,i,existence=True)
    516         if compare:
    517             if i < 2 or hb[i] == k:

/opt/sage_flatsurf/local/lib/python2.7/site-packages/sage/combinat/designs/latin_squares.pyc in mutually_orthogonal_latin_squares(k, n, partitions, check, existence)
    359             raise ValueError("there are no bound on k when 0<=n<=1")
    360 
--> 361         k = orthogonal_array(None,n,existence=True) - 2
    362         if existence:
    363             return k

/opt/sage_flatsurf/local/lib/python2.7/site-packages/sage/combinat/designs/orthogonal_arrays.pyc in orthogonal_array(k, n, t, resolvable, check, existence)
    811         else:
    812             for k in range(t-1,n+2):
--> 813                 if not orthogonal_array(k+1,n,t=t,existence=True):
    814                     break
    815         if existence:

/opt/sage_flatsurf/local/lib/python2.7/site-packages/sage/combinat/designs/orthogonal_arrays.pyc in orthogonal_array(k, n, t, resolvable, check, existence)
    909         OA = OA_from_quasi_difference_matrix(M,G,add_col=True)
    910 
--> 911     elif may_be_available and find_recursive_construction(k,n):
    912         _OA_cache_set(k,n,True)
    913         if existence:

/opt/sage_flatsurf/local/lib/python2.7/site-packages/sage/misc/cachefunc.so in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:4678)()

/opt/sage_flatsurf/local/lib/python2.7/site-packages/sage/combinat/designs/orthogonal_arrays_find_recursive.so in sage.combinat.designs.orthogonal_arrays_find_recursive.find_recursive_construction (build/cythonized/sage/combinat/designs/orthogonal_arrays_find_recursive.c:2565)()

TypeError: function takes exactly 3 arguments (2 given)

Vincent


Last 10 new commits:

80e0ceftrac #16879: orthogonal_array_recursive.py -> pyx
0549110trac #16879: a is_available function in orthogonal_arrays_recursive
0201bcbtrac #16879: more speed up
b110018trac #16879: rename orthogonal_arrays_recursive to orthogonal_arrays_find_recursive
d9e0475trac #16879: Move constructions from orthogonal_arrays_find to orthogonal_arrays_build (this, and only this)
5031aeetrac #16879: Fix the import statements
0c1893ftrac #16879: Fix the doc
6a3869dtrac #16879: speed up
d7129d6trac #16879: Trivial stuff
00f4fdbtrac #16879: make cython code in OA interruptible

comment:2 Changed 5 years ago by vdelecroix

  • Dependencies set to #16879

comment:3 Changed 5 years ago by vdelecroix

  • Status changed from new to needs_review

comment:4 Changed 5 years ago by ncohen

  • 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 5 years ago by jdemeyer

  • 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 5 years ago by jdemeyer

Perhaps the warning at http://www.sagemath.org/doc/developer/coding_in_cython.html#using-sig-on-and-sig-off is not clear enough, but really the code inside sig_on() should be pure C or Cython code.

comment:7 follow-up: Changed 5 years ago by jdemeyer

This ticket inspired me to create #16902.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by vdelecroix

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 5 years ago by jdemeyer

Replying to vdelecroix:

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.

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 5 years ago by git

  • Commit changed from 00f4fdb067d61ec76b87efb0f250a4937bda9046 to a54ef1458ec570aa3680507aa25963c0e8471468

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

a54ef14trac #16901: make cython code in OA interruptible

comment:11 Changed 5 years ago by git

  • Commit changed from a54ef1458ec570aa3680507aa25963c0e8471468 to 4917f8563795f8976910c7d798ae7909b3a0396d

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

4917f85trac #16901: make cython code in OA interruptible

comment:12 Changed 5 years ago by vdelecroix

  • 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 follow-up: Changed 5 years ago by ncohen

Okayyyyyyyy.. This being said, why did you change the bint to int ?

Nathann

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by vdelecroix

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 ; follow-up: Changed 5 years ago by ncohen

  • 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 a bint 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 = b

The first conversion will involves b.__int__ while the second b.__nonzero__.

I hate Python.

Nathann

comment:16 in reply to: ↑ 15 ; follow-up: Changed 5 years ago by vdelecroix

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 = b

The first conversion will involves b.__int__ while the second b.__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 5 years ago by ncohen

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 5 years ago by vbraun

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 --warn-long 43.5 src/sage/combinat/designs/orthogonal_arrays_find_recursive.pyx  # 13 doctests failed
sage -t --long --warn-long 43.5 src/sage/combinat/designs/orthogonal_arrays_build_recursive.py  # 3 doctests failed
sage -t --long --warn-long 43.5 src/sage/combinat/designs/orthogonal_arrays.py  # 11 doctests failed

comment:19 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:20 Changed 5 years ago by git

  • Commit changed from 4917f8563795f8976910c7d798ae7909b3a0396d to e5343b297f02c90ad2100e9563b8fc9d372d6b3a

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

e5343b2trac #16901: being careful with return value

comment:21 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

Hum.. I wonder what happened!

Vincent

comment:22 Changed 5 years ago by ncohen

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 5 years ago by vdelecroix

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 follow-up: Changed 5 years ago by ncohen

Why did you change it in the first place ? O_o

comment:25 in reply to: ↑ 24 Changed 5 years ago by vdelecroix

Replying to ncohen:

Why did you change it in the first place ? O_o

To make cython happier ;=)

comment:26 Changed 5 years ago by ncohen

With different types of output ? O_o

Nathann

comment:27 Changed 5 years ago by git

  • Commit changed from e5343b297f02c90ad2100e9563b8fc9d372d6b3a to 6097260461d3d20653d3406db362b4e9c9a8b40c

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

6097260trac #16901: make cython code in OA interruptible

comment:28 Changed 5 years ago by vdelecroix

done.

Vincent

comment:29 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:30 Changed 5 years ago by vbraun

  • Branch changed from u/vdelecroix/16901 to 6097260461d3d20653d3406db362b4e9c9a8b40c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.