Opened 5 years ago
Closed 5 years ago
#19525 closed enhancement (fixed)
Improve GLPK error handling
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage7.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, GitHub, GitLab)  Commit:  76a11fd0a01ec366c844e81bef879b64746c0eb1 
Dependencies:  Stopgaps: 
Description (last modified by )
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:
 change the implementation of
MIPSolverException
to just inherit fromRuntimeError
without any custom implementation.  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/helpglpk/201511/msg00008.html
Change History (38)
comment:1 Changed 5 years ago by
 Dependencies set to #19527
comment:2 Changed 5 years ago by
 Cc ncohen added
comment:3 Changed 5 years ago by
 Branch set to u/jdemeyer/improve_glpk_error_handling
comment:4 Changed 5 years ago by
 Commit set to f808061d3d4cbaf2bd0cfe42f25fb67b5131a302
comment:5 Changed 5 years ago by
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
 Status changed from new to needs_review
comment:6 Changed 5 years ago by
 Description modified (diff)
comment:7 Changed 5 years ago by
 Description modified (diff)
comment:8 Changed 5 years ago by
 Commit changed from f808061d3d4cbaf2bd0cfe42f25fb67b5131a302 to f31281897dcf292890a74aa39b83dd30a1913b56
Branch pushed to git repo; I updated commit sha1. New commits:
f312818  Improve printing of solver exceptions

comment:9 Changed 5 years ago by
 Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.
comment:10 Changed 5 years ago by
 Commit changed from f31281897dcf292890a74aa39b83dd30a1913b56 to 18265ee99fd0165d87b30324e7d81ea54e0b7c47
Branch pushed to git repo; I updated commit sha1. New commits:
18265ee  Rename glp_have_error > glp_at_error

comment:11 Changed 5 years ago by
 Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.
comment:12 Changed 5 years ago by
 Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.
comment:13 Changed 5 years ago by
 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 5 years ago by
 Report Upstream changed from Not yet reported upstream; Will do shortly. to Fixed upstream, in a later stable release.
Upstream claims we are misusing 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 asis and then deal with the upgrade on a new ticket.
comment:15 followup: ↓ 16 Changed 5 years ago by
Does a reviewer need to create a patched GLPK tarball himself?
comment:16 in reply to: ↑ 15 Changed 5 years ago by
Replying to dimpase:
Does a reviewer need to create a patched GLPK tarball himself?
No. This isn't a GLPK upgrade.
comment:17 Changed 5 years ago by
 Description modified (diff)
comment:18 Changed 5 years ago by
Any news on the upstream situation?
comment:19 followup: ↓ 21 Changed 5 years ago by
 Status changed from needs_review to needs_work
does not apply on the latest beta
comment:20 Changed 5 years ago by
 Dependencies #19527 deleted
 Milestone changed from sage6.10 to sage7.2
Any news on the upstream situation?
See 14
comment:21 in reply to: ↑ 19 ; followup: ↓ 22 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
 Commit changed from 18265ee99fd0165d87b30324e7d81ea54e0b7c47 to 597415278f3416e303a7eea9731c1e80090d0127
Branch pushed to git repo; I updated commit sha1. New commits:
5974152  Merge tag '7.2.beta1' into t/19525/improve_glpk_error_handling

comment:24 Changed 5 years ago by
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 5 years ago by
 Commit changed from 597415278f3416e303a7eea9731c1e80090d0127 to 52113765b23781f4a080003c67b243cc15f253f1
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5211376  Merge tag '7.2.beta1' into t/19525/improve_glpk_error_handling

comment:26 Changed 5 years ago by
import "cysignals/signals.pxi" instead of "sage/ext/interrupt.pxi", see #20002 for details ;)
comment:27 Changed 5 years ago by
 Commit changed from 52113765b23781f4a080003c67b243cc15f253f1 to 3e333424fdc6b62f46ce4b350ca560103486e3f3
Branch pushed to git repo; I updated commit sha1. New commits:
3e33342  Use cysignals instead of interrupt.pxi

comment:28 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:29 Changed 5 years ago by
 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:
71f52a4  Trac 19525: fix error messages for cplex and gurobi

comment:30 Changed 5 years ago by
See also #10232
comment:31 Changed 5 years ago by
Indeed, in order to fix #10232 there need to be the appropriate sig_on
and sig_off
around the GLPK calls.
comment:32 Changed 5 years ago by
 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 5 years ago by
If yes, then the ticket is good to go from my point of view.
comment:34 Changed 5 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_info to positive_review
comment:35 Changed 5 years ago by
 Status changed from positive_review to needs_work
Fails on 32bit
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/sitepackages/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/sitepackages/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 5 years ago by
 Branch changed from u/vdelecroix/19525 to u/jdemeyer/19525
comment:37 Changed 5 years ago by
 Commit changed from 71f52a44647b8ba440152420070e6a869f2190a1 to 76a11fd0a01ec366c844e81bef879b64746c0eb1
 Status changed from needs_work to positive_review
comment:38 Changed 5 years ago by
 Branch changed from u/jdemeyer/19525 to 76a11fd0a01ec366c844e81bef879b64746c0eb1
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Implement GLPK error handling