Replace is_package_installed with Features

[saraedum]

CC: @mkoeppe @jdemeyer @nexttime @embray @defeo @isuruf @slel

Component: build

Keywords: days85

Author: Julian Rüth, Jeroen Demeyer

Branch: ad6dcc6

Reviewer: Nicolas M. Thiéry, François Bissey, Julian Rüth

Issue created by migration from https://trac.sagemath.org/ticket/20382

[saraedum]

Branch: u/saraedum/replace_is_package_installed_with_features

[kiwifb]

Commit: 7c1ca31

[kiwifb]

comment:2

Indeed same kind of work. Much more sophisticated on your part I must say. Do I understand well that you need an executable class for any external binaries that you want to call?

It could also solve the issue that I was thinking about executables and other files requirement. You should be able to find and use something already installed in a standard location. theta being already present in /usr/local/bin instead of $SAGE_LOCAL/bin (which resolves to /usr/bin in sage-on-gentoo) or anywhere in the PATH. Similarly you can install gap packages in ~/.gap/pkg and there is no reasons why you shouldn't be able to use those.

---

New commits:

3233147	Add Features to check the environment at runtime
dfd97f8	Check for grape with GapPackage instead of is_package_installed()
7c1ca31	Check for a working CSDP binary instead of is_package_installed()

[nthiery]

Changed branch from u/saraedum/replace_is_package_installed_with_features to u/nthiery/replace_is_package_installed_with_features

[sagetrac-git]

Changed commit from 7c1ca31 to 97d47af

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

97d47af

20382: missing docstring

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

8b6fcc9

20382: desirable error message + typo

[sagetrac-git]

Changed commit from 97d47af to 8b6fcc9

[kiwifb]

comment:6

Handling of database_gap will be delicate. The spkg includes bits removed stuff from gap that are expected by default (no need to load if present) and one "real" gap package that one would need to load (tomlib).

That may require special consideration.

[saraedum]

comment:7

Thanks for the pointer about database_gap. I should have a complete proposal up on the other ticket in the next 24 hours.

[kiwifb]

comment:8

I think this ticket should replace the other one unless you have a plan. I am making a list of stuff that needs to come from the other ticket and stuff that should use your feature detection. I guess we could split it along those lines.

[kiwifb]

comment:9

Going through the files that I have changed in #20377 which this ticket should supersede:

• sage/databases/cremona.py we need to check the real presence of a file in a specific location in this case. Adding something to feature may be overkill for that particular case.
• sage/game_theory/normal_form_game.py + sage/geometry/polyhedron/base.py look for the lrs executable
• sage/graphs/generators/classical_geometries.py already covered in this ticket
• sage/graphs/generic_graph.py I think my stuff should stay. It is mostly straight try...expect whether or not a python module is available.
• sage/graphs/graph_generators.py 3 different executables to detect
• sage/graphs/lovasz_theta.py covered here
• sage/groups/generic.py possibly detect if gap package tomlib is present
• sage/groups/perm_gps/permgroup.py again dodgy database_gap detection in one part. The part using the gap hap package would need a nice rework not just migration. I'd like to pick the mind of the person who wrote this.
• sage/misc/all.py my stuff should stay. It is removal of import no one uses.
• sage/rings/polynomial/multi_polynomial_sequence.py finding one executable

[saraedum]

Changed branch from u/nthiery/replace_is_package_installed_with_features to u/saraedum/replace_is_package_installed_with_features

[jhpalmieri]

comment:11

Is there overlap with #20182? Should #20182 be rewritten to use this?

---

New commits:

1e275a8

Merge branch 'develop' into t/20382/replace_is_package_installed_with_features

[jhpalmieri]

Changed commit from 8b6fcc9 to 1e275a8

[nthiery]

Reviewer: Nicolas M. Thiéry, ...

[nthiery]

Author: Julian Rüth

[nthiery]

comment:12

Hi Julian,

Just some rambling, thinking about use cases for this ticket or some follow up:

While discussing with the Debian people, it was stressed out that we
will eventually want to be able to support simultaneously a stable
version of software X (as provided by distributions), and a devel
version of it (e.g. for our researchers that need the very latest
features). Therefore we probably will want to have code that could
look like:

        def latest_feature():
            """
            ...

            EXAMPLES::

                sage: X = ...
                sage: X.latest_feature()    # optional gap >= 3.5
            """
            GAP(min_version=3.5).require()      # or some better syntax
            ...
            return gap.latest_feature()

To support this use case we would need:

• support for version checking in the Feature code (probably in a follow up ticket).

  Probably not much can be written generically, except possibly for Executables,
  trying to pass the --version and scanning the output for something that looks
  like a version number

• some updates to the doctesting framework to support version
  annotations (definitely in another ticket)

• mapping feature names appearing in the #optional annotations to actual Feature's
  (it's going in this direction in Automatic doctest for external softwares #20182).

Not directly related to this ticket, but building on the discussion we had while walking in Cernay, we may want to be able to write the annotation as::

        def latest_feature():
            """
            ...

            .. REQUIRES:: gap >= 3.5

            EXAMPLES::

                sage: X = ...
                sage: X.latest_feature()
            """

Cheers,
Nicolas

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

340d36e	Improve reporting of Features
0fc00d9	grape is in the gap_packages SPKG
aa12d10	Fixed typos in Features
76aedb4	check for GAP package "hap" through a Feature test
2e451b4	20382: Proofreading
97d47af	20382: missing docstring
8b6fcc9	20382: desirable error message + typo
1bba301	Merge branch 'u/nthiery/replace_is_package_installed_with_features' of git://trac.sagemath.org/sage into t/20382/replace_is_package_installed_with_features

[sagetrac-git]

Changed commit from 1e275a8 to 1bba301

[saraedum]

comment:14

nthiery: About version checks. I think it would be easy to add this in the current setup. However, I would recommend to follow the philosophy of autoconf here. I.e., we should not check for a version because you need to keep track which version range supportes the feature you are looking for (and often things are heavily patched so the version might be misleading.) Instead, we should check for the actual feature/interface required. It is a bit more work of course but usually better and much easier to maintain.

[nthiery]

comment:15

Yep there is value in the autoconf approach. On the other hand, since this is mostly about doctests at this point, a version information is more useful for the user: "oh, I need to upgrade to 3.5" is simpler than "oh, I need to upgrade to a recent enough version that supports method xxx on object yyy". We could do both: make a feature check, and provide a version number for user reporting; but that's more work. Another advantage of version checking is that you just need to fetch the version once, and don't need to relaunch, e.g a GAP command, each time you want to check another feature.

[videlec]

comment:16

I like more the centralized approach in this ticket rather than the one in #20377 in which each feature needs a special care. Though many things looks very complicated and the related #20182 is much more readable (though using inheritance from this ticket would save some efforts). Moreover #20182 implements some database of available features which is not discussed here. The latter has the advantage of minimizing the requests time after the first call. With the architecture proposed here, each check involves a Feature class creation and some associated test...

Some more specific remarks:

• Do we really care about the class FeatureTestResult? If the result is True then we mostly don't care about explanation and if it is False we likely want an error (with an explicit error message). Similarly, I would get rid of FeatureNotPresentError. Instead, I would add an error and a message arguments in the require method (with of course some reasonable default which would mimic the current behavior using the standard RuntimeError).

• Are you sure CSDP is better inside the file lovasz_theta.py? Do we want to spread the Features all around the modules or should they be centralized?

• Finally class names might be confusing. GapPackage is not a handle on a gap package nor something describing what is inside a given package (what I would guess from the name). Similarly, with having CSDP has a class when CSDP is the name of the program. It would be fine if all of them belong to a fixed module.

[videlec]

comment:17

And last but not the least: what about having the "packages" part directly attached to executable?

sage.interfaces.gap.gap.require_package('grape')

instead of

sage.misc.features.GapPackage('grape').require()

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

7b6df60	Buckygen feature
0529a07	ignore stderr output of theta
baf2eed	Benzene feature test
2b2bbf9	Added Feature for plantri
30caced	Added features for bliss and igraph
3a2f8d2	is_package_installed() is deprecated
2a594ee	Feature for lrslib

[sagetrac-git]

Changed commit from 1bba301 to 2a594ee

[jdemeyer]

Changed dependencies from #24720 to none

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8e4c504	Add Features to check the environment at runtime
ad6dcc6	Cleanup feature tests

[sagetrac-git]

Changed commit from 32a8eab to ad6dcc6

[jdemeyer]

comment:155

Setting milestone to sage-pending because I think that this ticket should not be merged in Sage 8.2. The reason is that this ticket has a lot of potential to break stuff because it involves optional packages and those are tested poorly. This would be an ideal ticket to merge in 8.3.beta0.

[saraedum]

Changed reviewer from Nicolas M. Thiéry, François Bissey to Nicolas M. Thiéry, François Bissey, Julian Rüth

[vbraun]

Changed branch from u/jdemeyer/features to ad6dcc6

[videlec]

comment:159

Replying to @jdemeyer:

Setting milestone to sage-pending because I think that this ticket should not be merged in Sage 8.2. The reason is that this ticket has a lot of potential to break stuff because it involves optional packages and those are tested poorly. This would be an ideal ticket to merge in 8.3.beta0.

Indeed... help for fixing the consequences in: #25332

And one question: the LattE feature from #25400 needs to check for two executables. Should I introduce an abstract class feature "Executables" (whose constructor argument would be a list of Executable classes)?

[videlec]

Changed commit from ad6dcc6 to none