Access database and other files through features, for simpler configuration #37024

tornaria · 2024-01-07T14:05:58Z

This PR reworks the way sagemath access database files, and some other external files. The end result is that the runtime detection of databases as features matches exactly what will be used. Moreover, this leads to an easy implementation of search paths, meaning we don't need configuration as long as the files are in a few standard system locations.

Notes on implementation:

First every user of these files access them via features, and never directly through sage.env, since the former is dynamic and the latter is static.
Then we add a variable SAGE_DATA_PATH which is a colon separated search path for databases. The default should work for sage-the-distro and most distros (think /usr/share/sagemath:/usr/share for a system install on /usr).
Now a database, say cremona is searched on $p/cremona for each $p in SAGE_DATA_PATH.
I also added $DOT_SAGE/db first in the search path, this makes it easy for a user to install a missing database (or update an old one).
Finally, as an example I did something similar to find JmolData.jar and three.min.js. The former is searched in $SAGE_SHARE/sagemath/jmol and $SAGE_SHARE/jmol and the latter also in $SAGE_SHARE/jupyter/nbextensions/threejs-sage which seems to make sense.

Other files can be done later (e.g. mathjax). In principle anything that has a default value in sage.env or sage_conf should benefit from using a search path and removing the default value.

With this PR, I can run stock sagemath without any sage_conf.py.

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have created tests covering the changes.

⌛ Dependencies

tornaria · 2024-01-07T16:36:43Z

What's with this:

UserWarning: Feature database_cremona_mini_ellcurve is declared optional, but it is provided by elliptic_curves, which is declared standard in SAGE_ROOT/build/pkgs

I can't reproduce.

src/sage/graphs/strongly_regular_db.pyx

mkoeppe · 2024-01-07T16:45:37Z

Very nice. I'll take a closer look later.

src/sage/features/databases.py

src/sage/features/jmol.py

src/sage/features/threejs.py

src/sage/features/__init__.py

tornaria · 2024-01-08T02:37:18Z

src/sage/features/__init__.py

            Feature('null')
+            sage: sh = StaticFile(name="shell", filename="sh",
+            ....:                 search_path="/dev:/bin:/usr")


There's one test failure here because I forgot to change this test after I removed the split at ":" feature for search_path.

I'll fix it after current CI finishes with this:

Suggested change

....: search_path="/dev:/bin:/usr")

....: search_path=("/dev", "/bin", "/usr"))

tornaria · 2024-01-09T15:10:36Z

I think this is ready to go. Only failure is in codecov/patch, but for some of the new lines I have no way to test them in CI (need optional packages, etc).

tornaria · 2024-01-11T01:59:06Z

I moved _required_threejs_version to a method of the threejs feature, since sage.repl.rich_output.display_manager cannot be imported at install time.

mkoeppe · 2024-01-11T04:47:44Z

Overall, looks good to me.

The solution for replacing the COMBINATORIAL_DESIGN_DATA_DIR seems fine to me. I don't have a preference between this approach and @orlitzky's approach in #37025 using a new repo. Merging the present branch does not conflict with implementing @orlitzky's approach if so decided later.

I'll hold off with setting to positive review for a moment so that other downstream packagers can comment whether this meets their needs. Feel free to set to positive review if there's no reaction.

src/sage/features/threejs.py

orlitzky · 2024-01-11T16:35:51Z

I strongly believe that separate packages with static sagelib dependencies and discoverability via python is a better design, but this doesn't make anything worse at the moment.

orlitzky · 2024-01-11T16:40:45Z

Can you please include my improvements from https://github.com/orlitzky/mols-handbook-data/commits/master/ since this ticket will be used as an excuse not to merge them?

tornaria · 2024-01-11T17:33:17Z

Can you please include my improvements from https://github.com/orlitzky/mols-handbook-data/commits/master/ since this ticket will be used as an excuse not to merge them?

Sure, should I just place your __init__.py file as src/sage/combinat/designs/mols_handbook_data.py and use it through mols_handbook_data.lower_bound() ?

This way we keep your documentation and testing.

orlitzky · 2024-01-11T17:43:30Z

Sure, should I just place your __init__.py file as src/sage/combinat/designs/mols_handbook_data.py and use it through mols_handbook_data.lower_bound() ?

Yeah that's fine with me. A few doctests might need to be tweaked to use sage: rather than >>> , and the docstring for lower_bound() is NumPy-style rather than Sage-style. Otherwise it's just a file full of numbers.

tornaria · 2024-01-11T18:27:03Z

Sure, should I just place your __init__.py file as src/sage/combinat/designs/mols_handbook_data.py and use it through mols_handbook_data.lower_bound() ?

Yeah that's fine with me. A few doctests might need to be tweaked to use sage: rather than >>> , and the docstring for lower_bound() is NumPy-style rather than Sage-style. Otherwise it's just a file full of numbers.

Done. Feel free to make a PR against my branch if you think anything else needs changing, or just make a follow-up PR on top of this.

tornaria · 2024-01-11T18:44:11Z

I strongly believe that separate packages with static sagelib dependencies and discoverability via python is a better design, but this doesn't make anything worse at the moment.

I think using separate packages is fine if there's an active upstream that cares about the package. What you are suggesting is that sagemath becomes the upstream for a package that (in principle) only sagemath will use and for which there is no technical advantage for splitting.

Being upstream for a few packages is not working well for sagemath, and it doesn't scale. You make the package to start with, but are you willing to maintain it? Do you even have the bandwidth to maintain all of this? For instance, you created conway-polynomials and got it used in sagemath, but there are two open PRs (from before this was merged into sagemath) that didn't even get a "not interested" comment.

orlitzky · 2024-01-11T19:26:40Z

I think using separate packages is fine if there's an active upstream that cares about the package. What you are suggesting is that sagemath becomes the upstream for a package that (in principle) only sagemath will use and for which there is no technical advantage for splitting.

SageMath is also upstream for it when you include it with sagelib. The technical advantage for the combinatorial designs is minor (for example, saving space in most peoples' git checkouts) but nonzero. For the other database packages, the advantages are greater: not having to define SAGE_SHARE or guess at paths, not having coupling in both directions between sage and its database packages.

Being upstream for a few packages is not working well for sagemath, and it doesn't scale. You make the package to start with, but are you willing to maintain it? Do you even have the bandwidth to maintain all of this? For instance, you created conway-polynomials and got it used in sagemath, but there are two open PRs (from before this was merged into sagemath) that didn't even get a "not interested" comment.

I moved the repo under the sagemath organization so that I'm not a single point of failure, but if there's a group of people who get sagemath notifications by default, I'm not in it. You haven't gotten a response not because I'm ignoring them, but because I didn't know they were there.

tornaria · 2024-01-11T20:12:12Z

SageMath is also upstream for it when you include it with sagelib.

Yes but it's just a file out of 10k.

The technical advantage for the combinatorial designs is minor (for example, saving space in most peoples' git checkouts) but nonzero.

Not significant, moreover all the files build/pkgs/combinatorial_designs takes more blocks than the actual file (!). But on the other side of the coin, you add another gh repo, another upstream, another pypi project, another package for each distribution, etc.

For the other database packages, the advantages are greater: not having to define SAGE_SHARE or guess at paths, not having coupling in both directions between sage and its database packages.

Sure: having a proper "python" package instead of an ad-hoc "spkg" package seems nicer, but it still has a cost, and an "ad-hoc" python package may not be enough benefit to offset the cost. At least updating a file in the main repo is something that we know how to do, but managing several packages and pypi is something we do badly.

Examples: cypari2 (I think) needed to be updated but noone could do it in pypi; threejs-sage is a mess and there is an upstream but sage is too special to use that so fork it and then have a mishmash of versions between spkg / gh / pypi / upstream, moreover having a separate package (forked, under full sagemath control) is useless since the filepaths have an exact version so the package can't really be updated independent of sagemath.

Since I'm ranting: we have sage_setup split off (why?) but we require exactly the same version, what is that good for? I have sage_setup 10.2 in my system and I can't use it to build 10.3.beta4, wtf, etc, etc.

I moved the repo under the sagemath organization so that I'm not a single point of failure, but if there's a group of people who get sagemath notifications by default, I'm not in it. You haven't gotten a response not because I'm ignoring them, but because I didn't know they were there.

I'm not blaming you, and the issue is not new nor specific to this package. I have other PRs in the same status (e.g. sagemath/cysignals#192). Another one is sagemath/cypari2#138, and before you say this is merged, look again because here it's not: https://pypi.org/project/cypari2/.

…tion

…onfiguration

…uration

Search path at sage.env.SAGE_DATA_PATH with a good default. Function `sage_data_path(data_name)` creates search path for a database named `data_name`.

This way databases can be found in several locations, reducing the need for sage_conf.

tornaria · 2024-01-15T18:35:33Z

Rebased without changes on top of 10.3.beta5.

github-actions · 2024-01-15T19:10:03Z

Documentation preview for this PR (built with commit 2d3b596; changes) is ready! 🎉

…or simpler configuration This PR reworks the way sagemath access database files, and some other external files. The end result is that the runtime detection of databases as features matches exactly what will be used. Moreover, this leads to an easy implementation of search paths, meaning we don't need configuration as long as the files are in a few standard system locations. Notes on implementation: - First every user of these files access them via features, and never directly through `sage.env`, since the former is dynamic and the latter is static. - Then we add a variable `SAGE_DATA_PATH` which is a colon separated search path for databases. The default should work for sage-the-distro and most distros (think `/usr/share/sagemath:/usr/share` for a system install on `/usr`). - Now a database, say `cremona` is searched on `$p/cremona` for each `$p` in `SAGE_DATA_PATH`. - I also added `$DOT_SAGE/db` first in the search path, this makes it easy for a user to install a missing database (or update an old one). - Finally, as an example I did something similar to find `JmolData.jar` and `three.min.js`. The former is searched in `$SAGE_SHARE/sagemath/jmol` and `$SAGE_SHARE/jmol` and the latter also in `$SAGE_SHARE/jupyter/nbextensions/threejs-sage` which seems to make sense. Other files can be done later (e.g. mathjax). In principle anything that has a default value in `sage.env` or `sage_conf` should benefit from using a search path and removing the default value. With this PR, I can run stock sagemath without any `sage_conf.py`. ### 📝 Checklist - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have created tests covering the changes. ### ⌛ Dependencies URL: sagemath#37024 Reported by: Gonzalo Tornaría Reviewer(s): François Bissey, Gonzalo Tornaría, Matthias Köppe, Michael Orlitzky

As noted in sagemath#37024 and in sagemath#36489, `sage-conf` is actually not needed on a few systems now. For this reason, we make the installation of it optional (as there are no optional install requires as far as I know, this means we remove it from `pyproject.toml` and `setup.cfg`). We also remove it from "install all dependencies via conda" as its not needed there.

…ion after sagemath#37024   `sage.features` is shipped by **sagemath-environment**, but the version file that the Feature introduced in sagemath#37024 uses is shipped by **sagemath-repl**. This breaks doctesting in a modularized environment. As seen in ipython/ipython#14317 (https://github.com/ipytho n/ipython/actions/runs/7731330166/job/21078723213?pr=14317#step:9:136)    ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#37178 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe

As noted in sagemath#37024 and in sagemath#36489, `sage-conf` is actually not needed on a few systems now. For this reason, we make the installation of it optional (as there are no optional install requires as far as I know, this means we remove it from `pyproject.toml` and `setup.cfg`). We also remove it from "install all dependencies via conda" as its not needed there.