Opened 7 years ago
Closed 6 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, 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/help-glpk/2015-11/msg00008.html
Change History (38)
comment:1 Changed 7 years ago by
- Dependencies set to #19527
comment:2 Changed 7 years ago by
- Cc ncohen added
comment:3 Changed 7 years ago by
- Branch set to u/jdemeyer/improve_glpk_error_handling
comment:4 Changed 7 years ago by
- Commit set to f808061d3d4cbaf2bd0cfe42f25fb67b5131a302
comment:5 Changed 7 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 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 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 7 years ago by
- Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.
comment:10 Changed 7 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 7 years ago by
- Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.
comment:12 Changed 7 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 7 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 7 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 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: ↓ 16 Changed 7 years ago by
Does a reviewer need to create a patched GLPK tarball himself?
comment:16 in reply to: ↑ 15 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:18 Changed 6 years ago by
Any news on the upstream situation?
comment:19 follow-up: ↓ 21 Changed 6 years ago by
- Status changed from needs_review to needs_work
does not apply on the latest beta
comment:20 Changed 6 years ago by
- 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: ↓ 22 Changed 6 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 6 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 6 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 6 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 6 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 6 years ago by
import "cysignals/signals.pxi" instead of "sage/ext/interrupt.pxi", see #20002 for details ;-)
comment:27 Changed 6 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 6 years ago by
- Status changed from needs_work to needs_review
comment:29 Changed 6 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 6 years ago by
See also #10232
comment:31 Changed 6 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 6 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 6 years ago by
If yes, then the ticket is good to go from my point of view.
comment:34 Changed 6 years ago by
- Reviewers set to Vincent Delecroix
- Status changed from needs_info to positive_review
comment:35 Changed 6 years ago by
- 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 6 years ago by
- Branch changed from u/vdelecroix/19525 to u/jdemeyer/19525
comment:37 Changed 6 years ago by
- Commit changed from 71f52a44647b8ba440152420070e6a869f2190a1 to 76a11fd0a01ec366c844e81bef879b64746c0eb1
- Status changed from needs_work to positive_review
comment:38 Changed 6 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