Opened 4 years ago

Closed 4 years ago

#19525 closed enhancement (fixed)

Improve GLPK error handling

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.2
Component: cython Keywords:
Cc: ncohen Merged in:
Authors: Jeroen Demeyer Reviewers: Vincent Delecroix
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 76a11fd (Commits) Commit: 76a11fd0a01ec366c844e81bef879b64746c0eb1
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This ticket adds a proper error handler for GLPK (similar to what we already have for PARI, NTL, ...). It suffices to add sig_on()/sig_off() around a GLPK call to get an error message.

Some more cleanup is also done:

  1. change the implementation of MIPSolverException to just inherit from RuntimeError without any custom implementation.
  2. use MemoryAllocator in a few places to make some allocations safer.

This branch requires a patch to GLPK, a modified version of it was accepted upstream: http://lists.gnu.org/archive/html/help-glpk/2015-11/msg00008.html

Change History (38)

comment:1 Changed 4 years ago by jdemeyer

  • Dependencies set to #19527

comment:2 Changed 4 years ago by ncohen

  • Cc ncohen added

comment:3 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/improve_glpk_error_handling

comment:4 Changed 4 years ago by git

  • Commit set to f808061d3d4cbaf2bd0cfe42f25fb67b5131a302

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

f808061Implement GLPK error handling

comment:5 Changed 4 years ago by jdemeyer

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.
  • Status changed from new to needs_review

comment:6 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:8 Changed 4 years ago by git

  • Commit changed from f808061d3d4cbaf2bd0cfe42f25fb67b5131a302 to f31281897dcf292890a74aa39b83dd30a1913b56

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

f312818Improve printing of solver exceptions

comment:9 Changed 4 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

comment:10 Changed 4 years ago by git

  • Commit changed from f31281897dcf292890a74aa39b83dd30a1913b56 to 18265ee99fd0165d87b30324e7d81ea54e0b7c47

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

18265eeRename glp_have_error -> glp_at_error

comment:11 Changed 4 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.

comment:12 Changed 4 years ago by jdemeyer

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

comment:13 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from Fixed upstream, in a later stable release. to Not yet reported upstream; Will do shortly.

comment:14 Changed 4 years ago by jdemeyer

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Fixed upstream, in a later stable release.

Upstream claims we are mis-using the GLPK error hook. They are probably right. However, given that I'd like to keep "upgrade GLPK" separated from this ticket, I still propose to review this ticket as-is and then deal with the upgrade on a new ticket.

comment:15 follow-up: Changed 4 years ago by dimpase

Does a reviewer need to create a patched GLPK tarball himself?

comment:16 in reply to: ↑ 15 Changed 4 years ago by jdemeyer

Replying to dimpase:

Does a reviewer need to create a patched GLPK tarball himself?

No. This isn't a GLPK upgrade.

comment:17 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:18 Changed 4 years ago by mkoeppe

Any news on the upstream situation?

comment:19 follow-up: Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

does not apply on the latest beta

comment:20 Changed 4 years ago by jdemeyer

  • Dependencies #19527 deleted
  • Milestone changed from sage-6.10 to sage-7.2

Any news on the upstream situation?

See 14

comment:21 in reply to: ↑ 19 ; follow-up: Changed 4 years ago by jdemeyer

Replying to vdelecroix:

does not apply on the latest beta

Are you actually interested in reviewing the ticket? (no offense, but I don't like people complaining that branches don't apply if they have absolutely no interest in reviewing the ticket).

comment:22 in reply to: ↑ 21 Changed 4 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

does not apply on the latest beta

Are you actually interested in reviewing the ticket? (no offense, but I don't like people complaining that branches don't apply if they have absolutely no interest in reviewing the ticket).

I will not pronounce about a review. But of course, I was interested in looking at the code.

comment:23 Changed 4 years ago by git

  • Commit changed from 18265ee99fd0165d87b30324e7d81ea54e0b7c47 to 597415278f3416e303a7eea9731c1e80090d0127

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

5974152Merge tag '7.2.beta1' into t/19525/improve_glpk_error_handling

comment:24 Changed 4 years ago by vdelecroix

hmm

@@ -618,8 +634,14 @@ cdef class GLPKBackend(GenericBackend):
             (2.0, 2.0)
         """
         cdef int n = glp_get_num_cols(self.lp)
+<<<<<<< HEAD
+        cdef MemoryAllocator mem = MemoryAllocator()
+        cdef int * c_indices = <int*>mem.allocarray(n+1, sizeof(int))
+        cdef double * c_values = <double*>mem.allocarray(n+1, sizeof(double))
+=======
         cdef int * c_indices = <int*> sig_malloc((n+1)*sizeof(int))
         cdef double * c_values = <double*> sig_malloc((n+1)*sizeof(double))
+>>>>>>> 7.2.beta1
         cdef list indices = []
         cdef list values = []
         cdef int i,j

comment:25 Changed 4 years ago by git

  • Commit changed from 597415278f3416e303a7eea9731c1e80090d0127 to 52113765b23781f4a080003c67b243cc15f253f1

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

5211376Merge tag '7.2.beta1' into t/19525/improve_glpk_error_handling

comment:26 Changed 4 years ago by vdelecroix

import "cysignals/signals.pxi" instead of "sage/ext/interrupt.pxi", see #20002 for details ;-)

comment:27 Changed 4 years ago by git

  • Commit changed from 52113765b23781f4a080003c67b243cc15f253f1 to 3e333424fdc6b62f46ce4b350ca560103486e3f3

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

3e33342Use cysignals instead of interrupt.pxi

comment:28 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:29 Changed 4 years ago by vdelecroix

  • Branch changed from u/jdemeyer/improve_glpk_error_handling to u/vdelecroix/19525
  • Commit changed from 3e333424fdc6b62f46ce4b350ca560103486e3f3 to 71f52a44647b8ba440152420070e6a869f2190a1

A commit to fix changes in error messages.


New commits:

71f52a4Trac 19525: fix error messages for cplex and gurobi

comment:30 Changed 4 years ago by mkoeppe

See also #10232

comment:31 Changed 4 years ago by vdelecroix

Indeed, in order to fix #10232 there need to be the appropriate sig_on and sig_off around the GLPK calls.

comment:32 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_info

Jeroen, do you prefer another ticket to add the appropriate sig_on/sig_off around GLPK calls?

comment:33 Changed 4 years ago by vdelecroix

If yes, then the ticket is good to go from my point of view.

comment:34 Changed 4 years ago by jdemeyer

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_info to positive_review

comment:35 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Fails on 32-bit

sage -t --long src/sage/numerical/backends/glpk_backend.pyx
**********************************************************************
File "src/sage/numerical/backends/glpk_backend.pyx", line 847, in sage.numerical.backends.glpk_backend.GLPKBackend.solve
Failed example:
    lp.solve()
Expected:
    Traceback (most recent call last):
    ...
    GLPKError: Assertion failed: col->lb < col->ub
    Error detected in file glpnpp05.c at line ...
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.numerical.backends.glpk_backend.GLPKBackend.solve[24]>", line 1, in <module>
        lp.solve()
      File "sage/numerical/mip.pyx", line 2115, in sage.numerical.mip.MixedIntegerLinearProgram.solve (/home/buildbot/slave/sage_git/build/src/build/cythonized/sage/numerical/mip.c:14757)
        self._backend.solve()
      File "sage/numerical/backends/glpk_backend.pyx", line 1005, in sage.numerical.backends.glpk_backend.GLPKBackend.solve (/home/buildbot/slave/sage_git/build/src/build/cythonized/sage/numerical/backends/glpk_backend.cpp:8406)
        sig_on()
    GLPKError: Assertion failed: q->lb < q->ub
    Error detected in file glpnpp03.c at line 557
**********************************************************************
1 item had failures:
   1 of  76 in sage.numerical.backends.glpk_backend.GLPKBackend.solve

comment:36 Changed 4 years ago by jdemeyer

  • Branch changed from u/vdelecroix/19525 to u/jdemeyer/19525

comment:37 Changed 4 years ago by jdemeyer

  • Commit changed from 71f52a44647b8ba440152420070e6a869f2190a1 to 76a11fd0a01ec366c844e81bef879b64746c0eb1
  • Status changed from needs_work to positive_review

This should do the trick...


New commits:

76a11fdUse ellipsis in assertion error message

comment:38 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/19525 to 76a11fd0a01ec366c844e81bef879b64746c0eb1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.