Replace is_package_installed with Features

[fbissey]

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()

[git]

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

​97d47af

20382: missing docstring

[git]

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

​8b6fcc9

20382: desirable error message + typo

[fbissey]

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]

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

[fbissey]

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.

[fbissey]

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

[jhpalmieri]

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

[nthiery]

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 #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

[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

[saraedum]

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]

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.

[vdelecroix]

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.

[vdelecroix]

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()

[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

[saraedum]

Replying to vdelecroix:

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

Sure. A followup ticket could define gap.require_package as a call out to GapPackage().require.

[saraedum]

Replying to vdelecroix:

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...

The docstring tells you that you might want to cache the result if you think it is a performance problem. Most of the time you are going to call out to a binary anyway so I do not see the point of caching it as a default. "Premature optimization is the root of all evil…" We could centralize the checks. I do not really have an opinion on that.

• 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).

That's what is_present vs require is about. There are places where the default algorithm is chosen depending on the presence of some feature.

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).

So you have to catch whatever RuntimeError? instead of being able to catch something specific? I do not see the advantage of that. Also, if I understand your proposal, each caller has to cook up their own error message. I think it is nice to have a central place where these error messages and resolutions live. Finally, with your approach you do not get the granularity between `abc is not on the path vs abc is there but does not work`. That's something that some people requested.

• 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?

See above.

• 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).

GapPackage? lives in sage.misc.feature. So it should be clear that this is not the actual package. We can suffix everything with Feature if that is preferred. I prefer short class names if possible.

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.

See above. (Sorry if I misunderstood you. It seems you are raising the centralization vs localization issue several times in your comment.)

[saraedum]

Replying to nthiery:

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.

Sure. Both options are valid. They are not in the scope of this ticket I think. In either case, such "features" can easily be added to Feature.

[git]

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

​90be504	Centralize feature tests
​da5e893	More checks for Lrs
​a9cbda3	Move generic resolution information from Executable to Feature
​2007e18	Add Feature CremonaDatabase

[slabbe]

If I do ls in sage/misc directory and I see the file feature.py listed, I would never be able to guess its content. Can the filename be more verbose maybe package_feature.py?

[vdelecroix]

Replying to slabbe:

If I do ls in sage/misc directory and I see the file feature.py listed, I would never be able to guess its content. Can the filename be more verbose maybe package_feature.py?

Would not make sense. It is not only about packages but also system softwares and libraries on the system. However, it could contains the words dependencies or requirement. I am not sure what would be the clearest.

[nthiery]

Maybe something like feature_test? This seems to be consistent with autoconf's vocabulary.

[saraedum]

I like feature_test. I will rename it to that.

[vdelecroix]

Hello,

This code is mostly Sage agnostic right? Would it be possible to move this code out of Sage source code? It would better be in sage/src/sage_setup/ or whatever appropriate (even possibly an external python module). We might want to use it outside of Sage or before Sage is built to track possible dependencies.

[fbissey]

As far as I am concerned sage_setup is only used at install time and shouldn't be used at runtime. But spinning it as a separate python package sounds like an interesting idea. Other packages could probably benefits from those mechanism.

[saraedum]

I already thought about moving parts of this out of the source tree. I have not really reached a conclusion yet. It certainly needs to be used during build time. Currently, we have sage_setup/optional_extension.py which calls sage.misc.package.is_package_installed. So, at least in a first version, I would certainly mimic this behaviour and check for a feature (in this case, whether Cython can build against a library) at build time.

Replying to vdelecroix:

This code is mostly Sage agnostic right?

The idea is Sage agnostic. The features it checks for are of course relatively Sage specific.

Would it be possible to move this code out of Sage source code? It would better be in sage/src/sage_setup/ or whatever appropriate (even possibly an external python module). We might want to use it outside of Sage or before Sage is built to track possible dependencies.

I am not sure about moving it to sage_setup. We need this at runtime. Can we (and should we?) access things in sage_setup at runtime? It is an interesting idea to make it a separate package. There seems to be no python package around which does anything similar. I would be happy to pursue such a path but would like to postpone this to a followup ticket.

[git]

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

​ef9cf7d

Merge branch 'develop' into t/20382/replace_is_package_installed_with_features

[git]

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

​095b402	fixed incorrect import statement
​670a3e2	Merge branch 'u/saraedum/replace_is_package_installed_with_features' of git://trac.sagemath.org/sage into t/20382/replace_is_package_installed_with_features
​6f5dc21	Completed implementation of CremonaDatabase feature
​074a318	Implement SmallGroupsLibrary feature

[git]

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

​6d508a1

Merge branch 'develop' of https://github.com/sagemath/sage into t/20382/replace_is_package_installed_with_features

[git]

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

​10c1a47	Asking whether a package is installed is not deprecated
​680c19a	removed unused import
​bb47ef0	Replace is_installed with SmallGroupsLibrary.require
​3acffa7	A note on is_package_installed versus features
​b6c4797	Optional sage modules and shared libraries as features

[saraedum]

This seems to be essentially complete now. Some docstrings are still missing and I also need to do some testing but I replaced all occurrences of is_package_installed if the code is just checking whether an optional package is installed. Any feedback is already very welcome until I find the time to polish all the docstrings.

[fbissey]

That's really cool. I will give it a good look. I may even try it in sage-on-gentoo.

[saraedum]

I was actually a little confused that is_package_installed gets used in so few places. (Or maybe search_src is lying about it.)

[fbissey]

Looks good to me. There is one last thing that I think should be done, but I wouldn't hold the ticket for it, is remove

from .package import (install_package,
        installed_packages, is_package_installed,
        standard_packages, optional_packages, experimental_packages,
        upgrade, package_versions)

from sage/misc/all.py we don't really want those to be directly exposed in my opinion - and they are/were always called explicitly in sage itself.

[fbissey]

My wish above may be best served by different ticket. Can you put this one in "need review".

[vdelecroix]

Checking availability of a Python module as you did has consequences

sage: def is_present(module):
....:    import importlib
....:    try:
....:        importlib.import_module(module)
....:        return True
....:    except ImportError:
....:        return False
sage: print len(sys.modules), 'numpy' in sys.modules
2625 False
sage: is_present('numpy')
True
sage: print len(sys.modules), 'numpy' in sys.modules
2710 True

This would better be documented.

[vdelecroix]

I would be very happy to use this ticket to solve #21135 where I need to set the octave parameters for its Sage interface depending on its version... hence I am asking whether it would be possible to have a version method on Features (or at least on Executable)? For executable it is reasonable to just have an attribute _version_cmd_option (that would generally be -v or --version or --dumpversion) with a default implementation that runs the command with the option _version_cmd_option. What do you think?

[vdelecroix]

Note also that for interfaces we allow remote programs with possibly subtle commands (see #20388 and the file sage/interfaces/expect.py). For example, I can access magma and get it work within sage by doing

$ SAGE_MAGMA_COMMAND="module load magma/2.11.13; magma"
$ SAGE_MAGMA_SERVER="plafrim"
$ export SAGE_MAGMA_COMMAND SAGE_MAGMA_SERVER
$ sage -q
sage: magma('1+1')
2

Does a magma feature should take care of this?

[saraedum]

Replying to vdelecroix:

I would be very happy to use this ticket to solve #21135 where I need to set the octave parameters for its Sage interface depending on its version...

It would be great to see somebody use this interface already :) I have completing this Ticket high on my priority list. I hope I find the time to do this soon.

hence I am asking whether it would be possible to have a version method on Features (or at least on Executable)? For executable it is reasonable to just have an attribute _version_cmd_option (that would generally be -v or --version or --dumpversion) with a default implementation that runs the command with the option _version_cmd_option. What do you think?

Sometimes version checking is a good thing to do but I generally follow the autoconf tradition here that sees version numbers as something very unreliable. I think you should check whether the executable provides the interface that you need.

So, I would be reluctant to provide a general interface for version checks. I should certainly add a comment about this somewhere. Anyway, you can extend any Feature. To check for a version you might want to roughly do something like what I did for StaticFile.absolute_path.

[saraedum]

Replying to vdelecroix:

Note also that for interfaces we allow remote programs with possibly subtle commands (see #20388 and the file sage/interfaces/expect.py). For example, I can access magma and get it work within sage by doing

$ SAGE_MAGMA_COMMAND="module load magma/2.11.13; magma"
$ SAGE_MAGMA_SERVER="plafrim"
$ export SAGE_MAGMA_COMMAND SAGE_MAGMA_SERVER
$ sage -q
sage: magma('1+1')
2

Does a magma feature should take care of this?

Sorry, but I am not sure whether I understand your question. Do you mean whether a Magma Feature should look at SAGE_MAGMA_COMMAND and SAGE_MAGMA_SERVER? Yes, absolutely.

[vdelecroix]

Replying to saraedum:

Replying to vdelecroix:

Note also that for interfaces we allow remote programs with possibly subtle commands (see #20388 and the file sage/interfaces/expect.py). For example, I can access magma and get it work within sage by doing

$ SAGE_MAGMA_COMMAND="module load magma/2.11.13; magma"
$ SAGE_MAGMA_SERVER="plafrim"
$ export SAGE_MAGMA_COMMAND SAGE_MAGMA_SERVER
$ sage -q
sage: magma('1+1')
2

Does a magma feature should take care of this?

Sorry, but I am not sure whether I understand your question. Do you mean whether a Magma Feature should look at SAGE_MAGMA_COMMAND and SAGE_MAGMA_SERVER? Yes, absolutely.

Yes. That was mostly the question ;-) There are some interferences between the branch provided here and how software are detected (and configured) in sage/interfaces/. This should be done in one place and I guess features is the right one.

[vdelecroix]

Replying to saraedum:

Replying to vdelecroix:

[...]

Sometimes version checking is a good thing to do but I generally follow the autoconf tradition here that sees version numbers as something very unreliable. I think you should check whether the executable provides the interface that you need.

Argh. I do not see how I can check that octave does not launch the GUI... I can check whether octave --no-gui is available. If octave does not exist I got a OSError and if it does not accept --no-gui I get a 1 return code. Is that what I should do? Does that should be part of an Octave feature?

[git]

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

​2a67f26

Trivial doctest fixes in sage.misc.feature

[git]

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

​948bb45

Fill in missing doctests for sage.misc.feature

[git]

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

​e824e0d

Fixed doctests in sage.misc.feature

[git]

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

​7cf972c

Merge branch 'develop' of https://github.com/sagemath/sage into t/20382/replace_is_package_installed_with_features

[saraedum]

Replying to fbissey:

[...] Do I understand well that you need an executable class for any external binaries that you want to call?

Its up to you. You can create a new class and make it easier for others to use or you could just write Executable(...).require() to check for an executable in one place.

[saraedum]

Replying to fbissey:

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).

I tried to check for the package that was actually needed whenever the old code queried the gap_packages SPKG. I am not sure I fully understand your comment here. Could you point me to an example in the code where this is a problem?

[saraedum]

Replying to fbissey:

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/databases/cremona.py loads the cremona DB from a very specific place, namely SAGE_SHARE. This is certainly not ideal. But making that code more flexible should imho go into a followup ticket. Should I create one?

• sage/game_theory/normal_form_game.py + sage/geometry/polyhedron/base.py look for the lrs executable

Addressed with the Lrs feature.

• 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.

I am checking this now with a PythonModule feature.

• sage/graphs/graph_generators.py 3 different executables to detect

I am checking for these executables (and run them on trivial inputs.)

• sage/graphs/lovasz_theta.py covered here
• sage/groups/generic.py possibly detect if gap package tomlib is present

I am checking for the presence of the small groups library. Is that the same somehow?

• 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.

I am checking for the hap package here. Is there anything else that should happen?

• sage/misc/all.py my stuff should stay. It is removal of import no one uses.

Sure. But I am not addressing this here.

• sage/rings/polynomial/multi_polynomial_sequence.py finding one executable

Actually you need to check for the presence of the fes library. Not only that but also whether the sage interface to that library has been built (which only happens if sage can find the library, i.e., SPKG at build time.)

[saraedum]

Replying to nthiery:

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.

That's the approach I would like to follow. Currently there is no code like this in here because I am just replacing is_package_installed which also did not do any version checks. But, yes, people should check for a certain feature of a package and then the reason of a test failure should say "Your X does not support Y." and the resolution should say "You need at least version Z of X. Try sage -i X to fix this." or something along these lines.

[git]

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

​7d38138

Renamed sage.misc.feature to sage.misc.feature_test

[saraedum]

Replying to vdelecroix:

Checking availability of a Python module as you did has consequences

sage: def is_present(module):
....:    import importlib
....:    try:
....:        importlib.import_module(module)
....:        return True
....:    except ImportError:
....:        return False
sage: print len(sys.modules), 'numpy' in sys.modules
2625 False
sage: is_present('numpy')
True
sage: print len(sys.modules), 'numpy' in sys.modules
2710 True

This would better be documented.

You are right. I documented that.

[saraedum]

Replying to vdelecroix:

Replying to saraedum:

Replying to vdelecroix:

[...]

Sometimes version checking is a good thing to do but I generally follow the autoconf tradition here that sees version numbers as something very unreliable. I think you should check whether the executable provides the interface that you need.

Argh. I do not see how I can check that octave does not launch the GUI... I can check whether octave --no-gui is available. If octave does not exist I got a OSError and if it does not accept --no-gui I get a 1 return code. Is that what I should do? Does that should be part of an Octave feature?

I have a feeling that I already answered this one. But apparently I have not. I would write an Octave feature that checks whether there is an octave binary and then checks whether it accepts --no-gui. Sort of like StaticFile has an absolute_path() that tells you something about how this feature works, Octave could expose information about which parameters are needed to actually invoke it so it reads from stdin.

[saraedum]

I hope I addressed all comments on this ticket. I did not have a chance to run all doctests yet (my sage 7.4 is still building) but essentially this is ready for review now.

[git]

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

​0529637	Reordered classes in sage.misc.feature_test
​6f2d596	Fixed doctests which failed because of the feature -> feature_test renaming
​edd4f6e	add missing sig_off to doctest

[saraedum]

New commits:

​0529637	Reordered classes in sage.misc.feature_test
​6f2d596	Fixed doctests which failed because of the feature -> feature_test renaming
​edd4f6e	add missing sig_off to doctest

---

New commits:

​0529637	Reordered classes in sage.misc.feature_test
​6f2d596	Fixed doctests which failed because of the feature -> feature_test renaming
​edd4f6e	add missing sig_off to doctest

[git]

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

​2111eb3

Move Bliss feature test to feature_test.py

[saraedum]

Since is_package_installed is very sage (the distribution) specific, it causes trouble for package managers which would like to refer to check whether a package is available on the local system instead of whether it has been installed with sage -i (or similar.) This replaces all occurrences of is_package_installed (which are not part of sage the distribution) with a "feature" check which is performed much in the spirit of autoconf: check for the presence of the features needed, not for version numbers. These checks are certainly not perfect yet and will need to evolve over time.

[git]

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

​c3857a6	Bliss feature is an OptionalModule
​44e9a5f	Fix suffix of cythonized files

[jdemeyer]

Needs to be merged with Sage 7.4.beta2.

[jdemeyer]

For OptionalModule, you should separate the library check from the Python module check. They really are independent things. It just happens that there is essentially a one-to-one correspondence in Sage, but that doesn't have to be the case.

And once you do this, you can use the library check in src/sage_setup/optional_extension.py to decide which optional extensions to build.

[jdemeyer]

The changes to src/sage/misc/cython.py might make sense but do not belong to this ticket.

[fbissey]

I must say to me in sage-on-gentoo the fact that the current OptionalModule only require presence to build the extension is problematic but I don't expect a solution to that particular aspect in the near future. In effect the fact that you only check for presence/availability as a signal for building is introducing automagic dependencies.

I currently have a variable mechanism to signal that the dependency should be present and I want to build. A setup.cfg could be a viable solution. I realise that it would be a burden in the current sage model in the short term.

[jdemeyer]

Replying to fbissey:

I must say to me in sage-on-gentoo the fact that the current OptionalModule only require presence to build the extension is problematic but I don't expect a solution to that particular aspect in the near future.

Ideally, Sage should have something like Gentoo's USE flags for optional dependencies...

[git]

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

​a427d75

Merge remote-tracking branch 'origin/develop' into t/20382/replace_is_package_installed_with_features

[saraedum]

Replying to jdemeyer:

For OptionalModule, you should separate the library check from the Python module check. They really are independent things. It just happens that there is essentially a one-to-one correspondence in Sage, but that doesn't have to be the case.

Ok. I am splitting this up.

And once you do this, you can use the library check in src/sage_setup/optional_extension.py to decide which optional extensions to build.

That would be a followup ticket I believe.

[saraedum]

Replying to jdemeyer:

The changes to src/sage/misc/cython.py might make sense but do not belong to this ticket.

I'd prefer to sneak this change in here. The shared library test code in here actually tests that this does the right thing and I'd prefer not to write some constructed doctest for something like this.

[git]

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

​2086217

Isolate library checks from the OptionalModule implementation code

[fbissey]

Sorry, but it will need rebasing against #21289 which is already merged for the next beta.

[jdemeyer]

Replying to saraedum:

Replying to jdemeyer:

For OptionalModule, you should separate the library check from the Python module check. They really are independent things. It just happens that there is essentially a one-to-one correspondence in Sage, but that doesn't have to be the case.

Ok. I am splitting this up.

Well, I meant really splitting those it up: get rid of OptionalModule altogether and replace it by Module. And remove the dependencies: if the dependencies are not satisfied, the module will not import. There is no need for an additional check.

And once you do this, you can use the library check in src/sage_setup/optional_extension.py to decide which optional extensions to build.

That would be a followup ticket I believe.

Fair enough.

[saraedum]

Replying to jdemeyer:

Replying to saraedum:

Replying to jdemeyer:

For OptionalModule, you should separate the library check from the Python module check. They really are independent things. It just happens that there is essentially a one-to-one correspondence in Sage, but that doesn't have to be the case.

Ok. I am splitting this up.

Well, I meant really splitting those it up: get rid of OptionalModule altogether and replace it by Module. And remove the dependencies: if the dependencies are not satisfied, the module will not import. There is no need for an additional check.

Sure, the check is not necessary. But I would like to tell people what is the problem here. In that particular case, the module also would not import but that's more obscure than telling people that the required library is missing. More generally, an OptionalModule checks whether the module imports and is functional. Running some tests to make sure that the underlying library is working properly is a reasonable thing to do imho.

[jdemeyer]

Replying to saraedum:

But I would like to tell people what is the problem here.

The problem is that they need to install the optional Sage package. That's what you should tell.

In that particular case, the module also would not import but that's more obscure than telling people that the required library is missing.

But the OptionalModule machine checks whether the optional Sage package is installed. Having the library available from the OS would not help.

Running some tests to make sure that the underlying library is working properly is a reasonable thing to do imho.

I don't like to put the same information in different places. That way, you will eventually end up with inconsistencies. The information that the module needs the library should be in exactly one place, namely the build system which decides when the module is compiled.

[fbissey]

Replying to jdemeyer:

Replying to saraedum:

But I would like to tell people what is the problem here.

The problem is that they need to install the optional Sage package. That's what you should tell.

In that particular case, the module also would not import but that's more obscure than telling people that the required library is missing.

But the OptionalModule machine checks whether the optional Sage package is installed. Having the library available from the OS would not help.

Indeed. The current build system assumes that if the package is installed it is because I explicitly want it in sage. It is a long standing convention which from where I sit on the outside is called automagic detection. The reality is that it belongs to configuration space (with availability checking - autoconf style). Sure it is possibly more risky to use an outside package but if it satisfy the availability checking, why not?

After all, at least some of us want to push sage down that way.

Also note that for executables Features only cares about availability in the path, from sage or not (as it should as far as I am concerned). So not caring whether it is from sage or not is in spirit.

I'll admit to have a bias since I want most of sage/misc/package.py to be gone with a top priority on is_package_installed in particular.

[jdemeyer]

Replying to fbissey:

The reality is that it belongs to configuration space (with availability checking - autoconf style). Sure it is possibly more risky to use an outside package but if it satisfy the availability checking, why not?

I'm not disagreeing with that. However, it is besides the point. This ticket should implement the current reality, not some future plan.

[fbissey]

I don't think feature can replace the current reality of what happens in OptionalExtension. It should be strictly future work. I am OK for it to be left as is until we decide it is time to move.

[fbissey]

I guess if building and possibly documentation are the only place left using stuff from package.py it could be moved to sage_setup where things not needed at run-time belongs in my opinion.

[fbissey]

documentation -> doctesting

[jdemeyer]

I think I have not made my point clear in 82

What I wanted to say: you should remove the check for libraries from the OptionalModule features. It is currently (whether you like it or not) the job of OptionalExtension in sage_setup to decide whether or not to build the Python module. I don't see the point of the redundant check in OptionalModule. It needlessly complicates things and it is actually wrong (OptionalModule requires a Sage package, not a system package).

[fbissey]

Rebasing on current develop. Someone else should check that I did everything right. One worry when building in parallel building of documentation kept on failing on :module: in package.py. When I built serial it just worked.

---

New commits:

​e23341d

Merge branch 'develop' into features

[saraedum]

Replying to jdemeyer:

I think I have not made my point clear in 82

What I wanted to say: you should remove the check for libraries from the OptionalModule features. It is currently (whether you like it or not) the job of OptionalExtension in sage_setup to decide whether or not to build the Python module. I don't see the point of the redundant check in OptionalModule. It needlessly complicates things and it is actually wrong (OptionalModule requires a Sage package, not a system package).

I don't understand. I meant to write this so it works for a sage that has not been installed through sage-the-distribution. So, for the module to work, I have to check that it has been compiled and that the library it uses actually works. For sage-the-distribution the second part is redundant for other distributions it may not be.

[jdemeyer]

Replying to saraedum:

So, for the module to work, I have to check that it has been compiled and that the library it uses actually works.

No, you don't need to check anything. The module is linked against the library. If the library didn't work, the module probably wouldn't even compile and it certainly wouldn't be importable. If you can import the module, then surely the library works.

[embray]

Something I don't quite get about this: It seems to be purely for runtime feature checks, which is fine and a good thing to have. But is there also any intention or thought put into making this usable for build-time feature checks when building Sage? Is there any way this could be used to check the system for features during ./configure (i.e. as a replacement for a m4 scripts to do the same)?

[saraedum]

When we originally discussed this during Sage days the idea was to eventually use it for both, build and compile time. For that it would have to be moved out of the sage library probably. So yes, we thought about this as a possible second step.

[embray]

Thanks for clarifying that. Yes, for that reason alone it would make sense to have as a stand-alone package of sorts.

Is there anything I can do to help move this forward? I think getting this to a state where it can also detect already installed dependencies will help massively speed up development time in some cases.

[fbissey]

I am quite happy to have it for run-time only in the first instance. One issue for building is that we are not quite ready to use non sage installed optional package at this stage (i.e. finding optional package not installed by sage).

Fixing runtime would remove a burden from sage-on-distro, especially the bits depending on is_package_installed that we don't want to use in a distro. Build time can be fixed if the distro decides to support a particular option. But ideally we would want something more fine grained. Ideally it would tie up with a proper configuration phase.

[embray]

Replying to fbissey:

I am quite happy to have it for run-time only in the first instance. One issue for building is that we are not quite ready to use non sage installed optional package at this stage (i.e. finding optional package not installed by sage).

I'm not sure what you mean exactly by this. Why should optional packages be a problem as long as this can be useful for non-optional packages?

Fixing runtime would remove a burden from sage-on-distro, especially the bits depending on is_package_installed that we don't want to use in a distro. Build time can be fixed if the distro decides to support a particular option. But ideally we would want something more fine grained. Ideally it would tie up with a proper configuration phase.

Yes, that work is going on in #21566 and other associated tickets.

To be clear, I think we're in agreement that this ticket should not handle any tasks directly related to enabling build-time detection of dependencies. I agree that's a separate issue. My point is that we should keep that use-case in mind for this ticket insofar as the checks its provides don't depend on having a fully working Sage install already, and that it exists outside the main Sage package.

In other words, this ticket could remain focused on fixing the is_package_installed issue, but just keep in mind the other use case that this code could help resolve.

[saraedum]

After some discussion with jdemeyer, I'll remove the current checks OptionalModule?. I'll leave the possibility for such checks in though.

[saraedum]

New commits:

​a3947c4	Merge branch 'develop' into t/20382/features
​c809deb	Remove runtime dependencies on shared libraries

[jdemeyer]

Shouldn't all feature checks be cached?

[jdemeyer]

Copy-paste typo:

class Bliss(OptionalModule):
    r"""
    A :class:`Feature` which describes whether the ;module:`sage.graphs.bliss`
    module has been enabled for this build of Sage and is functional.

    .. NOTE::

------> This module does not depend on :class:`LibFESLibrary`. If the module
        imports we just assume that the library is working.

[jdemeyer]

I don't like _test_code: you define the attribute _test_code, pass it through __init__ and then assign it as the test_code attribute. Why so convoluted, you might as well assign test_code directly.

[jdemeyer]

Speaking of test code, wouldn't it be cleaner to move the test code snippets to separate files? Right now, I find it a bit distracting to have the Cython test code in between the Python code implementing the testing logic.

[jdemeyer]

Typo: ;module:`sage.libs.fes`

[jdemeyer]

Replying to saraedum:

After some discussion with jdemeyer, I'll remove the current checks OptionalModule?.

Given that OptionalModule and Module essentially do the same thing, can you just get rid of the OptionalModule class completely?

[jdemeyer]

I don't really like the names LibFES and Bliss. I would prefer something which clearly reminds that this is just about the module: I would call them FESModule and BlissModule.

[jdemeyer]

Another suggestion: split this one big file src/sage/misc/feature_test.py up. It would be good to separate the abstract base classes (like Executable, SharedLibrary,...) from the actual concrete tests.

Personally, I would even have one file for each package, for example src/sage/misc/feature_test/bliss.py for BlissLibrary and BlissModule. In this case, you can define the Cython test code as module global, which would solve 109 too.

[saraedum]

Except for the typos, we do not fully agree on whether the above should be changed or not. jdemeyer agreed to split up the module, introduce caching, and do the renamings himself.

[embray]

This is a minor nit, but is there a reason this isn't using super().__init__ for calling the base-class __init__s?

[saraedum]

What should we do about this issue? I had put a lot of work into this and I would like to get this merged.

Jeroen: Would you agree to merge this as is now (after fixing patchbot errors)? Or can I expect a patch anytime soon?

[embray]

I've in the meantime started moving in a different direction for build-time feature dependency checks, so to me at least my earlier questions about using this for that purpose are moot. But I'm still +1 on having it for runtime feature checks.

It looks like the patch does not apply so I think you'll have to set this back to needs_work. I'd be happy to help rebasing your patch if need be.

[jdemeyer]

I'll clean this up.

---

New commits:

​3deea82

Add Features to check the environment at runtime

[jdemeyer]

In some cases, bigger changes were made than just using this new Features framework. I'll split those off as separate tickets.

[git]

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

​c244878

Add Features to check the environment at runtime

[jdemeyer]

It seems that SharedLibrary is not checking for libraries at all: it just tries to compile some Cython code. So it would be better to rename it as CythonFeature or whatever better name I can come up with...

[jdemeyer]

Replying to jdemeyer:

Given that OptionalModule and Module essentially do the same thing, can you just get rid of the OptionalModule class completely?

I will remove OptionalModule and also rename Module -> PythonModule.

[jdemeyer]

I just found a problem which is not easy to fix: there is no way as far as I can tell to silence compiler error in distutils. This means that feature checks will show messages like

/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.4/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lxxxxxxxx
collect2: error: ld returned 1 exit status

[embray]

I could take a look at that. I feel like that's a problem I've solved once before, also when using distutils for feature checks (even if it means temporarily redirecting stdio or something).

[git]

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

​00cb88e	Add Features to check the environment at runtime
​91558bd	Cleanup feature tests

[git]

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

​5d85c54	Add Features to check the environment at runtime
​92f9e79	Cleanup feature tests

[git]

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

​35f6601

Cleanup feature tests

[jdemeyer]

I'm setting this to needs_review mainly for patchbot testing. It's probably going to break somewhere, but it should mostly work.

[git]

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

​5e3ede5

Cleanup feature tests

[git]

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

​56a0531

Cleanup feature tests

[git]

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

​69ad200

Cleanup feature tests

[jdemeyer]

I now consider this finished, apart from #24720.

[git]

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

​af22cc4

Cleanup feature tests

[git]

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

​3696d19

Cleanup feature tests

[git]

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

​32a8eab

Cleanup feature tests

[fbissey]

It looks really good. There are a few files remaining to migrate to "features" not covered here but that's good for a first iteration and will prove that the concept work. One more file should be migrated in this ticket I think, databases/cremona.py because it is the only file that would use features/databases.py as it currently stands.

For info the following files have stuff to convert

• coding/linear_code.py
• coding/guava.py
• coding/databases.py
• graphs/generators/smallgraphs.py
• groups/abelian_gps/abelian_group.py

All of these are about some gap packages.

[jdemeyer]

Replying to fbissey:

One more file should be migrated in this ticket I think, databases/cremona.py because it is the only file that would use features/databases.py as it currently stands.

I split that off as #24718.

[jdemeyer]

If you care about this ticket, please review #24722, #24724, #24720 which all deal with improving sage.misc.cython to allow for silent operation.

[fbissey]

OK I think it is time to move this ticket further.

[jdemeyer]

I never set this to needs_review.

[jdemeyer]

There is one issue remaining: this shouldn't use cython_import() but a direct cython() call. For simplicity, I plan to do that once all dependencies are merged.

[fbissey]

Ok, at ​https://trac.sagemath.org/ticket/20382#comment:138 you said you considered this done. So I assumed too much out of that. Works well as far as I am concerned.

[jdemeyer]

I guess I meant more broadly "apart from issues with Cython".

[fbissey]

It looks like the dependencies that you listed will be in the next beta - which could turn out to be a rc at this stage. Are there any more tickets that need to go in prior to this one in your opinion?

[jdemeyer]

#24764 would be nice because it touches the same code.

[slelievre]

#24764 is in.

[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

[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.

[vdelecroix]

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)?