Opened 19 months ago
Last modified 4 days ago
#32242 needs_review enhancement
Use GNU Info to parse singular's Info file
Reported by: | mjo | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | |
Component: | packages: standard | Keywords: | |
Cc: | Merged in: | ||
Authors: | Michael Orlitzky | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/mjo/ticket/32242 (Commits, GitHub, GitLab) | Commit: | 137a9182a02d4aa329a78c2e936b48431c1e9017 |
Dependencies: | #32302 | Stopgaps: |
Description (last modified by )
Our Singular interface currently contains a hand-written parser for Singular's "info" file. In this ticket, we add an optional GNU Info package to parse this file instead.
Relative to running "info", the hand-written parser has several drawbacks:
- The extra code is a maintenance burden. We should not be wasting our time (poorly) reimplementing standard tools.
- The custom parser is buggy. For example, it is supposed to raise a
KeyError
when documentation for a non-existent function is requested. However, the parser does not keep track of what section it's in, so, for example,get_docstring("Preface")
will happily return the contents of the preface even though "Preface" is not a Singular function.
- The first time documentation is requested, the entire info file (see above) is loaded into a dictionary. With the Singular info file currently weighing in at around 11MiB, this is wasting a good chunk of memory.
- The
singular_console()
itself tries to launch GNU Info to display its interactive help, and cannot use our hand-written parser.
The one downside to requiring "info" is that it would add a new runtime dependency to sagelib. To avoid that, we make GNU Info optional, and suggest to the user that he install it when he tries to read Singular's documentation.
GNU Info (or its superset, Texinfo) are widespread, portable, and easy to install on all of the systems we support, so in most cases this should be a "free" improvement.
Change History (35)
comment:1 Changed 19 months ago by
Authors: | → Michael Orlitzky |
---|---|
Branch: | → u/mjo/ticket/32242 |
Commit: | → fc7fd02b833b2f76755f1e50a28dd7dd566962e5 |
Status: | new → needs_review |
comment:2 Changed 19 months ago by
Commit: | fc7fd02b833b2f76755f1e50a28dd7dd566962e5 → c4f693803fdc7deb904bb85c7352f6327f338a66 |
---|
comment:3 Changed 19 months ago by
I don't know... shouldn't there be a feature request / PR to Singular to make the docstrings available via API?
comment:4 Changed 19 months ago by
Singular renders its own interactive help using "info" or one of its alternatives: https://www.singular.uni-kl.de/Manual/latest/sing_16.htm
I don't think they can give us an API without making "info" a dependency of Singular itself.
comment:5 follow-up: 7 Changed 19 months ago by
OK I see.
I still don't get why it's necessary or helpful to replace our parser with calling the info
program.
Wouldn't it be much easier to just obtain the help file location from Singular itself?
comment:6 follow-up: 8 Changed 19 months ago by
I would also think it's pretty problematic from the viewpoint of modularization. We may soon have a pip-installable package for the Sage functionality that uses Singular. The wheel could then vendor libsingular
. But vendoring the info
program? That would be rather strange.
comment:7 follow-up: 9 Changed 19 months ago by
Replying to mkoeppe:
OK I see.
I still don't get why it's necessary or helpful to replace our parser with calling the
info
program.
The interface gets a lot smaller and simpler.
Wouldn't it be much easier to just obtain the help file location from Singular itself?
Singular does know where it lives, but how do you propose we get it? Ask them to add a special command-line option to support our outlandish use case?
comment:8 Changed 19 months ago by
Replying to mkoeppe:
I would also think it's pretty problematic from the viewpoint of modularization. We may soon have a pip-installable package for the Sage functionality that uses Singular. The wheel could then vendor
libsingular
. But vendoring theinfo
program? That would be rather strange.
Leave it as part of the main Sage package, assumed to be there by the Singular wheel? In almost all cases, the system Info will be used anyway. But I don't really know enough about what you're trying to do to make a good suggestion.
comment:9 follow-up: 10 Changed 19 months ago by
Replying to mjo:
Singular does know where it lives, but how do you propose we get it?
Use feGetResource
in libsingular_resources?
comment:10 Changed 19 months ago by
Replying to mkoeppe:
Replying to mjo:
Singular does know where it lives, but how do you propose we get it?
Use
feGetResource
in libsingular_resources?
Looks like it will work, thanks. I still think it's kinda dumb to poorly reimplement a ubiquitous 300k tool in math software, but it's good to know that we can find the help file if we really need to.
comment:11 Changed 19 months ago by
info is a well-documented, stable, extremely simple file format, I don't think there's anything wrong with doing basic parsing on it in Python.
comment:12 Changed 19 months ago by
Well, we don't really parse it. It's more like a grep:
sage: from sage.interfaces.singular import get_docstring sage: get_docstring("Preface")
will dutifully pull up the "Preface" node without regard for where in the manual it lives (it's not a function that we wrap and should throw a KeyError
instead). The current branch oversimplifies a bit here too, checking only that the parent node is "Functions", but you can pass a series of (sub)nodes to info
to ensure that you're navigating to where you think you are.
It looks like we also install info files for maxima and giac. So not only would having "info" installed allow you to run the interactive help in a singular_console()
, but it may help somebody read the maxima and giac docs, too.
comment:13 Changed 19 months ago by
I don't really have an objection to adding info
as a package. Accessing the interactive help in the standalone giac, maxima, singular shipped with Sage is a nice-to-have feature; but I don't think this is a strong argument to making it a "standard" package.
However, I don't support replacing the in-Python code (no issues have been reported with it -- no matter how primitive the code may be) by calling out to the info
program. This adds a complication that I would really hope to avoid.
comment:14 follow-ups: 15 16 Changed 19 months ago by
Conversely, I'm only willing to tolerate the new dependency if it eliminates these hacks. It may be true that no issues have been reported with the info parser, but how long have we been waiting for someone to write an spkg-configure.m4 for singular? The only reason no one has done it (singular has a pc file, and is a huge, slow-to-build package) is because everyone is afraid to step into a pile of shit when they open singular.py{,x} =)
Replacing the cruft is its own reward if it makes maintaining the singular interface easier. I'm unhappy about the dependency myself, but who does it really hurt? Doing something about SINGULARPATH
is necessary before we can use Singular from the system. Using GNU Info to read the info file is an easy and principled way to get us to a point where everyone on Fedora, Debian, Gentoo, Homebrew, Conda, Nix, etc. can just use Singular (and Info) from their package manager. That's a big gain for what constitutes almost the entirety of our users. And who is penalized by having Info be a standard package? Maybe some BSD installs that aren't currently supported platforms? This should be something like bzip2 that no one ever builds.
comment:15 Changed 19 months ago by
Replying to mjo:
who is penalized by having Info be a standard package?
There is no problem with making info
a standard package.
But there is a problem in adding an additional non-Python runtime dependency to the Sage library. info
cannot be installed by pip
.
comment:16 Changed 19 months ago by
comment:17 Changed 19 months ago by
One final point for GNU Info:
The Singular manual is now 11MB in singular-4.2.1. The existing implementation loads the entire thing (not just the function documentation, as I've shown) into a global variable that lives forever. This permanently wastes roughly the same amount of memory:
sage: from sage.interfaces.singular import generate_docstring_dictionary sage: get_memory_usage() 6467.30859375 sage: generate_docstring_dictionary() sage: get_memory_usage() 6479.34765625
And that will get worse as the Singular manual grows.
comment:18 follow-up: 19 Changed 19 months ago by
By the way, this ticket does not solve the original problem at all. If we want to use system Singular on a system that does not have info
, then the installed info
from the proposed SPKG does not have information regarding the info file location. So you are only shifting the problem of locating the info file from runtime to build time; *and* you are introducing a new non-Python dependency for the sagelib runtime.
comment:19 Changed 19 months ago by
Replying to mkoeppe:
By the way, this ticket does not solve the original problem at all. If we want to use system Singular on a system that does not have
info
, then the installedinfo
from the proposed SPKG does not have information regarding the info file location. So you are only shifting the problem of locating the info file from runtime to build time; *and* you are introducing a new non-Python dependency for the sagelib runtime.
While it's possible to put the info files anywhere, we can stick a colon on the end of $INFOPATH
to have the SPKG try the default list as fallbacks: https://git.savannah.gnu.org/cgit/texinfo.git/tree/info/filesys.h#n70
If we manage to find a system that both doesn't have GNU Info and yet still goes out of its way to install info files in a crazy location... then I guess at that point we could use DEPCHECK to ensure that the Singular SPKG gets built on that system.
comment:20 follow-up: 22 Changed 19 months ago by
Sorry, overall this ticket looks like a solution in search of a problem, and one that will create new problems as a side effect.
The actual problem has a simple solution, comment:9.
comment:21 Changed 19 months ago by
We could always make GNU Info an optional package, and take the same approach to
sage: singular.<function>?
that we take to
sage: singular_console() > help <function>;
That is, it doesn't work unless you have Info installed. The docstring for the sage object would suggest the optional package if it's not available.
comment:22 Changed 19 months ago by
Replying to mkoeppe:
Sorry, overall this ticket looks like a solution in search of a problem, and one that will create new problems as a side effect.
The actual problem has a simple solution, comment:9.
This is only a solution if you insist on keeping our lame python implementation and all of the problems it comes with. It'll get the job done, but I won't be the one to do it.
comment:23 Changed 19 months ago by
Commit: | c4f693803fdc7deb904bb85c7352f6327f338a66 → a82847c7617969350dbc409c6821d10abc74817f |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
a82847c | Trac #32242: use fallback defaults for $INFOPATH.
|
comment:24 Changed 19 months ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → reduce ticket to adding info as optional package |
comment:25 Changed 18 months ago by
Dependencies: | → #32302 |
---|---|
Milestone: | sage-9.4 → sage-9.5 |
comment:26 Changed 18 months ago by
Commit: | a82847c7617969350dbc409c6821d10abc74817f → 624a0e8d064ba70d40af701ec65b0cd47d9cbad6 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2acc06e | Trac #32302: drop SINGULAR_EXECUTABLE from src/bin/sage-env.
|
fde32de | Trac #32242: new optional package for the standalone GNU Info reader.
|
7d1e61b | Trac #32242: use fallback defaults for $INFOPATH.
|
56186bf | Trac #32242: use GNU Info to obtain Singular docstrings.
|
624a0e8 | Trac #32242: add "optional" tags for tests that require GNU Info.
|
comment:27 Changed 18 months ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
Work issues: | reduce ticket to adding info as optional package |
New commits:
2acc06e | Trac #32302: drop SINGULAR_EXECUTABLE from src/bin/sage-env.
|
fde32de | Trac #32242: new optional package for the standalone GNU Info reader.
|
7d1e61b | Trac #32242: use fallback defaults for $INFOPATH.
|
56186bf | Trac #32242: use GNU Info to obtain Singular docstrings.
|
624a0e8 | Trac #32242: add "optional" tags for tests that require GNU Info.
|
comment:28 Changed 18 months ago by
It's now completely optional. When "info" is not installed, you'll be prodded to install it:
sage: singular.align? Signature: singular.align(*args, **kwds) Type: SingularFunction String form: align File: ~/src/sage.git/local/lib/python3.9/site-packages/sage/interfaces/singular.py Docstring: This function is an automatically generated pexpect wrapper around the Singular function 'align'. EXAMPLES: sage: groebner = singular.groebner sage: P.<x, y> = PolynomialRing(QQ) sage: I = P.ideal(x^2-y, y+x) sage: groebner(singular(I)) x+y, y^2-y The relevant Singular documentation will be shown here if you install GNU Texinfo, the stand-alone GNU Info program, or the SageMath "info" SPKG.
comment:29 Changed 18 months ago by
I have opened #32323 for the uncontroversial part "Add GNU info as an optional package".
comment:30 Changed 14 months ago by
Summary: | Use GNU Info (new package) to parse singular's Info file → Use GNU Info to parse singular's Info file |
---|
comment:31 Changed 14 months ago by
Commit: | 624a0e8d064ba70d40af701ec65b0cd47d9cbad6 → 137a9182a02d4aa329a78c2e936b48431c1e9017 |
---|
comment:32 Changed 13 months ago by
Milestone: | sage-9.5 → sage-9.6 |
---|
comment:33 Changed 9 months ago by
Milestone: | sage-9.6 → sage-9.7 |
---|
comment:34 Changed 5 months ago by
Milestone: | sage-9.7 → sage-9.8 |
---|
comment:35 Changed 4 days ago by
Milestone: | sage-9.8 |
---|
New commits:
Trac #XXXXX: new package for the standalone GNU Info reader.
Trac #XXXXX: drop SINGULAR_EXECUTABLE from src/bin/sage-env.
Trac #XXXXX: use GNU Info to obtain Singular docstrings.