Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src/sage/graphs/generic_graph.py: work around doctest hang #38851

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

orlitzky
Copy link
Contributor

One doctest in this file is "hanging" on ARM64 and RISC-V as GLPK tries courageously to solve a MIP. A tweak to the solver options allows this problem to be solved on those two architectures without affecting any others. This is unlikely to solve the general problem, but it may buy us some time.

@orlitzky orlitzky marked this pull request as draft October 24, 2024 22:23
Copy link

github-actions bot commented Oct 24, 2024

Documentation preview for this PR (built with commit f592087; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done only when the solver is glpk. I have cplex by default and get

sage: G.edge_disjoint_spanning_trees(2, solver='cplex')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[5], line 1
----> 1 G.edge_disjoint_spanning_trees(Integer(2), solver='cplex')

File ~/sage/src/sage/graphs/generic_graph.py:7373, in GenericGraph.edge_disjoint_spanning_trees(self, k, algorithm, root, solver, verbose, integrality_tolerance)
   7360 # We now solve this program and extract the solution
   7361 try:
   7362     # The MIP approach with GLPK is prone to compiler and
   7363     # optimization-level weirdness on some hardware:
   (...)
   7371     # that it probably hasn't badly broken some other use
   7372     # case.
-> 7373     p.solver_parameter("presolve_intopt", False)
   7374 except KeyError:
   7375     # Must not be using GLPK...
   7376     pass

File ~/sage/src/sage/numerical/mip.pyx:2863, in sage.numerical.mip.MixedIntegerLinearProgram.solver_parameter()
   2861         return self._backend.solver_parameter(name)
   2862     else:
-> 2863         self._backend.solver_parameter(name, value)
   2864 
   2865 cpdef sum(self, L):

File sage_numerical_backends_cplex/cplex_backend.pyx:1632, in sage_numerical_backends_cplex.cplex_backend.CPLEXBackend.solver_parameter()

ValueError: This parameter is not available.

@orlitzky
Copy link
Contributor Author

This should be done only when the solver is glpk.

Yeah I tried, but I missed a few exceptions. I've added NotImplementedError and ValueError to the list.

@orlitzky
Copy link
Contributor Author

@collares can you please test this on aarch64? I think it will work, but for the commit message to not be a lie, it has to be attempted.

@dcoudert
Copy link
Contributor

you can check the backend.

sage: from sage.numerical.mip import MixedIntegerLinearProgram
sage: from sage.numerical.backends.glpk_backend import GLPKBackend
sage: p = MixedIntegerLinearProgram(maximization=True)
sage: p.get_backend()
<sage_numerical_backends_cplex.cplex_backend.CPLEXBackend object at 0x13e0c14c0>
sage: isinstance(p.get_backend(), GLPKBackend)
False
sage: p = MixedIntegerLinearProgram(maximization=True, solver='glpk')
sage: p.get_backend()
<sage.numerical.backends.glpk_backend.GLPKBackend object at 0x13e042460>
sage: isinstance(p.get_backend(), GLPKBackend)
True

One doctest in this file is "hanging" on ARM64 and RISC-V as GLPK
tries courageously to solve a MIP. A tweak to the solver options
allows this problem to be solved on those two architectures without
affecting any others. This is unlikely to solve the general problem,
but it may buy us some time.

Closes sagemath#34575
Closes sagemath#38831
@orlitzky
Copy link
Contributor Author

you can check the backend.

Much easier, thank you.

@orlitzky
Copy link
Contributor Author

CI says it's OK.

There's a risk that I've made things worse on "normal" systems in some way that we don't yet measure, but we could always revert it if random failures start popping up on x86_64 machines.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dcoudert
Copy link
Contributor

If other random issues occur, we can either revert this change, or prevent using some solvers for this method, or...

Unfortunately, I will not be able to work on the combinatorial algorithm in the forthcoming months. I can be a project for next GSoC if we find a strong student. It's a hard work.

@orlitzky
Copy link
Contributor Author

orlitzky commented Oct 25, 2024

Thanks.

After I posted the GLPK bug report, I was contacted by one of the HiGHS developers who basically asked, "why are you still using GLPK?!?!" It's a fair question. HiGHS is able to solve this problem, and is actively maintained by a paid developer.

HiGHS already has a python interface, so it shouldn't be too hard to create a sage-numerical-backends-highs package like we have for CPLEX. It also doesn't look too hard to package HiGHS for linux/homebrew/conda/etc, which makes it a strong candidate for replacing GLPK as the default IMO.

@dcoudert
Copy link
Contributor

dcoudert commented Oct 25, 2024

indeed. this seems a very good idea. We already have a backend for scip. We can certainly work on a backend for HiGHS and then drop glpk. You can open an issue to monitor this task.
In the long term we will have to improve all backends for faster interactions. We currently wasting a lot of time passing models to solvers, we don't have access to dual variables, we cannot do column generation, etc.

Copy link
Contributor

@collares collares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm that the relevant test no longer hangs on aarch64-linux with this patch

@orlitzky
Copy link
Contributor Author

Great, thank you.

@vbraun vbraun merged commit ce4a78d into sagemath:develop Oct 26, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants