Opened 7 years ago
Closed 7 years ago
#19525 closed enhancement (fixed)
Improve GLPK error handling
Reported by:  Jeroen Demeyer  Owned by:  

Priority:  major  Milestone:  sage7.2 
Component:  cython  Keywords:  
Cc:  Nathann Cohen  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 7 years ago by
Dependencies:  → #19527 

comment:2 Changed 7 years ago by
Cc:  Nathann Cohen added 

comment:3 Changed 7 years ago by
Branch:  → u/jdemeyer/improve_glpk_error_handling 

comment:4 Changed 7 years ago by
Commit:  → f808061d3d4cbaf2bd0cfe42f25fb67b5131a302 

comment:5 Changed 7 years ago by
Report Upstream:  N/A → Reported upstream. No feedback yet. 

Status:  new → needs_review 
comment:6 Changed 7 years ago by
Description:  modified (diff) 

comment:7 Changed 7 years ago by
Description:  modified (diff) 

comment:8 Changed 7 years ago by
Commit:  f808061d3d4cbaf2bd0cfe42f25fb67b5131a302 → f31281897dcf292890a74aa39b83dd30a1913b56 

Branch pushed to git repo; I updated commit sha1. New commits:
f312818  Improve printing of solver exceptions

comment:9 Changed 7 years ago by
Report Upstream:  Reported upstream. No feedback yet. → Reported upstream. Developers acknowledge bug. 

comment:10 Changed 7 years ago by
Commit:  f31281897dcf292890a74aa39b83dd30a1913b56 → 18265ee99fd0165d87b30324e7d81ea54e0b7c47 

Branch pushed to git repo; I updated commit sha1. New commits:
18265ee  Rename glp_have_error > glp_at_error

comment:11 Changed 7 years ago by
Report Upstream:  Reported upstream. Developers acknowledge bug. → Fixed upstream, but not in a stable release. 

comment:12 Changed 7 years ago by
Report Upstream:  Fixed upstream, but not in a stable release. → Fixed upstream, in a later stable release. 

comment:13 Changed 7 years ago by
Description:  modified (diff) 

Report Upstream:  Fixed upstream, in a later stable release. → Not yet reported upstream; Will do shortly. 
comment:14 Changed 7 years ago by
Report Upstream:  Not yet reported upstream; Will do shortly. → 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 7 years ago by
Does a reviewer need to create a patched GLPK tarball himself?
comment:16 Changed 7 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 7 years ago by
Description:  modified (diff) 

comment:19 followup: 21 Changed 7 years ago by
Status:  needs_review → needs_work 

does not apply on the latest beta
comment:20 Changed 7 years ago by
Dependencies:  #19527 

Milestone:  sage6.10 → sage7.2 
Any news on the upstream situation?
See 14
comment:21 followup: 22 Changed 7 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 Changed 7 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 7 years ago by
Commit:  18265ee99fd0165d87b30324e7d81ea54e0b7c47 → 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 7 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 7 years ago by
Commit:  597415278f3416e303a7eea9731c1e80090d0127 → 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 7 years ago by
import "cysignals/signals.pxi" instead of "sage/ext/interrupt.pxi", see #20002 for details ;)
comment:27 Changed 7 years ago by
Commit:  52113765b23781f4a080003c67b243cc15f253f1 → 3e333424fdc6b62f46ce4b350ca560103486e3f3 

Branch pushed to git repo; I updated commit sha1. New commits:
3e33342  Use cysignals instead of interrupt.pxi

comment:28 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:29 Changed 7 years ago by
Branch:  u/jdemeyer/improve_glpk_error_handling → u/vdelecroix/19525 

Commit:  3e333424fdc6b62f46ce4b350ca560103486e3f3 → 71f52a44647b8ba440152420070e6a869f2190a1 
A commit to fix changes in error messages.
New commits:
71f52a4  Trac 19525: fix error messages for cplex and gurobi

comment:31 Changed 7 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 7 years ago by
Status:  needs_review → needs_info 

Jeroen, do you prefer another ticket to add the appropriate sig_on/sig_off
around GLPK calls?
comment:34 Changed 7 years ago by
Reviewers:  → Vincent Delecroix 

Status:  needs_info → positive_review 
comment:35 Changed 7 years ago by
Status:  positive_review → 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 7 years ago by
Branch:  u/vdelecroix/19525 → u/jdemeyer/19525 

comment:37 Changed 7 years ago by
Commit:  71f52a44647b8ba440152420070e6a869f2190a1 → 76a11fd0a01ec366c844e81bef879b64746c0eb1 

Status:  needs_work → positive_review 
comment:38 Changed 7 years ago by
Branch:  u/jdemeyer/19525 → 76a11fd0a01ec366c844e81bef879b64746c0eb1 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Implement GLPK error handling