Opened 9 years ago

Closed 9 years ago

#15352 closed enhancement (fixed)

Declare various sig_ macros as nogil

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.1
Component: c_lib Keywords:
Cc: robertwb, ohanar, pbruin Merged in:
Authors: Jeroen Demeyer Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: u/jdemeyer/ticket/15352 (Commits, GitHub, GitLab) Commit: 463bffe560eda24298ebff260cf6fb2c2abd2496
Dependencies: #14029, #13311, #9640 Stopgaps:

Status badges

Description (last modified by jdemeyer)

One consequence is to allow Cython's OpenMP facilities, see #12690.

While we're at it, do lots of clean-up of signal/error handling after #9640 and friends:

  • use sig_error() for NTL and libGAP
  • remove the old _sig_on macros which are deprecated since 3 years

Follow-up at #15543: update documentation

Attachments (1)

15352_sig_nogil.patch (49.8 KB) - added by jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by jdemeyer

  • Dependencies set to #14029, #13311, #9640

comment:2 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 9 years ago by jdemeyer

  • Cc robertwb added
  • Status changed from new to needs_review

comment:4 Changed 9 years ago by jdemeyer

  • Cc ohanar added

comment:5 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 9 years ago by jdemeyer

  • Cc pbruin added

Changed 9 years ago by jdemeyer

comment:8 Changed 9 years ago by pbruin

  • Reviewers set to Peter Bruin
  • Status changed from needs_review to positive_review

Looks good and all tests pass.

comment:9 Changed 9 years ago by vbraun

Conflicts with the new developer manual, please resolve...

comment:10 Changed 9 years ago by jdemeyer

What was the point of introducing changes like

-When writing Cython code for Sage, special care must be taken to ensure
-the code can be interrupted with ``CTRL-C``.
-Since Cython optimizes for speed,
-Cython normally does not check for interrupts.
+When writing Cython code for Sage, special care must be taken to
+ensure the code can be interrupted with ``CTRL-C``.  Since Cython
+optimizes for speed, Cython normally does not check for interrupts.

Seriously, that's just asking for merge conflicts...

comment:11 Changed 9 years ago by jdemeyer

This is even worse:

-A common mistake is to put ``sig_off()`` towards the end of a
+A common mqistake is to put ``sig_off()`` towards the end of a

(from 67a76032fda3ee140c3bc52422020ef722cbee52)

Last edited 9 years ago by jdemeyer (previous) (diff)

comment:12 Changed 9 years ago by jdemeyer

Wow, this bad: commit 67a76032fda3ee140c3bc52422020ef722cbee52 reverts all changes to coding_in_cython.rst done by #13311. How did that happen?

-    sage: alarm(0.5); factor(10^1000 + 3)
-    Traceback (most recent call last):
-    ...
-    AlarmInterrupt
+    sage: import sage.tests.interrupt
+    sage: try:
+    ...     sage.tests.interrupt.interrupt_after_delay()
+    ...     factor(10^1000 + 3)
+    ... except KeyboardInterrupt:
+    ...     print "ok!"
+    ok!
+
Last edited 9 years ago by jdemeyer (previous) (diff)

comment:13 Changed 9 years ago by jdemeyer

Volker, apologies for my bad mood. I could easily revert the changes from 67a76032fda3ee140c3bc52422020ef722cbee52 which conflict with this patch, but I guess you should tell me whether that's a good idea.

comment:14 Changed 9 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/15352
  • Created changed from 11/04/13 13:19:41 to 11/04/13 13:19:41
  • Modified changed from 12/18/13 12:59:30 to 12/18/13 12:59:30

comment:15 Changed 9 years ago by jdemeyer

  • Branch u/jdemeyer/ticket/15352 deleted
  • Description modified (diff)

I moved the documentation part to #15543. The git branch contains all changes from the Mercurial patch except for the documentation.

comment:16 Changed 9 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/15352
  • Commit set to 463bffe560eda24298ebff260cf6fb2c2abd2496

I moved the documentation part to #15543. The git branch contains all changes from the Mercurial patch except for the documentation.


New commits:

463bffeDeclare various sig_ macros as nogil

comment:17 Changed 9 years ago by vbraun

The changes to the developer guide were lost because I had forked that off to rewrite it, sorry. I think the changes from #13311 were the only ones affected.

comment:18 Changed 9 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.