Opened 9 years ago

Closed 6 years ago

#9640 closed enhancement (fixed)

Change PARI error catching mechanism

Reported by: jdemeyer Owned by: was
Priority: major Milestone: sage-5.13
Component: interfaces Keywords: pari error interrupt
Cc: leif, jdemeyer Merged in: sage-5.13.beta3
Authors: Peter Bruin, Jeroen Demeyer Reviewers: Jeroen Demeyer, Peter Bruin
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14029, #13311 Stopgaps:

Description (last modified by jdemeyer)

Currently, the exceptions thrown by PARI are rather cryptic, like

Traceback (most recent call last):
...
PariError:  (15)

This ticket does the following:

  • Use cb_pari_handle_exception() instead of err_catch() to catch PARI exceptions.
  • Using a mechanism similar to #9636, catch the full text of the exception and pass it to PariError.
  • Introduce a new function sig_error() to callback to the interrupt/signal handling code to handle errors which aren't signals and use that for PARI.

Apply: 9640-pari_error_callbacks_v2.patch

Attachments (4)

9640old.patch (85.5 KB) - added by jdemeyer 9 years ago.
Old patch for reference
9640-pari_error_handling.patch (11.9 KB) - added by pbruin 6 years ago.
based on 5.12.beta4 + #15124
9640-pari_error_callbacks.patch (15.9 KB) - added by jdemeyer 6 years ago.
based on 5.12.beta4 + #15124
9640-pari_error_callbacks_v2.patch (36.0 KB) - added by jdemeyer 6 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 9 years ago by leif

  • Authors set to Jeroen Demeyer
  • Cc leif added

Changed 9 years ago by jdemeyer

Old patch for reference

comment:3 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Keywords pari error interrupt added

comment:4 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 6 years ago by pbruin

  • Cc pbruin added

comment:6 Changed 6 years ago by pbruin

I am working on a patch that does the following things suggested by the ticket description:

  • Rewrite pari_catch_sig_on() and pari_catch_sig_off() to use the callbacks cb_pari_handle_exception and cb_pari_err_recover (defined by the PARI library) in combination with sig_on()/sig_off(), as opposed to the additional setjmp() of the current implementation.
  • Catch output sent to pariErr and store it in a string attribute pari_instance.error_string for later use by PariError.

It seems to work quite well; the only doctest failures I've seen so far are a few tests expecting a PARI warning message. With the patch, these are not printed because they also go to pariErr.

To be done:

  • Make PariError aware of the stored string to give a more informative error message. This should probably simply be extracted from the lines starting with *** in pari_instance.error_string.
  • Minor change in PARI: send any extra newline at the start of the error message to pariErr instead of pariOut (this happens if the last printed character was not a newline).
  • Distinguish between errors and warnings; the latter should probably be printed to stderr as before. This should be done either by filtering the text sent to pariErr or by having pari_catch_sig_off() check if pari_instance.error_string is non-empty even if no error occurred.

This may make #14894 redundant, although that ticket could be used for general things to be done for the upgrade to PARI 2.6.

Changed 6 years ago by pbruin

based on 5.12.beta4 + #15124

comment:7 Changed 6 years ago by pbruin

  • Status changed from new to needs_review
  • Work issues set to see comment 6

The attached patch passes doctests on x86_64 GNU/Linux, except for two tests expecting a PARI warning (see above).

comment:8 Changed 6 years ago by pbruin

  • Status changed from needs_review to needs_work

comment:9 Changed 6 years ago by pbruin

  • Authors changed from Jeroen Demeyer to Peter Bruin
  • Cc jdemeyer added; pbruin removed

It would be better practice to move _pari_init_err(), _pari_handle_exception() and _pari_err_recover() from pari_err.h to a C file or to translate them to Cython. At the moment I don't think it is worth the trouble.

comment:10 Changed 6 years ago by pbruin

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues see comment 6 deleted

The latest patch does everything that I think it should do. I hope it is reasonably self-explanatory. A few remarks:

  • The error text is now passed to PariError and can be accessed using PariError.errtext(); is is not printed automatically. Using this text to improve messages like PariError: (15) will probably have to wait until another ticket. (Edit: user errors are an exception, since they are converted into RuntimeError; here we append the error text to the currently used "PARI user exception".)
  • There is no problem with PARI sending escape sequences for colours in error messages; this is disabled by default.
  • The printing of a possible extra newline before the error message is easily avoided using pari_set_last_newline(1).
  • The callback functions are now Cython functions in the new file pari_error.pyx. It is important that this does not include pari_err.pxi; because of the way Cython imports functions, pari_err.pxi and pari_err.h use function pointers with the same names as the functions defined in pari_error.pyx.
Last edited 6 years ago by pbruin (previous) (diff)

comment:11 Changed 6 years ago by pbruin

Patchbot:

apply 9640-pari_error_callbacks.patch​

comment:12 follow-up: Changed 6 years ago by jdemeyer

Why not set cb_pari_handle_exception = _pari_handle_exception globally and have pari_catch_sig_on() simply be equal to sig_on() (and then of course change pari_catch_sig_on() back to sig_on()).

comment:13 follow-up: Changed 6 years ago by jdemeyer

Instead of using sage_siglongjmp(), I think it's safer to call abort() and use the normal signal handling mechanism. Otherwise we have to consider race conditions like a SIGINT arriving during the sage_siglongjmp() call...

comment:14 in reply to: ↑ 12 Changed 6 years ago by pbruin

Replying to jdemeyer:

Why not set cb_pari_handle_exception = _pari_handle_exception globally and have pari_catch_sig_on() simply be equal to sig_on() (and then of course change pari_catch_sig_on() back to sig_on()).

Good question. That would mean we simply always have PARI exception handling enabled, which would be more efficient and should not make any difference since PARI shouldn't be called outside of sig_on()/sig_off() anyway. I'll try it out.

comment:15 follow-up: Changed 6 years ago by jdemeyer

Anyway, I am thinking about a review patch so I will make these changes myself.

Peter, is there any chance you could review #14029 and #13311 because those tickets also make changes to the interrupt code.

comment:16 in reply to: ↑ 13 ; follow-up: Changed 6 years ago by pbruin

Replying to jdemeyer:

Instead of using sage_siglongjmp(), I think it's safer to call abort() and use the normal signal handling mechanism. Otherwise we have to consider race conditions like a SIGINT arriving during the sage_siglongjmp() call...

Why abort() and not raise(SIGUSR1) or another signal code?

comment:17 Changed 6 years ago by jdemeyer

I am thinking about merging (parts of) #10018 here, especially the doctests.

comment:18 in reply to: ↑ 16 ; follow-up: Changed 6 years ago by jdemeyer

Replying to pbruin:

Replying to jdemeyer:

Instead of using sage_siglongjmp(), I think it's safer to call abort() and use the normal signal handling mechanism. Otherwise we have to consider race conditions like a SIGINT arriving during the sage_siglongjmp() call...

Why abort() and not raise(SIGUSR1) or another signal code?

Because we already handle abort() anyway and because I see no gain of adding an extra signal, with the risk that SIGUSR1 is used by other Sage packages.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 6 years ago by pbruin

Replying to jdemeyer:

Replying to pbruin:

Why abort() and not raise(SIGUSR1) or another signal code?

Because we already handle abort() anyway and because I see no gain of adding an extra signal, with the risk that SIGUSR1 is used by other Sage packages.

But we handle it by raising a new RuntimeError, among other things, unless I misunderstand interrupt.c. So wouldn't the SIGABRT handler need to have a special case for the case where it is called in this way? This would be rather inelegant.

comment:20 in reply to: ↑ 19 Changed 6 years ago by jdemeyer

Replying to pbruin:

This would be rather inelegant.

I personally don't find it inelegant. In any case, we would encapsalate this through some call like sig_error().

comment:21 in reply to: ↑ 15 Changed 6 years ago by pbruin

Replying to jdemeyer:

Peter, is there any chance you could review #14029 and #13311 because those tickets also make changes to the interrupt code.

Not very soon, unfortunately (lots of work at the moment), but I'll put my name in the CC list.

comment:22 Changed 6 years ago by jdemeyer

  • Dependencies set to #14029, #13311
  • Status changed from needs_review to needs_work

comment:23 Changed 6 years ago by jdemeyer

Working/thinking about this...

Changed 6 years ago by jdemeyer

based on 5.12.beta4 + #15124

comment:24 Changed 6 years ago by jdemeyer

  • Authors changed from Peter Bruin to Peter Bruin, Jeroen Demeyer
  • Description modified (diff)

comment:25 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:26 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:27 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:28 follow-up: Changed 6 years ago by pbruin

What is the use of PariError.parimessage()? It doesn't seem to be used, and appears to normally be a substring of PariError.__str__(). Do you have any application for it in mind, or can it be removed?

comment:29 in reply to: ↑ 28 ; follow-up: Changed 6 years ago by jdemeyer

Replying to pbruin:

What is the use of PariError.parimessage()? It doesn't seem to be used, and appears to normally be a substring of PariError.__str__(). Do you have any application for it in mind, or can it be removed?

For me, it can be removed. But it existed before (as a function __errmessage()) and one possible use would be to check that an error is of a given type without relying on the PARI error codes (for example, in pure Python code). Something like:

try:
    pari.....
except PariError as E:
    if E.parimessage() == "division by zero":
        raise ZeroDivisionError
    else:
        raise

Perhaps this is never going to happen, but at least the method parimessage() doesn't hurt either.

comment:30 in reply to: ↑ 29 Changed 6 years ago by pbruin

Replying to jdemeyer:

Replying to pbruin:

What is the use of PariError.parimessage()? It doesn't seem to be used, and appears to normally be a substring of PariError.__str__(). Do you have any application for it in mind, or can it be removed?

For me, it can be removed. But it existed before (as a function __errmessage()) and one possible use would be to check that an error is of a given type without relying on the PARI error codes (for example, in pure Python code).

That is a possibility. On the other hand, the global variable errmessage is another thing that has disappeared in PARI 2.6. It has been replaced by a function const char *numerr_name(long numerr) which returns e.g. "e_INV" for zero division errors. This means that in the near future your example will probably look like

if pari.numerr_name(E.errnum()) == "e_INV":
    raise ZeroDivisionError

Of course, one could then think of a method of PariError encapsulating this if necessary, but parimessage would no longer be an appropriate name.

For the time being (at least until we switch to PARI 2.6) or unless someone really wants a method like this, I think it is easiest to just remove it.

Changed 6 years ago by jdemeyer

comment:31 Changed 6 years ago by jdemeyer

Removed PariError.parimessage() method and moved the cdef extern from "misc.h": block to the top of gen.pyx.

comment:32 follow-up: Changed 6 years ago by pbruin

I'm happy with this now. Hopefully someone wants to review it.

comment:33 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer, Peter Bruin
  • Status changed from needs_review to positive_review

comment:34 in reply to: ↑ 32 Changed 6 years ago by jdemeyer

Replying to pbruin:

I'm happy with this now. Hopefully someone wants to review it.

Since we both looked at eachother's work, I guess that counts as positive review.

comment:35 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.13.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.