Replace sage.doctest.external.has_* functions by Features

Question

Replace sage.doctest.external.has_* functions by Features

mkoeppe opened this issue 4 years ago · 71 comments

Some of them already go through features - for those we just rename the feature so that its name attribute is equal to the intended "optional" tag.

For the remaining ones, we add new Feature subclasses in new files src/sage/features/internet.py, mip_backends.py.

Follow-ups:

#32650 latex.py,
#32866 interfaces.py,
#32174, in which we can deprecate the has_... functions

Depends on #30887
Depends on #32614

CC: @kwankyu @jhpalmieri @seblabbe @dimpase @jdemeyer @saraedum @tscrim @dcoudert @sagetrac-tmonteil @videlec

Component: refactoring

Author: Matthias Koeppe, Kwankyu Lee

Branch: 1d02bd0

Reviewer: Kwankyu Lee, Matthias Koeppe, Sébastien Labbé

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

Answer 1 · 2021-10-07T18:32:53.000Z

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
+Some of them already go through features - for those we just rename the feature so that its `name` attribute is equal to the intended optional tag.
 
+For the remaining ones, we add new Feature subclasses in new files `src/sage/features/internet.py`, `latex.py`, `interfaces.py`, `mip_backends.py`.

Answer 2 · 2021-10-07T19:02:34.000Z

Branch: u/mkoeppe/replace_sage_doctest_external_has___functions_by_features

Answer 3 · 2021-10-07T19:02:52.000Z

New commits:

`1c6335a`	`sage.doctest.external.has_internet: Refactor through new Feature`

Answer 4 · 2021-10-07T19:02:52.000Z

Commit: 1c6335a

Answer 5 · 2021-10-07T19:02:52.000Z

Author: Matthias Koeppe, ...

Answer 6 · 2021-10-07T19:32:31.000Z

Dependencies: #30887

Answer 7 · 2021-10-07T19:47:16.000Z

Changed commit from 1c6335a to 0c012f7

Answer 8 · 2021-10-07T19:47:16.000Z

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

`5c23cc9`	`DocTestReporter: Fix 'sage -t --optional=all'`
`1b8634d`	`sage.doctest.external: Add 4ti2`
`d9d4f99`	`Merge tag '9.4.beta6' into t/30887/public/30887`
`646e182`	src/sage/features/four_ti_2.py: Move import of SAGE_ENV inside the `__init__` method, to remove confusion of sage.misc.dev_tools
`f0c61f7`	`Merge #30887`
`b7f7903`	`sage.features.graphviz: Make .name lowercase to match optional tag; refactor through JoinFeature`
`2c95e70`	`sage.features.pandoc: Make .name lowercase to match optional tag`
`214fa89`	`sage.features.ffmpeg: Make .name lowercase to match optional tag`
`eb8f637`	`sage.features.imagemagick: Make .name 'imagemagick' to match optional tag via JoinFeature`
`0c012f7`	`sage.features.rubiks: Make .name lowercase to match optional tag; refactor through JoinFeature`

Answer 9 · 2021-10-07T19:55:38.000Z

Changed commit from 0c012f7 to 0321b5e

Answer 10 · 2021-10-07T19:55:38.000Z

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

`0321b5e`	`src/sage/features/rubiks.py: Fixup`

Answer 11 · 2021-10-07T20:49:46.000Z

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 Some of them already go through features - for those we just rename the feature so that its `name` attribute is equal to the intended optional tag.
 
 For the remaining ones, we add new Feature subclasses in new files `src/sage/features/internet.py`, `latex.py`, `interfaces.py`, `mip_backends.py`.
+
+Follow-up: #32174, in which we can deprecate the `has_...` functions

Answer 12 · 2021-10-07T22:11:49.000Z

Changed commit from 0321b5e to 1bba486

Answer 13 · 2021-10-07T22:11:49.000Z

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

`1bba486`	`src/sage/features: Change remaining features to make .name equal to the optional tag`

Answer 14 · 2021-10-07T22:21:13.000Z

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

`d7bd9c4`	`src/sage/features/gap.py: Set .name of a GapPackage feature as gap_package_PACKAGE`

Answer 15 · 2021-10-07T22:21:13.000Z

Changed commit from 1bba486 to d7bd9c4

Answer 16 · 2021-10-07T22:26:34.000Z

Changed commit from d7bd9c4 to 9ee3243

Answer 17 · 2021-10-07T22:26:34.000Z

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

`9ee3243`	`Feature: Add documentation`

Answer 18 · 2021-10-07T23:13:28.000Z

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

`50248ed`	`sage.doctest.external.has_{cplex,gurobi}: Refactor through new Features`

Answer 19 · 2021-10-07T23:13:28.000Z

Changed commit from 9ee3243 to 50248ed

Answer 20 · 2021-10-07T23:44:09.000Z

Changed author from Matthias Koeppe, ... to Matthias Koeppe

Answer 21 · 2021-10-07T23:44:09.000Z

Description changed:

--- 
+++ 
@@ -1,5 +1,9 @@
-Some of them already go through features - for those we just rename the feature so that its `name` attribute is equal to the intended optional tag.
+Some of them already go through features - for those we just rename the feature so that its `name` attribute is equal to the intended "optional" tag.
 
-For the remaining ones, we add new Feature subclasses in new files `src/sage/features/internet.py`, `latex.py`, `interfaces.py`, `mip_backends.py`.
+For the remaining ones, we add new Feature subclasses in new files `src/sage/features/internet.py`, `mip_backends.py`.
 
-Follow-up: #32174, in which we can deprecate the `has_...` functions
+Follow-ups:
+- #32650 `latex.py`, 
+- `interfaces.py`, 
+- #32174, in which we can deprecate the `has_...` functions
+

Answer 22 · 2021-10-08T01:48:18.000Z

Changed dependencies from #30887 to #30887, #32614

Answer 23 · 2021-10-08T01:48:51.000Z

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

`66cf3e1`	`src/sage/doctest/control.py: Fixup handling of sage_optional_tags`
`1ec0c48`	`src/sage/features/sagemath.py: Add sage.rings.number_field`
`0063749`	`src/sage/features/sagemath.py: Add features for modules that were optional extensions`
`14fd1e5`	`src/doc/en/developer/coding_basics.rst: Update discussion of feature tags`
`27c53ac`	`src/sage/features/sagemath.py: Add 'sage.plot'`
`180e31d`	`Merge #30887`
`10e8d63`	`sage.features.sagemath: Use JoinFeature when tag is different from the actually tested module`
`654d09c`	`sage.features.sagemath: Change sage_optional_tags to sage_features`
`f63a7d0`	`src/sage/features/: Move features depending on optional packages to separate files`
`9358502`	`Merge #32614`

Answer 24 · 2021-10-08T01:48:51.000Z

Changed commit from 50248ed to 9358502

Answer 25 · 2021-10-08T01:55:23.000Z

Changed commit from 9358502 to 6457ee9

Answer 26 · 2021-10-08T01:55:23.000Z

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

`6457ee9`	`src/sage/features/mip_backends.py: Fixup`

Answer 27 · 2021-10-08T06:38:29.000Z

comment:17

Abandoning human-readable string for name of a feature seems to be a regression. How about adding a new attribute like short_name or slug to be used in optional tag, which defaults to the name (perhaps normalized) if not explicitly provided?

Answer 28 · 2021-10-08T16:00:55.000Z

comment:18

A regression, I don't know. If you look at the updated error messages, such as here:

@@ -264,7 +274,7 @@ class FeatureNotPresentError(RuntimeError):
             sage: GapPackage("gapZuHoh8Uu").require()  # indirect doctest
             Traceback (most recent call last):
             ...
-            FeatureNotPresentError: GAP package gapZuHoh8Uu is not available.
+            FeatureNotPresentError: gap_package_gapZuHoh8Uu is not available.
             `TestPackageAvailability("gapZuHoh8Uu")` evaluated to `fail` in GAP.
         """
         lines = ["{feature} is not available.".format(feature=self.feature.name)]

I don't think it is any less human readable than before; in particular because features have full control over how to format the reason for failure.

Answer 29 · 2021-10-09T11:41:52.000Z

comment:20

Replying to @mkoeppe:

I don't think it is any less human readable than before...

I meant replacing space with underline and decapitalization etc. I am not strong on this point if the authors of the features frame are positive.

Answer 30 · 2021-10-09T21:24:04.000Z

comment:21

Cc'ing a few more people who have worked on sage.features

Answer 31 · 2021-10-10T05:32:14.000Z

comment:22

Replying to @kwankyu:

I meant replacing space with underline and decapitalization etc.

Yes - that's what comment:18 referred to

Answer 32 · 2021-10-14T19:49:48.000Z

comment:23

Current branch has merge conflicts with 9.5.beta3.

Answer 33 · 2021-10-14T19:53:30.000Z

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

`4558791`	`Merge tag '9.5.beta3' into t/32614/features_and_optional_tags_for_sage_subset_distributions`
`eeaa722`	`Merge #32614`

Answer 34 · 2021-10-14T19:53:30.000Z

Changed commit from 6457ee9 to eeaa722

Answer 35 · 2021-10-14T19:53:52.000Z

comment:25

merged latest version of #32614

Answer 36 · 2021-10-15T01:35:34.000Z

comment:26

While changing name to fit with "optional tag" (as you did), how about adding (optional) description argument to keep the original plain "name"?

--- a/src/sage/features/__init__.py
+++ b/src/sage/features/__init__.py
class Feature(TrivialUniqueRepresentation):
     A feature of the runtime environment
 
     INPUT:
 
     - ``name`` -- (string) name of the feature; this should be suitable as an optional tag
       for the Sage doctester, i.e., lowercase alphanumeric with underscores (``_``) allowed;
       features that correspond to Python modules/packages may use periods (``.``)
 
     - ``spkg`` -- (string) name of the SPKG providing the feature
 
+    - ``description`` -- (string) optional; plain English description of the feature
+
     - ``url`` -- a URL for the upstream package providing the feature
 
...

-    def __init__(self, name, spkg=None, url=None):
+    def __init__(self, name, spkg=None, url=None, description=None):
         r"""
         TESTS::
 
             sage: from sage.features import Feature
             sage: from sage.features.gap import GapPackage
             sage: isinstance(GapPackage("grape", spkg="gap_packages"), Feature)  # indirect doctest
             True
         """
         self.name = name
         self.spkg = spkg
         self.url = url
+        self.description = description
+
         self._cache_is_present = None
         self._cache_resolution = None
 
...
 
     def __repr__(self):
         r"""
         Return a printable representation of this object.
 
         EXAMPLES::
 
             sage: from sage.features.gap import GapPackage
             sage: GapPackage("grape")  # indirect doctest
-            Feature('gap_package_grape')
+            Feature('GAP package grape')
         """
-        return 'Feature({name!r})'.format(name=self.name)
+        description = self.description if self.description else self.name
+        return 'Feature({description!r})'.format(description=description)

Answer 37 · 2021-10-15T01:44:17.000Z

comment:27

I don't have a strong objection. But observing that (for a feature based on an spkg that is a Python distribution package), we have

the name of the Python package
the name of the spkg
the short description in the SPKG.rst
the long description in the SPKG.rst
the classname of the feature
the description of the feature
the tag of the feature
the docstring of the feature
... I considered it an improvement to cut out one of them....

Answer 38 · 2021-10-15T02:06:11.000Z

comment:28

Replying to @mkoeppe:

I don't have a strong objection. But observing that (for a feature based on an spkg that is a Python distribution package), we have

the name of the Python package

the name of the spkg

the short description in the SPKG.rst

the long description in the SPKG.rst

the classname of the feature

the description of the feature

the tag of the feature

the docstring of the feature
... I considered it an improvement to cut out one of them....

There is no "tag of the feature". From the user's point of view, the others does not help. As description is optional, it doesn't add to the developer's burden.

If you are okay, I can upload a commit to restore the original "name"s using description. Still it is okay with me to drop this commit eventually for some reason...

Answer 39 · 2021-10-15T02:07:26.000Z

comment:29

Fine, go ahead

Answer 40 · 2021-10-15T04:16:36.000Z

Changed branch from u/mkoeppe/replace_sage_doctest_external_has___functions_by_features to public/32649

Answer 41 · 2021-10-15T04:16:36.000Z

Reviewer: Kwankyu Lee

Answer 42 · 2021-10-15T04:16:36.000Z

Changed commit from eeaa722 to none

Answer 43 · 2021-10-15T04:16:56.000Z

Commit: c872d69

Answer 44 · 2021-10-15T04:16:56.000Z

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

`27c53ac`	`src/sage/features/sagemath.py: Add 'sage.plot'`
`180e31d`	`Merge #30887`
`10e8d63`	`sage.features.sagemath: Use JoinFeature when tag is different from the actually tested module`
`654d09c`	`sage.features.sagemath: Change sage_optional_tags to sage_features`
`f63a7d0`	`src/sage/features/: Move features depending on optional packages to separate files`
`9358502`	`Merge #32614`
`6457ee9`	`src/sage/features/mip_backends.py: Fixup`
`4558791`	`Merge tag '9.5.beta3' into t/32614/features_and_optional_tags_for_sage_subset_distributions`
`eeaa722`	`Merge #32614`
`c872d69`	`Add description to features`

Answer 45 · 2021-10-15T04:21:54.000Z

comment:32

Printing of a Feature is not changed: by name, but description is added if present.

Answer 46 · 2021-10-15T04:23:09.000Z

comment:33

I am positive on the branch.

You can set it positive if you are okay with my additions.

Answer 47 · 2021-10-15T04:36:53.000Z

Changed author from Matthias Koeppe to Matthias Koeppe, Kwankyu Lee

Answer 48 · 2021-10-15T04:36:53.000Z

Changed reviewer from Kwankyu Lee to Kwankyu Lee, Matthias Koeppe

Answer 49 · 2021-10-15T04:36:53.000Z

comment:34

Looking great, thanks.

Answer 50 · 2021-10-15T07:45:41.000Z

comment:35

Sorry, I should have said I started tests before going to sleep yesterday. Here is what I see this morning:

sage -t --long --random-seed=0 src/sage/features/join_feature.py
**********************************************************************
File "src/sage/features/join_feature.py", line 59, in sage.features.join_feature.JoinFeature._is_present
Failed example:
    Latte()._is_present()  # optional - latte_int
Expected:
    FeatureTestResult('LattE', True)
Got:
    FeatureTestResult('latte_int', True)
**********************************************************************
File "src/sage/features/join_feature.py", line 75, in sage.features.join_feature.JoinFeature.is_functional
Failed example:
    Latte().is_functional()  # optional - latte_int
Expected:
    FeatureTestResult('LattE', True)
Got:
    FeatureTestResult('latte_int', True)
**********************************************************************
2 items had failures:
   1 of   3 in sage.features.join_feature.JoinFeature._is_present
   1 of   3 in sage.features.join_feature.JoinFeature.is_functional
    0 tests not run because we ran out of time
    [12 tests, 2 failures, 0.01 s]

Everything else is fine (except the last commit about description which was not tested by me).

Answer 51 · 2021-10-15T09:02:58.000Z

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

`08248a1`	`Fix for doctest failures`

Answer 52 · 2021-10-15T09:02:58.000Z

Changed commit from c872d69 to 08248a1

Answer 53 · 2021-10-15T09:04:41.000Z

comment:37

Replying to @seblabbe:

Everything else is fine (except the last commit about description which was not tested by me).

Thanks for catching the failures.

Answer 54 · 2021-10-15T10:15:18.000Z

comment:38

Looks good now, that is, I get All tests passed with sage -btp --optional=sage,optional,external,build src/sage/features/ now.

Answer 55 · 2021-10-15T10:15:18.000Z

Changed reviewer from Kwankyu Lee, Matthias Koeppe to Kwankyu Lee, Matthias Koeppe, Sébastien Labbé

Answer 56 · 2021-10-15T16:18:26.000Z

comment:39

Thanks for reviewing!

Answer 57 · 2021-10-15T18:16:29.000Z

comment:40

Oups, is it too late to put it back to "needs work"? Because patchbot pyflakes says:

src/sage/doctest/external.py:38:1 'urllib.error' imported but unused
src/sage/doctest/external.py:39:1 'urllib.request.Request' imported but unused
src/sage/doctest/external.py:39:1 'urllib.request.urlopen' imported but unused
src/sage/doctest/external.py:40:1 'ssl.SSLContext' imported but unused

src/sage/features/graphviz.py:14:1 '.Feature' imported but unused
src/sage/features/graphviz.py:14:1 '.FeatureTestResult' imported but unused

src/sage/features/latte.py:5:1 '.Feature' imported but unused
src/sage/features/latte.py:5:1 '.FeatureTestResult' imported but unused

src/sage/features/rubiks.py:12:1 '.Feature' imported but unused
src/sage/features/rubiks.py:12:1 '.FeatureTestResult' imported but unused

found 10 pyflakes errors in the modified files
pyflakes -- 0 seconds

Also

========== coverage ==========
git checkout patchbot/ticket_merged
Full doctests in features/internet.py: 2 / 2 = 100%
Missing doctests in features/mcqd.py: 0 / 1 = 0%
Missing doctests in features/meataxe.py: 0 / 1 = 0%
Missing doctests in features/mip_backends.py: 0 / 4 = 0%
Missing doctests in features/sagemath.py: 1 / 7 = 14%
Missing doctests in features/tdlib.py: 0 / 1 = 0%
Coverage went from 50872 / 52638 = 96.645% to 50873 / 52652 = 96.621%
====================

Answer 58 · 2021-10-15T20:07:20.000Z

Changed commit from 08248a1 to 759c88b

Answer 59 · 2021-10-15T20:07:20.000Z

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

`759c88b`	`sage.doctest, sage.control: Remove unused imports`

Answer 60 · 2021-10-15T20:16:20.000Z

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

`74e5c9c`	`src/sage/features/mcqd.py: Add doctests`

Answer 61 · 2021-10-15T20:16:20.000Z

Changed commit from 759c88b to 74e5c9c

Answer 62 · 2021-10-15T20:16:44.000Z

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

`15729dc`	`src/sage/features/mcqd.py: Add doctests`

Answer 63 · 2021-10-15T20:16:44.000Z

Changed commit from 74e5c9c to 15729dc

Answer 64 · 2021-10-15T20:17:54.000Z

comment:45

Here are some fixes for these style warnings, contributions welcome

Answer 65 · 2021-10-16T03:47:36.000Z

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

`1d02bd0`	`More doctests for coverage`

Answer 66 · 2021-10-16T03:47:36.000Z

Changed commit from 15729dc to 1d02bd0

Answer 67 · 2021-10-16T03:50:02.000Z

comment:47

Added more doctests to make bots happy. I could not run tests for numerical backends (cplex, gurobi, coin) because I failed to install them.

Answer 68 · 2021-10-16T03:55:48.000Z

comment:48

Thank you! I'll test it with the coin backend.

Answer 69 · 2021-10-20T23:01:01.000Z

Changed branch from public/32649 to 1d02bd0

Answer 70 · 2021-11-13T18:35:46.000Z

Description changed:

--- 
+++ 
@@ -1,9 +1,9 @@
 Some of them already go through features - for those we just rename the feature so that its `name` attribute is equal to the intended "optional" tag.
 
-For the remaining ones, we add new Feature subclasses in new files `src/sage/features/internet.py`, `mip_backends.py`.
+For the remaining ones, we add new `Feature` subclasses in new files `src/sage/features/internet.py`, `mip_backends.py`.
 
 Follow-ups:
 - #32650 `latex.py`, 
-- `interfaces.py`, 
+- #32866 `interfaces.py`, 
 - #32174, in which we can deprecate the `has_...` functions

Answer 71 · 2021-11-13T18:35:46.000Z

Changed commit from 1d02bd0 to none