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.

Sign up for GitHub

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

Jump to bottom

Drop custom GLPK patch and error handling #37801

Merged
merged 3 commits into from
Jun 22, 2024
Merged

Drop custom GLPK patch and error handling #37801

merged 3 commits into from
Jun 22, 2024

Conversation

orlitzky
Copy link
Contributor

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 #29493, and this PR will close #29829.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 14, 2024 •
edited
Loading

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

@orlitzky
Copy link
Contributor Author

@mkoeppe are these CI failures a known issue? e.g.

2024-04-15T12:54:14.3132521Z #25 3690.5 cat: logs/install.time: No such file or directory
2024-04-15T12:54:14.3133024Z #25 3690.5 make: *** [Makefile:40: doc-html] Error 1

is (AFAICT) eventually causing

2024-04-15T14:01:25.4315886Z #25 7721.7 make[1]: *** [Makefile:240: test] Error 1
2024-04-15T14:01:25.4316823Z #25 7721.7 make[1]: Leaving directory '/sage'
2024-04-15T14:01:25.4317485Z #25 7721.7 make: *** [Makefile:255: ptest] Error 2
2024-04-15T14:01:25.4318987Z #25 ERROR: process "/bin/sh -c make SAGE_SPKG=\"sage-spkg -y -o\" ${USE_MAKEFLAGS} ${TARGETS}" did not co\
mplete successfully: exit code: 2

@mkoeppe
Copy link
Member

mkoeppe commented Apr 16, 2024

2024-04-15T12:54:14.3132521Z #25 3690.5 cat: logs/install.time: No such file or directory

This one is cosmetic and does not cause failures. Fixed in:

@mkoeppe
Copy link
Member

mkoeppe commented Apr 16, 2024

2024-04-15T12:54:14.3133024Z #25 3690.5 make: *** [Makefile:40: doc-html] Error 1

This one is:

Help with figuring out this one would be much appreciated.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 16, 2024

is (AFAICT) eventually causing

2024-04-15T14:01:25.4315886Z #25 7721.7 make[1]: *** [Makefile:240: test] Error 1
2024-04-15T14:01:25.4316823Z #25 7721.7 make[1]: Leaving directory '/sage'
2024-04-15T14:01:25.4317485Z #25 7721.7 make: *** [Makefile:255: ptest] Error 2
2024-04-15T14:01:25.4318987Z #25 ERROR: process "/bin/sh -c make SAGE_SPKG=\"sage-spkg -y -o\" ${USE_MAKEFLAGS} ${TARGETS}" did not co\
mplete successfully: exit code: 2

Yes, the doc-html build failure is what causes this.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 16, 2024

By the way, in https://github.com/sagemath/sage/actions/runs/8688244353/job/23823517261?pr=37801#step:11:6372 (for gentoo-python3.11-minimal`) I see a bunch of failures (obviously unrelated to the present PR, of course), including this one:

#24 5692.7 **********************************************************************
#24 5692.7 File "src/sage/categories/simplicial_sets.py", line 596, in sage.categories.simplicial_sets.SimplicialSets.Pointed.ParentMethods._canonical_twisting_operator
#24 5692.7 Failed example:
#24 5692.7     d
#24 5692.7 Expected:
#24 5692.7     {(s_0 v_0, sigma_1): f3, (sigma_1, s_0 v_0): f2*f3^-1, (sigma_1, sigma_1): f2}
#24 5692.7 Got:
#24 5692.7     {(s_0 v_0, sigma_1): f2, (sigma_1, s_0 v_0): f1*f2^-1, (sigma_1, sigma_1): f1}
#24 5692.7 **********************************************************************
#24 5692.7 File "src/sage/categories/simplicial_sets.py", line 598, in sage.categories.simplicial_sets.SimplicialSets.Pointed.ParentMethods._canonical_twisting_operator
#24 5692.7 Failed example:
#24 5692.7     list(d.values())[0].parent()
#24 5692.7 Expected:
#24 5692.7     Multivariate Laurent Polynomial Ring in f2, f3 over Integer Ring
#24 5692.7 Got:
#24 5692.7     Multivariate Laurent Polynomial Ring in f1, f2 over Integer Ring

(There are also a bunch of failures from a different version of Giac but you probably know about this already.)

@mkoeppe
Copy link
Member

mkoeppe commented Apr 16, 2024

OK, let's get rid of it. But why remove src/sage/libs/glpk/env.pxd?

@orlitzky
Copy link
Contributor Author

OK, let's get rid of it. But why remove src/sage/libs/glpk/env.pxd?

IIRC it's unused and I only noticed it because I was removing error.pyx at the time.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 16, 2024

But I think we often carry .pxd files that just provide the full API whether it's used or not.

@orlitzky
Copy link
Contributor Author

I would guess that it'd be better to regenerate it however-many years from now based on the latest GLPK, if/when someone wants it. But I don't really care if you think we should keep it. It was removed for housekeeping and not to solve any problem.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 16, 2024

Yes, let's please keep it. (By the way, I don't expect any API developments in GLPK to happen.)

@orlitzky
Copy link
Contributor Author

Easy enough, done.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 16, 2024

By the way, I hope to downgrade GLPK to "optional" soon and replace it by SCIP (see #37494)

mkoeppe
mkoeppe approved these changes Apr 16, 2024
View reviewed changes
Copy link
Member

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@orlitzky
Copy link
Contributor Author

Thanks.

By the way, I hope to downgrade GLPK to "optional" soon and replace it by SCIP (see #37494)

GLPK isn't well-maintained these days, and is not exactly at the forefront of mathematical programming, so I think that would be an improvement.

@vbraun
Copy link
Member

vbraun commented Jun 3, 2024

merge conflict

@orlitzky
sage/libs/glpk: remove glpk.error module
fd52348 
There are no longer any "expected crashes" in the GLPK backend, and
nothing else seems to be using the glpk.error module. This commit
removes it.
@orlitzky
build/pkgs/glpk: drop GLPK patch that was rejected by upstream.
ef282be 
SageMath has been carrying a patch to GLPK's error handling that was
rejected as unsupported by upstream. The GLPK backend has been updated
to obviate this patch (unless the user ignores a warning and changes
the default solver...), so we can finally drop it and bring SageMath's
GLPK package to parity with the various distributions.
@orlitzky
build/pkgs/glpk: bump patch level to p1
5a3698b
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
sagemathgh-37801: Drop custom GLPK patch and error handling
5115298 
    
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
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
sagemathgh-37801: Drop custom GLPK patch and error handling
eb26998 
    
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
@vbraun vbraun merged commit 8f6b1e5 into sagemath:develop Jun 22, 2024
28 of 36 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jul 8, 2024
@orlitzky orlitzky deleted the dont-patch-glpk branch July 14, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkoeppe mkoeppe mkoeppe approved these changes

Assignees
No one assigned
Labels
c: packages: standard v: moderate
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.

Remove GLPK error recovery patch
3 participants
@orlitzky @mkoeppe @vbraun