Open orlitzky opened 3 years ago
Author: Michael Orlitzky
Branch: u/mjo/ticket/32242
I don't know... shouldn't there be a feature request / PR to Singular to make the docstrings available via API?
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.
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?
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.
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?
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.
Replying to @orlitzky:
Singular does know where it lives, but how do you propose we get it?
Use feGetResource
in libsingular_resources?
Replying to @mkoeppe:
Replying to @orlitzky:
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.
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.
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.
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.
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.
Replying to @orlitzky:
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
.
Replying to @orlitzky:
how long have we been waiting for someone to write an spkg-configure.m4 for singular?
This task was waiting for the stalled Singular upgrade ticket #25993.
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.
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.
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.
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.
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.
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.
Branch pushed to git repo; I updated commit sha1. New commits:
a82847c | Trac #32242: use fallback defaults for $INFOPATH. |
Work Issues: reduce ticket to adding info as optional package
Dependencies: #32302
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. |
Changed work issues from reduce ticket to adding info as optional package to none
Description changed:
---
+++
@@ -1,6 +1,25 @@
-In the description of #29024, a few problems are listed as prerequisites for accepting a system installation of Singular. Basically, they ask that we either use singular in a less crazy manner, or find a way to make our existing hacks work with a system copy of singular.
+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.
-One of those issues is that the `sage/interfaces/singular.py` is using an environment variable `SINGULARPATH` to guess at the location where Singular's Info file is installed, so that it can be manually parsed.
+Relative to running "info", the hand-written parser has several drawbacks:
-In the interest of using Singular in a less crazy manner, I propose we add the standalone GNU "info" as a new package, and use it to read Singular's documentation from wherever it is configured to do so. This eliminates two of the problems in #29024.
+* 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.
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. |
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.
I have opened #32323 for the uncontroversial part "Add GNU info as an optional package".
I think this should be closed as not wanted
Are you still opposed to this now that info
is a standard package?
For the sage distribution, there's no additional runtime dependency because sage doesn't distinguish between build- and run-time dependencies; texinfo will be installed anyway.
On a linux distribution, texinfo will already be a runtime dependency of singular, because singular uses info
for its interactive help. (It should really be a dependency of the singular SPKG if we expect singular_console()
or sage --singular
to work).
I'm asking because I discovered another minor issue:
info
program supports compression, but our hand-rolled parser doesn't.Are you still opposed to this now that
info
is a standard package?
Yes, because https://github.com/sagemath/sage/issues/32242#issuecomment-1418140192 still applies -- it would be a new obstacle to pip-installability
I don't see pip/pypi as different from a linux distribution and its package manager in this scenario. How does singular get installed by pip? Does its interactive help work?
This really only affects interactive uses of sage and not the library itself. (Yes, you can access a docstring programmatically, but realistically...). If singular's interactive help doesn't work, I don't think it's reasonable to expect its help to work interactively through sage, either. Especially not if it brings extra costs along with it, those mentioned in the initial issue, and now the fact that we have to ensure that /usr/share/info/singular.info
is decompressed too.
Singular will be shipped by the wheel of the distribution sagemath-symbolics (in prep at #35095)
I'm making all of Sage pip-installable, including interactive uses (that's provided by sagemath-repl)
Then aren't you going to find yourself in the same situation that linux distributions do? If we try (from our perspectives also as sage developers) to pretend that info
isn't a dependency of singular, then attempting to use singular on its own will have a degraded experience, i.e. no help. That's a problem when you run singular
independently from the CLI, but also when you run sage --singular
or sage: singular_console()
. The latter two should be deprecated IMO, but the bottom line is that if singular is a part of sage, then we have to install a usable singular.
The purpose of shipping Singular in the pip-installable wheel will not be to provide a standalone Singular experience; only to use sage.libs.singular and sage.interfaces.singular
when you run
sage --singular
orsage: singular_console()
. The latter two should be deprecated IMO
I agree with this; in particular the consoles don't work within Jupyter.
The purpose of shipping Singular in the pip-installable wheel will not be to provide a standalone Singular experience; only to use sage.libs.singular and sage.interfaces.singular
Ok, I at least know what we're talking about now, but I still think it's a long shot:
info
installed on his operating systemAll of those have to hold for the absence of the hand-rolled parser to become a problem. On the other hand, replacing it would immediately benefit the rest of us who actually use and develop sage right now.
replacing it would immediately benefit the rest of us who actually use and develop sage right now.
I'd say this is an exaggeration. None of the drawbacks listed in the issue description are user-visible at all.
If info
was intended to be used programmatically, there would be a shared library offering that functionality.
But info
really is just a standalone alternative to the info browser in Emacs (which, of course, reads in the info format using emacs lisp.)
Wasting memory is user-visible. The inability to read compressed info pages is user-visible. The others, OK.
While each issue is minor on its own, there's a growing list of them, and only one relatively weak argument against fixing it.
If info was intended to be used programmatically, there would be a shared library offering that functionality.
I'm using info
in the exact same way that singular itself does. The singular manual is designed with command-line info
in mind. This is the closest thing to an "official" way to parse it that there is.
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.
Depends on #32302
Component: packages: standard
Author: Michael Orlitzky
Branch/Commit: u/mjo/ticket/32242 @
137a918
Issue created by migration from https://trac.sagemath.org/ticket/32242