Failing tests when using system glpk #29493
Comments
|
comment:2
We should/will have a general discussion about things like this on sage-devel after 9.1 is released. |
|
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 |
|
comment:4
It's certainly simpler than adding the tag. However there are two problems:
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. |
|
comment:5
To me there are two issues:
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. |
|
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. |
|
comment:7
What problem was this patch trying to solve? The GLPK documentation states,
Given that the doctest in 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. |
|
comment:8
For example, one of the GLPK crashes that we catch and throw is, The problem? We assume that the index Tada, one down. |
|
comment:9
Maybe someone involved #20710 can help us figure out, if we can drop this patch. |
|
comment:10
Running That leaves: 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: I haven't computed a tableau in about a decade. Is that something we can check before we call |
|
comment:11
Replying to @orlitzky:
Yup. I'm in the process of fixing all of these. From the GLPK docs: Synopsis: |
|
Branch: u/mjo/ticket/29493 |
|
Commit: |
|
Author: Michael Orlitzky |
|
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:
|
|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
|
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: |
|
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 |
|
comment:43
Replying to @orlitzky:
... this appears to be a documentation issue. I have added a commit that addresses it. New commits:
|
|
comment:44
The two removal commits are now on #29829. |
|
Changed author from Michael Orlitzky to Michael Orlitzky, Matthias Koeppe |
|
Reviewer: Matthias Koeppe, ... |
|
comment:46
Tests run at https://github.com/mkoeppe/sage/actions/runs/129317169 |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
comment:48
New test at https://github.com/mkoeppe/sage/runs/755764195 |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
comment:50
New tests at https://github.com/mkoeppe/sage/actions/runs/130367945 |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
comment:52
Passes tests now - see for example https://github.com/mkoeppe/sage/runs/756778388 |
|
comment:53
I have also tested with https://github.com/mkoeppe/sage-numerical-interactive-mip (which uses some of these methods). I would be ready to set it to |
|
comment:55
The sage process never exited, but the error left GLPK unusable from that point forward -- for example returning nothing instead of solving subsequent problems as in the ticket description. The error handler itself is fine; it's toggling the "there was an error" bit off afterwards that's causing all this trouble. I still think it's perverse to leave around 150 lines of code with a docstring that says "don't use this," but where we are is not even locally optimal, so let's just do it. |
|
comment:56
Replying to @orlitzky:
... "for production". |
|
Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Michael Orlitzky |
|
comment:58
Thank you for doing this. |
|
Changed branch from u/mkoeppe/ticket/29493 to |
The sage distribution patches GLPK to be able to recover from errors. Upstream rejected this patch, said what it does is unsupported, and no one else has ever adopted it. Sage works fine without it. If someone really wants to add error recovery to GLPK, it has to be done in a way that upstream does not object to. Otherwise, it cannot be done reliably -- no linux distros or conda or homebrew are going to ship our patch. These changes date back to sagemath#29493, and this PR will close sagemath#29829. URL: sagemath#37801 Reported by: Michael Orlitzky Reviewer(s): Matthias Köppe
The sage distribution patches GLPK to be able to recover from errors. Upstream rejected this patch, said what it does is unsupported, and no one else has ever adopted it. Sage works fine without it. If someone really wants to add error recovery to GLPK, it has to be done in a way that upstream does not object to. Otherwise, it cannot be done reliably -- no linux distros or conda or homebrew are going to ship our patch. These changes date back to sagemath#29493, and this PR will close sagemath#29829. URL: sagemath#37801 Reported by: Michael Orlitzky Reviewer(s): Matthias Köppe
The sage distribution patches GLPK to be able to recover from errors. Upstream rejected this patch, said what it does is unsupported, and no one else has ever adopted it. Sage works fine without it. If someone really wants to add error recovery to GLPK, it has to be done in a way that upstream does not object to. Otherwise, it cannot be done reliably -- no linux distros or conda or homebrew are going to ship our patch. These changes date back to sagemath#29493, and this PR will close sagemath#29829. URL: sagemath#37801 Reported by: Michael Orlitzky Reviewer(s): Matthias Köppe
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
This doctest was mentioned before in #29317 with a suggestion for a fix.
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
glp_intopttoglp_simplex_then_intopt, which is more robust;CC: @jdemeyer @mkoeppe @orlitzky @jpflori @embray @sagetrac-gouezel @kiwifb @dcoudert @dimpase
Component: packages: standard
Keywords: glpk, patches
Author: Michael Orlitzky, Matthias Koeppe
Branch/Commit:
8acdf34Reviewer: Matthias Koeppe, Michael Orlitzky
Issue created by migration from https://trac.sagemath.org/ticket/29493
The text was updated successfully, but these errors were encountered: