Automatic doctest for external softwares

[kwankyu]

Many doctests depend on softwares external to Sage (including "internet") available on the system, such as latex, magma, mathematica, maple, etc. This patch allows to run the optional doctests for the softwares available.

With the patch,

"sage -t --optional=PKGS": only run doctests including one of the "# optional - xxx" tags where xxx is listed in PKGS;
if "sage" is listed, will also run the standard doctests;
if "optional" is listed, will also run tests for installed optional (new-style) packages;
if "external" is listed, will also run tests for available external softwares;
if set to "all", then all tests will be run

This is a rewrite of #18904 and #13540.

Component: doctest framework

Author: Kwankyu Lee

Branch/Commit: c4e48bd

Reviewer: John Palmieri

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

[kwankyu]

Branch: public/20182

[sagetrac-git]

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

a0df818

Rewrite of automatic doctests for nonpackages

[sagetrac-git]

Commit: a0df818

[kwankyu]

comment:3

The patch is a draft or a proof of concept.

[kcrisman]

comment:4

Don't forget internet - or should that be its own ticket, since it's a different kind of non-package? One could presumably do some connectivity test and if it failed, not run the internet tests - they should really nearly always be run.

[jdemeyer]

comment:5

I don't like the fact that you explicitly need to specify --optional=nonpackages. That's almost as much "effort" as writing --optional=latex,magma,internet. Also the name "nonpackages" is a bit strange: why is "octave" a nonpackage?

Also, I don't believe that have_latex() is a sufficient test to check that latex works. You should really run latex to be sure. All the other tests actually run the optional program.

[kwankyu]

comment:7

The intention is to test the doctests of the form "#optional - xxx" where xxx is not a Sage package. So I think internet is also a "nonpackage" :-)

[jdemeyer]

comment:8

Replying to @jdemeyer:

I don't like the fact that you explicitly need to specify --optional=nonpackages.

To be clear, I meant that nonpackages should be passed by default if no --optional argument is given.

[jdemeyer]

comment:9

It would be cool if this would automatically go over all test functions instead of this ugly error-prone code:

    if test_latex(): pkgs.append('latex')
    if test_magma(): pkgs.append('magma')
    if test_matlab(): pkgs.append('matlab')     
    if test_mathematica(): pkgs.append('mathematica')  
    if test_macaulay2(): pkgs.append('macaulay2')  
    if test_octave(): pkgs.append('octave')  
    if test_scilab(): pkgs.append('scilab')  
    if test_maple(): pkgs.append('maple')
    if test_cplex(): pkgs.append('cplex')  
    if test_gurobi(): pkgs.append('gurobi')

[jdemeyer]

comment:10

You should also consider performance issues. I don't want to run all these tests every time I doctest something. You should look at #13540, which uses a lazy mechanism for testing optional tags.

[kwankyu]

comment:11

Replying to @jdemeyer:

Replying to @jdemeyer:

I don't like the fact that you explicitly need to specify --optional=nonpackages.

To be clear, I meant that nonpackages should be passed by default if no --optional argument is given.

Currently doctests for Sage optional packages, if installed on the system, are automatically run only if "--optional" is given. Doctests for nonpackages are run only if "--optional=nonpackages" is given. Perhaps we should keep this ability because of the performance issues that you mention...

[kwankyu]

comment:12

Replying to @jdemeyer:

It would be cool if this would automatically go over all test functions instead of this ugly error-prone code:

    if test_latex(): pkgs.append('latex')
    if test_magma(): pkgs.append('magma')
    if test_matlab(): pkgs.append('matlab')     
    if test_mathematica(): pkgs.append('mathematica')  
    if test_macaulay2(): pkgs.append('macaulay2')  
    if test_octave(): pkgs.append('octave')  
    if test_scilab(): pkgs.append('scilab')  
    if test_maple(): pkgs.append('maple')
    if test_cplex(): pkgs.append('cplex')  
    if test_gurobi(): pkgs.append('gurobi')

I totally agree! But I needed to write the code before I go to sleep, as I am living on the other side of the globe :-) You can help!

[jdemeyer]

comment:13

I think this is only really useful if "nonpackages" are tested by default.

[jhpalmieri]

comment:14

Replying to @kcrisman:

Don't forget internet - or should that be its own ticket, since it's a different kind of non-package? One could presumably do some connectivity test and if it failed, not run the internet tests - they should really nearly always be run.

There were some concerns about running internet tests whenever possible. I'm not sure I agree with those, but maybe there are other reasons not to do these all the time. (What if I lose my internet connection after testing testing connectivity but in the middle of doctesting? Do we have to test connectivity before every internet test?) But I think some of the patchbots could use the internet flag when doctesting.

[jhpalmieri]

comment:15

Replying to @kwankyu:

Currently doctests for Sage optional packages, if installed on the system, are automatically run only if "--optional" is given.

I don't think this is correct. When I run doctests without any extra flags, I see (after installing a few optional packages for testing purposes)

$ sage -tp src/sage/homology/
...
Using --optional=ccache,database_gap,gcc,mpir,ore_algebra,python2,sage
...

[vbraun]

comment:16

I would prefer "external" instead of "nonpackage", otherwise the --optional=nonpackages has a confusing double negation vibe.

[kwankyu]

comment:17

The behavior "--optional" tag has changed after the merged ticket: #18558, but the developer's manual seems not updated completely to reflect the changes. The following is from experiments and the documentation. So would you verify my understanding?

(1) "-t" : tests all ordinary doctests and also doctests about installed optional packages, in effect equivalent to "-t --optional=optional".
(2) "-t --optional": no-op
(3) "-t --optional=PKGS": only run doctests including one of the "# optional - xxx" tags where xxx is listed in PKGS;
if "sage" is listed will also test the standard doctests;
if "optional" is listed will also test all available(installed) optional packages;
if set to "all", then all tests will be run;
If this is correct, then

(A) Via this ticket I want to add one more line to (3):

                                if "external" is listed, then also include all doctests  with "# optional - xxx" where xxx is an available external softwares and resources like "magma", "matlab", "internet", "latex", etc (basically everything that is not a Sage standard or optional package).

(B) Also we can change (1) to equate "-t" to "-t --optional=optional,external".

Do you agree?

[kcrisman]

comment:18

This seems reasonable to me as a concept (implementation of (B) shouldn't slow things down ridiculously, though, so the devil may be in the details). Should "-t --optional" be equivalent to "-t" or is the desired behavior still thus?

sage-runtests: error: --optional option requires an argument

[kwankyu]

comment:19

Replying to @kcrisman:

This seems reasonable to me as a concept (implementation of (B) shouldn't slow things down ridiculously, though, so the devil may be in the details).

Agree. We definitely need to implement a lazy mechanism like that in #13540. I am reading the related codes in Sage.

Should "-t --optional" be equivalent to "-t" or is the desired behavior still thus?

sage-runtests: error: --optional option requires an argument

Once we implement "external", "all" will mean "sage"+"optional"+"external". Then we may set "-t --optional" to mean "-t --optional=all".

[sagetrac-git]

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

61b5136	Merge Sage 7.1.rc0 into trac20182
126e4e5	Initial implementaion using the lazyset mechanism

[sagetrac-git]

Changed commit from a0df818 to 126e4e5

[kwankyu]

Author: Kwankyu Lee

[sagetrac-git]

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

6d7d3d5

Define make (p)testoptional(long) to use --optional=sage,optional

[kwankyu]

comment:58

Why the patchbot is not working on this ticket? Since I made changes on Makefile?

[saraedum]

comment:59

I am working on a more generic way of checking for features of the system (including nice error messages) at #20382. klee: What do you think about it? You could potentially rebase your code to use the features there or I could do that once #20182 got merged.

[kwankyu]

comment:60

As this ticket is related to #20382, but independent from it. I think this ticket could be reviewed and merged independently, though still no one bothers reviewing it. Then I would be happy that you open a new ticket to use your features once #20382 got merged.

If #20382 is merged first, then I would also be very happy that you rebase this ticket to #20382.

[jhpalmieri]

comment:61

This doctest should be marked "random":

        sage: available_softwares.seen()
        ['internet', 'latex', 'magma']

Also, throughout, "softwares" should be "software" -- "which software do you have installed" is correct, for example, not "which softwares do you have installed".

[jhpalmieri]

comment:62

What should happen with failing doctests? klee's opinion is that they should be fixed on another ticket. I guess that's okay as long as they are not run by default. Anyone else have opinions?

For example:

sage -t src/sage/plot/graphics.py
**********************************************************************
File "src/sage/plot/graphics.py", line 1624, in sage.plot.graphics.Graphics.show
Failed example:
    plot(x, typeset='latex') # optional - latex
Expected nothing
Got:
    Graphics object consisting of 1 graphics primitive
**********************************************************************
File "src/sage/plot/graphics.py", line 1630, in sage.plot.graphics.Graphics.show
Failed example:
    plot(x, typeset='type1') # optional - latex
Expected nothing
Got:
    Graphics object consisting of 1 graphics primitive
**********************************************************************
1 item had failures:
   2 of  87 in sage.plot.graphics.Graphics.show

I get this at the end of make ptestall:

----------------------------------------------------------------------
sage -t src/sage/plot/graphics.py  # 2 doctests failed
sage -t src/sage/plot/plot.py  # 1 doctest failed
sage -t src/sage/arith/misc.py  # 1 doctest failed
sage -t src/sage/tests/cmdline.py  # 1 doctest failed
sage -t src/sage/dev/sagedev.py  # 2 doctests failed
sage -t src/sage/structure/sage_object.pyx  # 2 doctests failed
sage -t src/sage/combinat/designs/bibd.py  # 1 doctest failed
sage -t src/sage/misc/package.py  # 6 doctests failed
sage -t src/sage/doctest/external.py  # 1 doctest failed
sage -t src/sage/repl/rich_output/pretty_print.py  # 1 doctest failed
sage -t src/sage/dev/trac_interface.py  # 5 doctests failed
sage -t src/sage/repl/load.py  # 2 doctests failed
sage -t src/doc/en/developer/coding_basics.rst  # 1 doctest failed
sage -t src/sage/dev/patch.py  # 3 doctests failed
sage -t src/sage/databases/oeis.py  # 2 doctests failed
sage -t src/sage/combinat/integer_lists/invlex.pyx  # 1 doctest failed
sage -t src/sage/dev/digest_transport.py  # 1 doctest failed
sage -t src/sage/finance/stock.py  # 2 doctests failed
sage -t src/sage/misc/remote_file.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 1929.7 seconds
    cpu time: 3833.1 seconds
    cumulative wall time: 7184.9 seconds
External softwares detected for doctesting: internet,latex

[videlec]

comment:63

minor remark: there is a timeout optional argument for urlopen (that will gives you a URLError in case of failure). And in this particular test, why are you catching Exception?

[kwankyu]

comment:64

Replying to @jhpalmieri:

What should happen with failing doctests? klee's opinion is that they should be fixed on another ticket. I guess that's okay as long as they are not run by default.

This patch can be safely merged as long as the patchbot and the people checking a new Sage release use make ptestlong, which I think is the current practice. Then we need to open a ticket for the failing doctests for each external software, before we at some point switch to use make ptestall for a new release.

[sagetrac-git]

Changed commit from 6d7d3d5 to b243bd6

[sagetrac-git]

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

306067a	Minor fixes and URLError for internet timeout
b243bd6	Merge branch Sage 7.2.beta4 into trac20182

[jhpalmieri]

comment:66

Every function and method needs documentation and doctests.

$ sage --coverage src/sage/doctest/external.py 
------------------------------------------------------------------------
SCORE src/sage/doctest/external.py: 64.7% (11 of 17)

Missing documentation:
     * line 272: def __init__(self)
     * line 277: def __contains__(self, item)

Missing doctests:
     * line 224: def external_software()
     * line 236: def _lookup(software)
     * line 294: def issuperset(self, other)
     * line 303: def seen(self)
------------------------------------------------------------------------

[sagetrac-git]

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

308643a

Add and trim docstrings

[sagetrac-git]

Changed commit from b243bd6 to 308643a

[sagetrac-git]

Changed commit from 308643a to 08aca6e

[sagetrac-git]

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

08aca6e

trac 20182: minor rewording, remove "random" from one doctest

[jhpalmieri]

comment:69

Here are a few minor rewordings. The only possibly significant change is removing "# random" from one doctest, but the same thing is doctested later in the same file (in AvailableSoftware) without that marking.

I am happy with this now. Does anyone else have any comments?

[sagetrac-git]

Changed commit from 08aca6e to c4e48bd

[sagetrac-git]

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

69bcf21	Change one doctest to be nonrandom
c4e48bd	Merge with other modifications

[kwankyu]

comment:71

The list "external_software" should not be random. For convenience of later modification, I changed one of the duplicate doctests. Thank you for elaborate review!

[jhpalmieri]

Reviewer: John Palmieri

[vbraun]

Changed branch from public/20182 to c4e48bd