Failing tests when using system glpk

kliem commented 4 years ago

At the moment there are two failing doctests, when using the glpk from the system, e.g. on ubuntu eoan https://github.com/mkoeppe/sage/runs/542655821

sage -t src/sage/numerical/backends/glpk_backend.pyx
**********************************************************************
File "src/sage/numerical/backends/glpk_backend.pyx", line 2287, in sage.numerical.backends.glpk_backend.GLPKBackend.print_ranges
Failed example:
    p.print_ranges()
Expected:
    glp_print_ranges: optimal basic solution required
    1
Got:
    1

This doctest was mentioned before in #29317 with a suggestion for a fix.

 sage -t src/sage/libs/glpk/error.pyx
**********************************************************************
File "src/sage/libs/glpk/error.pyx", line 100, in sage.libs.glpk.error.setup_glpk_error_handler
Failed example:
    res = p.solve()
Expected:
          0: obj = ...
Got:
    <BLANKLINE>

The problem is that we have doctests that rely on error-recovery behavior added by a custom patch [#20710 comment:18], which wasn't accepted by upstream. (The doctest for the patch was added in #20832.)

The present ticket fixes the failures by

adding explicit input validation in several functions in the Cython wrappers so that the GLPK error handler is not reached;
changing the default optimization mode from glp_intopt to glp_simplex_then_intopt, which is more robust;
disabling the test for the error-recovery behavior, which provokes a crash with unpatched GLPK.

CC: @jdemeyer @mkoeppe @orlitzky @jpflori @embray @sagetrac-gouezel @kiwifb @dcoudert @dimpase

Component: packages: standard

Keywords: glpk, patches

Author: Michael Orlitzky, Matthias Koeppe

Branch/Commit: 8acdf34

Reviewer: Matthias Koeppe, Michael Orlitzky

Issue created by migration from https://trac.sagemath.org/ticket/29493

kliem commented 4 years ago

comment:1

What we could do is add a new testing flag style # optional - sage-glpk (and for any other package). This would than only be tested when sage build it's own version of the package.

This could also be used for #29092 and make the tests nicer for #29417.

kliem commented 4 years ago

comment:2

We should/will have a general discussion about things like this on sage-devel after 9.1 is released.

jhpalmieri commented 4 years ago

comment:3

How about a patch like this? Simpler than adding a new tag.

diff --git a/src/sage/doctest/parsing.py b/src/sage/doctest/parsing.py
index ebf7555106..503ad17e6f 100644
--- a/src/sage/doctest/parsing.py
+++ b/src/sage/doctest/parsing.py
@@ -44,6 +44,7 @@ optional_regex = re.compile(r'(py2|py3|long time|not implemented|not tested|know
 # which has not been patched, we need to ignore that message.
 # See :trac:`29317`.
 glpk_simplex_warning_regex = re.compile(r'(Long-step dual simplex will be used)')
+glpk_print_ranges_regex = re.compile(r'(glp_print_ranges: optimal basic solution required)')
 find_sage_prompt = re.compile(r"^(\s*)sage: ", re.M)
 find_sage_continuation = re.compile(r"^(\s*)\.\.\.\.:", re.M)
 find_python_continuation = re.compile(r"^(\s*)\.\.\.([^\.])", re.M)
@@ -1077,6 +1078,7 @@ class SageOutputChecker(doctest.OutputChecker):
         """
         got = self.human_readable_escape_sequences(got)
         got = glpk_simplex_warning_regex.sub('', got)
+        got = glpk_print_ranges_regex.sub('', got)
         if isinstance(want, MarkedOutput):
             if want.random:
                 return True
diff --git a/src/sage/libs/glpk/error.pyx b/src/sage/libs/glpk/error.pyx
index f7046b3ec4..26faf1e8d9 100644
--- a/src/sage/libs/glpk/error.pyx
+++ b/src/sage/libs/glpk/error.pyx
@@ -98,7 +98,7 @@ def setup_glpk_error_handler():
         sage: p.add_constraint(x >= 0)
         sage: p.set_objective(x + y)
         sage: res = p.solve()
-              0: obj = ...
+        ...
         sage: res  # rel tol 1e-15
         2.4
     """
diff --git a/src/sage/numerical/backends/glpk_backend.pyx b/src/sage/numerical/backends/glpk_backend.pyx
index b1acf12a28..4b2d12eddc 100644
--- a/src/sage/numerical/backends/glpk_backend.pyx
+++ b/src/sage/numerical/backends/glpk_backend.pyx
@@ -2285,7 +2285,6 @@ cdef class GLPKBackend(GenericBackend):
             sage: import sage.numerical.backends.glpk_backend as backend
             sage: p.solver_parameter(backend.glp_simplex_or_intopt, backend.glp_simplex_only)
             sage: p.print_ranges()
-            glp_print_ranges: optimal basic solution required
             1
             sage: p.solve()
             0

kliem commented 4 years ago

comment:4

It's certainly simpler than adding the tag. However there are two problems:

In #20710 people agreed that this patch is necessary. Is it ok to ignore it, if the system package is present? Do we warn the user that this package has some API problems with sage? (This is a more general issue, as there should be some agreement on how much failure do we accept when a system package is used.)
Isn't your approach the same as removing those tests. That would lead to us never noticing when #20710 breaks (unless the patch causes build failure of course).

The idea of the tag is that there might be tests that only pass when the sage-build package is used, e.g. #29092. With the tag we would properly document that this works with sage install of the package, but it might not otherwise. But there are probably better ideas out there.

jhpalmieri commented 4 years ago

comment:5

To me there are two issues:

what are these doctests trying to test? For the one in glpk_backend.pyx, is the extra message useful or can it be ignored? I don't know the answers, by the way.
re whether the patch is necessary: that should have been a discussion in the ticket which set up the spkg-configure.m4 file for glpk, which allows users to use the system version instead of Sage's. Should build/pkgs/glpk/spkg-configure.m4 somehow test this feature of Sage's glpk and not allow others?

Adding a tag to the doctest so it is only run when Sage's glpk is used also ignores whether the patch is necessary, so it doesn't really solve the problem, except to test whether Sage's glpk built correctly. Since the momentum is toward using more system packages, people who know about glpk (i.e., not me) need to decide whether to continue allowing use of unpatched system versions. If so, then in my opinion we should test Sage's modifications to glpk in its own test suite (spkg-check) and fix the doctests so they work with all versions.

kliem commented 4 years ago

comment:6

I agree that moving the second test makes more sense than an extra flag. (Unless of course people decide that this is unacceptable.)

The first tests should stay there though, as I don't think it is there to test the warning message. So your fix would be just fine.

orlitzky commented 4 years ago

comment:7

What problem was this patch trying to solve? The GLPK documentation states,

If some GLPK API routine detects erroneous or incorrect data passed by the application program, it writes appropriate diagnostic messages to the terminal and then abnormally terminates the application program. In most practical cases this allows to simplify programming by avoiding numerous checks of return codes. Thus, in order to prevent crashing the application program should check all data, which are suspected to be incorrect, before calling GLPK API routines.

Should note that this kind of error handling is used only in cases of incorrect data passed by the application program. If, for example, the application program calls some GLPK API routine to read data from an input file and these data are incorrect, the GLPK API routine reports about error in the usual way by means of the return code.

Given that the doctest in sage/libs/glpk/error.pyx has to go to great lengths to crash glpk...

sage: cython('''
....: # distutils: libraries = glpk z gmp
....: from cysignals.signals cimport sig_on, sig_off
....: from sage.libs.glpk.env cimport glp_term_out
....:
....: sig_on()
....: glp_term_out(12345)  # invalid value
....: sig_off()
....: ''')
Traceback (most recent call last):

what sort of real problems are we expecting this to catch during normal sage usage? Is there any place that a user passes data directly to glpk that we can't inspect/preprocess first, to ensure that it's correct? If not, maybe we should just crash sage when glpk crashes and drop the patch, because any crash of glpk is a low-level sage programming error.

orlitzky commented 4 years ago

comment:8

For example, one of the GLPK crashes that we catch and throw is,

sage: from sage.numerical.backends.generic_backend import get_solver
sage: p = get_solver(solver="GLPK")
sage: p.variable_upper_bound(2)
...
GLPKError: glp_get_col_ub: j = 3; column number out of range

The problem? We assume that the index 2 was valid, and use GLPK crashing as an indication that it isn't. Instead, we should just check that the index is valid (i.e. use the API correctly):

sage: from sage.numerical.backends.generic_backend import get_solver
sage: p = get_solver(solver="GLPK")
sage: p.ncols()
0
sage: p.add_variable()
0
sage: p.add_variable()
1
sage: p.add_variable()
2
sage: p.ncols()
3
sage: p.variable_upper_bound(2)
<no crash>

Tada, one down.

kliem commented 4 years ago

comment:9

Maybe someone involved #20710 can help us figure out, if we can drop this patch.

orlitzky commented 4 years ago

comment:10

Running git grep GLPKError shows all of the instances that we need to worry about. And all except two look like missing bounds checks that can be turned into aValueError.

That leaves:

.. WARNING::

    Sage uses GLPK's ``glp_intopt`` to find solutions.
    This routine sometimes FAILS CATASTROPHICALLY
    when given a system it cannot solve. (:trac:`12309`.)
    Here, "catastrophic" can mean either "infinite loop" or
    segmentation fault. Upstream considers this behavior
    "essentially innate" to their design, and suggests
    preprocessing it with ``glp_simplex`` first.
    Thus, if you suspect that your system is infeasible,
    set the ``preprocessing`` option first.

EXAMPLES::

    sage: lp = MixedIntegerLinearProgram(solver = "GLPK")
    sage: v = lp.new_variable(nonnegative=True)
    sage: lp.add_constraint(v[1] +v[2] -2.0 *v[3], max=-1.0)
    sage: lp.add_constraint(v[0] -4.0/3 *v[1] +1.0/3 *v[2], max=-1.0/3)
    sage: lp.add_constraint(v[0] +0.5 *v[1] -0.5 *v[2] +0.25 *v[3], max=-0.25)
    sage: lp.solve()
    0.0
    sage: lp.add_constraint(v[0] +4.0 *v[1] -v[2] +v[3], max=-1.0)
    sage: lp.solve()
    Traceback (most recent call last):
    ...
    GLPKError: Assertion failed: ...
    sage: lp.solver_parameter("simplex_or_intopt", "simplex_then_intopt")
    sage: lp.solve()
    Traceback (most recent call last):
    ...
    MIPSolverException: GLPK: Problem has no feasible solution

I've included the WARNING block from just prior to the doctest, because it tells us how to fix this problem. Instead of an unsafe default, we should enable preprocessing but let the user disable it if he thinks he knows what he's doing (and is willing to crash sage if he's wrong).

And the other instance:

cpdef eval_tab_row(self, int k):
    r"""
    Computes a row of the current simplex tableau.
    ...
    try:
        sig_on()            # to catch GLPKError
        i = glp_eval_tab_row(self.lp, k + 1, c_indices, c_values)
        sig_off()
    except GLPKError:
        raise MIPSolverException('GLPK: basis factorization does not exist; or variable must be basic')

I haven't computed a tableau in about a decade. Is that something we can check before we call glp_eval_tab_row()?

orlitzky commented 4 years ago

comment:11

Replying to @orlitzky:

I haven't computed a tableau in about a decade. Is that something we can check before we call glp_eval_tab_row()?

Yup. I'm in the process of fixing all of these. From the GLPK docs:

Synopsis: int glp_bf_exists(glp_prob *P); Comments: ... So before calling any API routine, which uses the basis factorization, the application program must make sure (using the routine glp_bf_exists) that the factorization exists and therefore available for computations.

orlitzky commented 4 years ago

Branch: u/mjo/ticket/29493

orlitzky commented 4 years ago

Commit: f5edb44

orlitzky commented 4 years ago

Author: Michael Orlitzky

orlitzky commented 4 years ago

comment:12

Here's a preliminary branch. I haven't run a ptestlong yet, and you can definitely crash sage if you ignore the warnings and disable the safety net. But I've basically eliminated the need for our custom GLPK error handling, and thus the need for the rejected patch to reset the error state.

New commits:

`e39cffd`	`Trac #29493: add sanity checks for GLPK's variable bound methods.`
`3fec617`	`Trac #29493: add missing bounds checks for other GLPK backend methods.`
`3a1b741`	`Trac #29493: add bounds checks to GLPK's add_linear_constraint() method.`
`3b75fc0`	`Trac #29493: add bounds checks to GLPK's get_col_dual() method.`
`51824a6`	`Trac #29493: verify input to GLPK row/column tableaux methods.`
`c9507a0`	`Trac #29493: make simplex_then_intopt the default GLPK method.`
`82a7527`	`Trac #29493: remove doctest that intentionally crashes GLPK.`
`7d8db10`	`Trac #29493: remove glpk.error extension module.`
`f5edb44`	`Trac #29493: drop rejected GLPK patch.`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from f5edb44 to 1cc9f61

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

`20e92ef`	`Trac #29493: make simplex_then_intopt the default GLPK method.`
`83ea75a`	`Trac #29493: remove doctest that intentionally crashes GLPK.`
`c9b9a24`	`Trac #29493: remove glpk.error and glpk.env extension modules.`
`1cc9f61`	`Trac #29493: drop GLPK patch that was rejected by upstream.`

orlitzky commented 4 years ago

comment:14

A ptestlong passes now, and I've cleaned up the commit messages. This is the only pseudo-regression introduced, if you ignore the warning about changing the default solver to intopt only:

sage: lp = MixedIntegerLinearProgram(solver = "GLPK")
sage: v = lp.new_variable(nonnegative=True)
sage: lp.add_constraint(v[1] +v[2] -2.0 *v[3], max=-1.0)
sage: lp.add_constraint(v[0] -4.0/3 *v[1] +1.0/3 *v[2], max=-1.0/3)
sage: lp.add_constraint(v[0] +0.5 *v[1] -0.5 *v[2] +0.25 *v[3], max=-0.25)
sage: lp.add_constraint(v[0] +4.0 *v[1] -v[2] +v[3], max=-1.0)
sage: lp.solver_parameter("simplex_or_intopt", "intopt_only")
sage: lp.solve()
Assertion failed: col->lb < col->ub
Error detected in file npp/npp5.c at line 528
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-8-34bddfdcab75> in <module>()
----> 1 lp.solve()

/home/mjo/src/sage.git/local/lib/python3.7/site-packages/sage/numerical/mip.pyx in sage.numerical.mip.MixedIntegerLinearProgram.solve (build/cythonized/sage/numerical/mip.c:15418)()
   2249         """
   2250         if log is not None: self._backend.set_verbosity(log)
-> 2251         self._backend.solve()
   2252         return self._backend.get_objective_value()
   2253 

/home/mjo/src/sage.git/local/lib/python3.7/site-packages/sage/numerical/backends/glpk_backend.pyx in sage.numerical.backends.glpk_backend.GLPKBackend.solve (build/cythonized/sage/numerical/backends/glpk_backend.c:9837)()
   1136         if ((self.simplex_or_intopt == glp_intopt_only)
   1137             or (self.simplex_or_intopt == glp_simplex_then_intopt) and (solution_status != GLP_UNDEF) and (solution_status != GLP_NOFEAS)):
-> 1138             sig_on()
   1139             solve_status = glp_intopt(self.lp, self.iocp)
   1140             solution_status = glp_mip_status(self.lp)

RuntimeError: Aborted

kliem commented 4 years ago

comment:15

I'm running tests here:

https://github.com/kliem/sage-test-27122/actions

e.g. the ubuntu eoan test will be here https://github.com/kliem/sage-test-27122/runs/731034181

(the naming of the repository is confusing, but I'm really only testing u/mjo/ticket/29493 on top of #29757).

jhpalmieri commented 4 years ago

comment:16

Tests pass on OS X with homebrew's glpk.

mkoeppe commented 4 years ago

comment:17

I've only had a chance to take a brief look. Just a quick comment that a possible concern is that exception types are changed by this ticket, which may break user code.

kliem commented 4 years ago

comment:18

Replying to @kliem:

I'm running tests here:

https://github.com/kliem/sage-test-27122/actions

e.g. the ubuntu eoan test will be here https://github.com/kliem/sage-test-27122/runs/731034181

(the naming of the repository is confusing, but I'm really only testing u/mjo/ticket/29493 on top of #29757).

Seems to work fine. However, the github testing workflow doesn't work very smoothly right now.

kliem commented 4 years ago

comment:19

Replying to @mkoeppe:

I've only had a chance to take a brief look. Just a quick comment that a possible concern is that exception types are changed by this ticket, which may break user code.

Well we invented the GLPKError. I would propose leaving GLPKError with a deprecation warning and also having it inherit from ValueError.

This way except GLPKError would still work. except MIPSolverException would sometimes not work anymore without a proper warning and the user will have to find out that this is sometimes a ValueError now.

orlitzky commented 4 years ago

comment:20

I don't think deprecating GLPKError makes sense here. If someone was actually catching a GLPKError in the only place it was documented, they'll now be getting a crash instead.

The other places where a GLPKError could have been raised are (still) undocumented. The GenericBackend interface is not quite done cooking in that regard. What happens if you ask for a variable that doesn't exist? What happens if try to set the type of a variable to a type that doesn't exist? The interface makes no promises. So the fact that one of the implementations now raises a different undocumented error from the previous undocumented error is something I don't think we have to be accountable for.

kliem commented 4 years ago

comment:21

sage: from sage.misc.superseded import deprecation
sage: class MyError(ValueError):
....:     def __init__(self, s):
....:         deprecation(1, 'foo')
....:         return ValueError.__init__(self, s)
....:     
sage: try:
....:     raise MyError("hello")
....: except MyError:
....:     pass
....: 
/home/jonathan/Applications/sage/src/bin/sage-ipython:2: DeprecationWarning: foo
See http://trac.sagemath.org/1 for details.
  # -*- coding: utf-8 -*-

This works fine. This is what I had in mind.

kliem commented 4 years ago

comment:22

Actually, no.

I had this in mind

sage: try:
....:     raise ValueError("hello")
....: except MyError:
....:     pass
....:

An this will not raise a deprecation warning. It won't even work (edited).

kliem commented 4 years ago

comment:23

Ok, I think I understand the problem now. I tried a few things, but I think it is just not possible.

So this ticket will possibly break users code and there is nothing we can do about it?

orlitzky commented 4 years ago

comment:24

Replying to @kliem:

So this ticket will possibly break users code and there is nothing we can do about it?

Every bugfix carries some risk of breaking user code. Python has no notion of "private", and sage doesn't distinguish between public/private classes (exceptions, in this case), or use semantic versioning, or anything else like that as a heuristic aid. Instead we have deprecation warnings, but... every bugfix carries some risk of breaking user code. So whether or not to deprecate comes down to a judgment call: how likely is this to break running code, how much will it hurt if that happens, and how annoying would it be to avoid doing so?

In this case, I didn't get the impression that the API was at the level of stability where it makes sense for every bugfix to be a year-long endeavor. I feel much more guilty about introducing a crash where previously an exception was raised. Changing the default method to simplex_then_intopt tries hard to avoid that, though; and really, there's no way to get rid of the custom glpk patch without ripping that band-aid off at some point.

mkoeppe commented 4 years ago

comment:25

I'm not overly concerned about breaking user code in this case. But it would be worth checking (or commenting on, if you have checked already) whether the differences to other backends are decreased or increased by this change.

A while back when I was working on cleaning up these backends, I introduced a number of _test... methods in GenericBackend to make sure that the API is actually the same; more could be added.

mkoeppe commented 4 years ago

comment:26

Replying to @orlitzky:

What problem was this patch trying to solve?

I was not around when the patch was introduced, but nevertheless let me make a few comments on this.

The GLPK documentation states,

If some GLPK API routine detects erroneous or incorrect data passed by the application program, it writes appropriate diagnostic messages to the terminal and then abnormally terminates the application program.

... and of course this is a rather unfortunate design for a library, and terrible for development in particular in an interactive environment. Basically, when working on new interfacing code, make one mistake and your session is killed.

I guess that there was hope at the time that the patch could be upstreamed, but of course now we just have to accept that GLPK is a stable target. So we now simply have to accept that we have to duplicate all input validation checks that GLPK is already doing. Fine.

Is there any place that a user passes data directly to glpk that we can't inspect/preprocess first, to ensure that it's correct?

I don't know the answer to this question.

If not, maybe we should just crash sage when glpk crashes and drop the patch, because any crash of glpk is a low-level sage programming error.

Well, I think the error recovery patch is still valuable for sage-the-distribution, so I don't think it should be removed. It adds value for Sage developers because it makes interactive development with the GLPK API simpler.

orlitzky commented 4 years ago

comment:27

Replying to @mkoeppe:

I'm not overly concerned about breaking user code in this case. But it would be worth checking (or commenting on, if you have checked already) whether the differences to other backends are decreased or increased by this change.

I considered this when fixing all of the methods that segfault. I had two choices in most cases: return garbage, or throw an error. For example,

sage: from sage.numerical.backends.generic_backend import get_solver
sage: p = get_solver(solver="PPL")
sage: p.row(12)
...
IndexError: list index out of range

sage: p = get_solver(solver="CVXOPT")
sage: p.row(12)
([], [])

That IndexError is an implementation detail since it tries to index a member variable list. Nothing is documented. But eliminating the option to segfault makes it more consistent, I guess? The methods should all be made to agree, of course, and the behavior documented. Another place I noticed some incongruity is that every backend returns garbage for the is-variable-foo methods:

sage: from sage.numerical.backends.generic_backend import get_solver
sage: p = get_solver(solver="PPL")
sage: p.is_variable_binary(12)
False
sage: p.is_variable_integer(12)
False
sage: p.is_variable_continuous(12)
True
sage: p = get_solver(solver="CVXOPT")
sage: p.is_variable_binary(12)
False
sage: p.is_variable_integer(12)
False
sage: p.is_variable_continuous(12)
True
sage: p = get_solver(solver="GLPK")
sage: p.is_variable_binary(12)
False
sage: p.is_variable_integer(12)
False
sage: p.is_variable_continuous(12)
False

As the last line shows, I opted for the garbage False that is more intuitive to me than garbage True. But if I were doing it from scratch, that would also be a (documented in the interface) error.

orlitzky commented 4 years ago

comment:28

Replying to @mkoeppe:

If not, maybe we should just crash sage when glpk crashes and drop the patch, because any crash of glpk is a low-level sage programming error.

Well, I think the error recovery patch is still valuable for sage-the-distribution, so I don't think it should be removed. It adds value for Sage developers because it makes interactive development with the GLPK API simpler.

If we keep the patch and ever take advantage of it, we'll be right back where we started, won't we?

The upstream maintainers and documentation have been pretty clear that you need to check the parameters you pass to GLPK. That's just the way the API works, and it really wasn't very much trouble to do. And so long as we do it, there's no urgent need for the error-recovery patch, because errors will never happen. The only real exception to my last statement is when you have an infeasible problem, ignore the warning, and set the method to intopt_only to skip the feasibility check performed by the continuous simplex solver. It's nice that we provide low-level access to that ability for people who really need it, but it's just that: low level access that you shouldn't use without reading the documentation and understanding what it does.

To summarize:

the patch is only helpful in a rare case (or when we use the API wrong)
...and if you ignore a warning
...and are wrong about how feasible your problem is
we can't take advantage of the patch in sagelib without breaking distros
the upstream developers have said "don't do that" multiple times

Even if we left it as "something for other people to use," I think it would be pretty annoying to write some sage code that works on your machine but not on your friend's because he's running debian and their glpk isn't patched.

mkoeppe commented 4 years ago

comment:29

Replying to @orlitzky:

Replying to @mkoeppe:

If not, maybe we should just crash sage when glpk crashes and drop the patch, because any crash of glpk is a low-level sage programming error.

Well, I think the error recovery patch is still valuable for sage-the-distribution, so I don't think it should be removed. It adds value for Sage developers because it makes interactive development with the GLPK API simpler.

If we keep the patch and ever take advantage of it, we'll be right back where we started, won't we?

No, we won't, obviously.

The input checking that you add on this ticket is valuable because it helps avoid using the GLPK API incorrectly, as you explain correctly above.

orlitzky commented 4 years ago

comment:30

Replying to @mkoeppe:

No, we won't, obviously.

The input checking that you add on this ticket is valuable because it helps avoid using the GLPK API incorrectly, as you explain correctly above.

Are we talking about the same thing? I'm assuming that you're talking about the patch that upstream rejected and that I deleted, that resets the error state back to zero. If we keep that patch in sage-the-distribution, and if anyone ever relies on it, we would be back in the situation that prompted this ticket: some behavior depends on the custom patch to work, and it will break on distros that don't carry the patch.

I'm also presuming that we don't want to enable that. But then how does keeping the patch around help anyone?

mkoeppe commented 4 years ago

comment:31

Replying to @orlitzky:

I'm assuming that you're talking about the patch that upstream rejected and that I deleted, that resets the error state back to zero.

That's right, I do not support removing the patch.

Because, as I said above:

It adds value for Sage developers because it makes interactive development with the GLPK API simpler.

mkoeppe commented 4 years ago

comment:32

Replying to @mkoeppe:

Replying to @orlitzky:

I'm assuming that you're talking about the patch that upstream rejected and that I deleted, that resets the error state back to zero.

That's right, I do not support removing the patch.

... and the error handler.

mkoeppe commented 4 years ago

comment:33

Replying to @orlitzky:

Replying to @mkoeppe:

it would be worth checking (or commenting on, if you have checked already) whether the differences to other backends are decreased or increased by this change.

I considered this when fixing all of the methods that segfault. I had two choices in most cases: return garbage, or throw an error. [...] That IndexError is an implementation detail since it tries to index a member variable list. Nothing is documented. But eliminating the option to segfault makes it more consistent, I guess?

Yes, this is good.

The methods should all be made to agree, of course, and the behavior documented. Another place I noticed some incongruity is that every backend returns garbage for the is-variable-foo methods [...] I opted for the garbage False that is more intuitive to me than garbage True.

Sure, that's fine.

But if I were doing it from scratch, that would also be a (documented in the interface) error.

I agree, and I would support introducing tighter error checking there (on some other ticket). This is clearly undefined behavior, and tightening the specification would be valuable.

orlitzky commented 4 years ago

comment:34

Replying to @mkoeppe:

That's right, I do not support removing the patch. ... and the error handler.

Some of the intermediate commits might otherwise be useful, but eliminating the patch and the error handler (that are causing test failures everywhere) was the whole point here =)

mkoeppe commented 4 years ago

comment:35

Let's see.

The doctest failure in sage.numerical.backends.glpk_backend.GLPKBackend.print_ranges is solved by your changes using glp_bf_exists.

The doctests that relied on the error handler for index bound checking etc. are solved by your additions to the Cython functions.

The doctest for sage.libs.glpk.error.setup_glpk_error_handler had become inappropriate now that we can use system GLPKs, so removing the doctest (or marking it "# optional - glpk_error_recovery") is sufficient to fix this doctest failure. It is not necessary to remove the error recovery feature for that.

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -27,5 +27,13 @@
     <BLANKLINE>

-The problem seems to be that we have a custom patch [#20710 comment:18], -which wasn't accepted by upstream. The doctest was added in #20832. +The problem is that we have doctests that rely on error-recovery behavior added by a custom patch [#20710 comment:18], +which wasn't accepted by upstream. (The doctest for the patch was added in #20832.) + +The present ticket fixes the failures by + adding explicit input validation in several functions in the Cython wrappers so that the GLPK error handler is not reached; + changing the default optimization mode from glp_intopt to glp_simplex_then_intopt, which is more robust; +* disabling the test for the error-recovery behavior, which provokes a crash with unpatched GLPK. + + +

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -32,7 +32,7 @@

 The present ticket fixes the failures by 
 * adding explicit input validation in several functions in the Cython wrappers so that the GLPK error handler is not reached;
-* changing the default optimization mode from ``glp_intopt`` to ``glp_simplex_then_intopt``, which is more robust;
+* changing the default optimization mode from `glp_intopt` to `glp_simplex_then_intopt`, which is more robust;
 * disabling the test for the error-recovery behavior, which provokes a crash with unpatched GLPK.

orlitzky commented 4 years ago

comment:38

Replying to @mkoeppe:

It is not necessary to remove the error recovery feature for that.

I also had to change the default solver method, and remove the doctest that purposely crashed GLPK.

Deleting the tests would have made the errors go away in the first place, but that doesn't solve the underlying problem: there are two fundamentally different behaviors, and you don't know which one you're going to get unless you know what distro your code will be running on (and what ./configure options will be used to build sage). That problem is less prominent after some of these commits, but still lurking unless we get rid of the custom patch.

If I'm writing a new sage package and if I rely on being able to catch GLPKError, then as soon as I send it to someone running Ubuntu, it's going to crash. I'll probably post to the mailing list, be linked to this ticket, and ultimately decide that I should be sanitizing my input before passing it to GLPK for the best results. But then what good has the patch done me, except to waste some time on a false start?

mkoeppe commented 4 years ago

comment:39

Replying to @orlitzky:

Replying to @mkoeppe:

It is not necessary to remove the error recovery feature for that.

I also had to change the default solver method, and remove the doctest that purposely crashed GLPK.

Yes, I know, see my updated description.

mkoeppe commented 4 years ago

comment:40

Replying to @orlitzky:

Deleting the tests would have made the errors go away in the first place, but that doesn't solve the underlying problem: there are two fundamentally different behaviors, and you don't know which one you're going to get unless you know what distro your code will be running on (and what ./configure options will be used to build sage). That problem is less prominent after some of these commits, but still lurking unless we get rid of the custom patch.

It is undefined behavior. We have a patch that makes it less deadly. It's a convenience for developers. I know that it has actually save my time during development (in contrast to the hypothetical developer that you are invoking), and as I have said above, I don't support removing the feature.

In any case, the removal is not necessary for this ticket. It is best to limit the scope of the ticket.

mkoeppe commented 4 years ago

comment:41

As a way forward, I have created #29829 for the removal of the patch (which I do not support).

Let's narrow down the changes in the present ticket to the uncontroversial part.

mkoeppe commented 4 years ago

Changed branch from u/mjo/ticket/29493 to u/mkoeppe/ticket/29493

mkoeppe commented 4 years ago

Changed commit from 1cc9f61 to 066b1e8

mkoeppe commented 4 years ago

comment:43

Replying to @orlitzky:

If I'm writing a new sage package and if I rely on being able to catch GLPKError

... this appears to be a documentation issue. I have added a commit that addresses it.

New commits:

`83629f5`	`sage.libs.glpk.error: Add documentation discouraging users from using GLPKError`
`066b1e8`	`setup_glpk_error_handler: Disable doctest that tests the GLPK error recovery patch`

mkoeppe commented 4 years ago

comment:44

The two removal commits are now on #29829.

mkoeppe commented 4 years ago

Changed author from Michael Orlitzky to Michael Orlitzky, Matthias Koeppe

sagemath / sage

Failing tests when using system glpk #29493