Opened 7 years ago
Closed 6 years ago
#20182 closed enhancement (fixed)
Automatic doctest for external softwares
Reported by:  Kwankyu Lee  Owned by:  

Priority:  major  Milestone:  sage7.2 
Component:  doctest framework  Keywords:  
Cc:  Merged in:  
Authors:  Kwankyu Lee  Reviewers:  John Palmieri 
Report Upstream:  N/A  Work issues:  
Branch:  c4e48bd (Commits, GitHub, GitLab)  Commit:  c4e48bd22aa1f018436795c14efbdc78a47673e1 
Dependencies:  Stopgaps: 
Description (last modified by )
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 (newstyle) packages; if "external" is listed, will also run tests for available external softwares; if set to "all", then all tests will be run
Change History (73)
comment:1 Changed 7 years ago by
Branch:  → public/20182 

comment:2 Changed 7 years ago by
Commit:  → a0df81845fd4b85ed411ce322ce879ac84e2df52 

comment:4 followup: 14 Changed 7 years ago by
Don't forget internet  or should that be its own ticket, since it's a different kind of nonpackage? One could presumably do some connectivity test and if it failed, not run the internet tests  they should really nearly always be run.
comment:5 followup: 8 Changed 7 years ago by
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.
comment:6 Changed 7 years ago by
Description:  modified (diff) 

comment:7 Changed 7 years ago by
Description:  modified (diff) 

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" :)
comment:8 followup: 11 Changed 7 years ago by
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.
comment:9 followup: 12 Changed 7 years ago by
It would be cool if this would automatically go over all test functions instead of this ugly errorprone 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')
comment:10 Changed 7 years ago by
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.
comment:11 followup: 15 Changed 7 years ago by
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 nooptional
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...
comment:12 Changed 7 years ago by
Replying to jdemeyer:
It would be cool if this would automatically go over all test functions instead of this ugly errorprone 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!
comment:13 Changed 7 years ago by
I think this is only really useful if "nonpackages" are tested by default.
comment:14 Changed 7 years ago by
Replying to kcrisman:
Don't forget internet  or should that be its own ticket, since it's a different kind of nonpackage? 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.
comment:15 Changed 7 years ago by
Replying to klee:
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 ...
comment:16 Changed 7 years ago by
I would prefer "external" instead of "nonpackage", otherwise the optional=nonpackages
has a confusing double negation vibe.
comment:17 Changed 7 years ago by
The behavior "optional" tag has changed after the merged ticket: http://trac.sagemath.org/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": noop (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?
comment:18 followup: 19 Changed 7 years ago by
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?
sageruntests: error: optional option requires an argument
comment:19 Changed 7 years ago by
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?
sageruntests: 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".
comment:20 Changed 7 years ago by
Commit:  a0df81845fd4b85ed411ce322ce879ac84e2df52 → 126e4e50e2270ba43b3cfd026f0405f568fff9be 

comment:21 Changed 7 years ago by
Authors:  → Kwankyu Lee 

Description:  modified (diff) 
Priority:  minor → major 
Summary:  Automatic doctest for nonpackages → Automatic doctest for external softwares 
comment:22 Changed 7 years ago by
Commit:  126e4e50e2270ba43b3cfd026f0405f568fff9be → 2098dd0b60081231baa95475b879f2a38cfc6f55 

Branch pushed to git repo; I updated commit sha1. New commits:
2098dd0  Fixed gross logical bugs

comment:23 Changed 7 years ago by
Commit:  2098dd0b60081231baa95475b879f2a38cfc6f55 → 82d58ebf89b13e168af232dc4f10e92fe48ee205 

Branch pushed to git repo; I updated commit sha1. New commits:
82d58eb  Fixed a bug and the doc

comment:24 Changed 7 years ago by
Commit:  82d58ebf89b13e168af232dc4f10e92fe48ee205 → b95936b7af18809d2c169da70ad14a9456b0f09b 

Branch pushed to git repo; I updated commit sha1. New commits:
b95936b  Fixed a typo

comment:25 Changed 7 years ago by
Commit:  b95936b7af18809d2c169da70ad14a9456b0f09b → 20f692a848b3b373ab555c7dcddffb1c63e03361 

comment:26 Changed 7 years ago by
Commit:  20f692a848b3b373ab555c7dcddffb1c63e03361 → 39e7390be155409d19a2b972a1af860f24e86b40 

Branch pushed to git repo; I updated commit sha1. New commits:
39e7390  Fixed a doctest failure

comment:27 Changed 7 years ago by
Commit:  39e7390be155409d19a2b972a1af860f24e86b40 → ee257470f2bf355efff9065b4d6da6ac4b23829e 

Branch pushed to git repo; I updated commit sha1. New commits:
ee25747  Trimmed the codes and removed whitespaces

comment:28 Changed 7 years ago by
Description:  modified (diff) 

comment:29 followup: 32 Changed 7 years ago by
Why does LazySet
inherit from set
? Are you actually using any set
functionality?
comment:30 followup: 33 Changed 7 years ago by
If I do sage tp optional=sage,optional,external ...
, then instead of printing
Using optional=...,external,...
it would be nice if it printed
Using optional=...,internet,latex,maple,...
comment:31 Changed 7 years ago by
Commit:  ee257470f2bf355efff9065b4d6da6ac4b23829e → 86a7e6f8cb3845aa4551422941ee081cfc46f713 

Branch pushed to git repo; I updated commit sha1. New commits:
86a7e6f  Removed set inheritance of LazySet

comment:32 Changed 7 years ago by
Replying to jdemeyer:
Why does
LazySet
inherit fromset
? Are you actually using anyset
functionality?
No. I was not. See the last commit.
comment:33 followup: 34 Changed 7 years ago by
Replying to jhpalmieri:
If I do
sage tp optional=sage,optional,external ...
, then instead of printingUsing optional=...,external,...it would be nice if it printed
Using optional=...,internet,latex,maple,...
Hmmm. It seems possible... But what do you think it means that "maple" appears in the list? Does it mean that maple is available and tests depending on it will be tested. This behavior defeats the lazy mechanism. Does it simply mean that any doctests depending on maple will run if maple turns out to be available lazily and not otherwise? Then this is very different from an optional package, say gcc or mpir, appearing in the list.
comment:34 followup: 35 Changed 7 years ago by
Replying to klee:
Replying to jhpalmieri:
If I do
sage tp optional=sage,optional,external ...
, then instead of printingUsing optional=...,external,...it would be nice if it printed
Using optional=...,internet,latex,maple,...Hmmm. It seems possible... But what do you think it means that "maple" appears in the list? Does it mean that maple is available and tests depending on it will be tested. This behavior defeats the lazy mechanism. Does it simply mean that any doctests depending on maple will run if maple turns out to be available lazily and not otherwise?
I say no to the last question. I don't know how to balance the laziness with what I'm looking for. Maybe a summary at the end of doctesting of which external packages were detected, or (even better) the intersection of those external packages which were detected and those for which a test was run. I think it is important to provide feedback about this in order to debug failed tests (or slow tests, etc.).
comment:35 Changed 7 years ago by
I don't know how to balance the laziness with what I'm looking for. Maybe a summary at the end of doctesting of which external packages were detected, or (even better) the intersection of those external packages which were detected and those for which a test was run.
Because of the lazyset mechanism, at least one test was run for an external software detected in most cases. There may be exceptional cases to this, but is this important? By the way, I prefer "external software" than "external package". Internet is rather a software than a package :)
I think it is important to provide feedback about this in order to debug failed tests (or slow tests, etc.).
I agree. Perhaps we can list the detected softwares at the end of doctesting as you suggested.
comment:36 Changed 7 years ago by
Commit:  86a7e6f8cb3845aa4551422941ee081cfc46f713 → f58377af2e7a7ac9e08335a42e52c31736e49626 

comment:37 Changed 7 years ago by
Commit:  f58377af2e7a7ac9e08335a42e52c31736e49626 → f4df0b843b9afcc8eebd7dd6267bf5045481a8f0 

Branch pushed to git repo; I updated commit sha1. New commits:
f4df0b8  Cleaned up util

comment:38 Changed 7 years ago by
The latest patch copes with the multiprocessing of Sage doctesting.
comment:39 Changed 7 years ago by
This is silly:
raise Exception # should not happen!
Replace it by something like
raise AssertionError("invalid value for self.seen")
comment:40 Changed 7 years ago by
Can you add a timeout (using alarm()
) for this:
urllib.request.urlopen("http://www.sagemath.org")
comment:41 Changed 7 years ago by
Commit:  f4df0b843b9afcc8eebd7dd6267bf5045481a8f0 → 6d360016e067bc0035aac4a9c324e371953360f9 

Branch pushed to git repo; I updated commit sha1. New commits:
6d36001  Improved the raise statement

comment:42 Changed 7 years ago by
Commit:  6d360016e067bc0035aac4a9c324e371953360f9 → 781796004587c55aaa385754f69498c1cecfea0d 

comment:43 followup: 49 Changed 7 years ago by
On my computer, doctesting the file external.py
causes two windows to open up, showing the results from view('2+3')
. Try to find another way to test whether latex is present and functional, maybe something like this:

src/sage/doctest/external.py
diff git a/src/sage/doctest/external.py b/src/sage/doctest/external.py index f05bc8d..463413a 100644
a b def has_latex(): 59 59 sage: has_latex() # random 60 60 True 61 61 """ 62 from sage.misc.latex import view 62 from sage.misc.latex import _run_latex_, _latex_file_ 63 from sage.misc.temporary_file import tmp_filename 63 64 try: 64 view('2+3') 65 f = tmp_filename(ext='.tex') 66 O = open(f, 'w') 67 O.write(_latex_file_('3')) 68 O.close() 69 _run_latex_(f) 65 70 return True 66 71 except Exception: 67 72 return False
Also, you might consider adding ImageMagick
(and in particular the convert
command) to the set of external programs to be tested.
comment:44 followup: 45 Changed 7 years ago by
Also, I'm getting a lot of failed internet
doctests, and some for latex
, too. If we enable testing those by default, Sage won't pass doctests anymore. They need to be fixed: on separate tickets?
comment:45 Changed 7 years ago by
comment:46 Changed 7 years ago by
I had to make this change in oeis.py
to fix one failure:

src/sage/databases/oeis.py
diff git a/src/sage/databases/oeis.py b/src/sage/databases/oeis.py index ff88286..b8de905 100644
a b class OEISSequence(SageObject): 841 841 A000045: Fibonacci numbers: F(n) = F(n1) + F(n2) with F(0) = 0 and F(1) = 1. 842 842 843 843 sage: f.keywords() # optional  internet 844 ('core', 'nonn', 'nice', 'easy', 'hear' )844 ('core', 'nonn', 'nice', 'easy', 'hear', 'changed') 845 845 846 846 TESTS:: 847 847
comment:47 followup: 54 Changed 7 years ago by
I think that the internet
doctests have to be robust enough that if an external website changes something, our tests will still pass. I tried to argue for this at #16252, but there was disagreement. I think it's either that or we don't enable internet
testing by default.
comment:48 Changed 7 years ago by
Commit:  781796004587c55aaa385754f69498c1cecfea0d → 4dc6df1f078cd58a8cc1c0f25dd941b94dbe5664 

comment:49 Changed 7 years ago by
Replying to jhpalmieri:
Also, you might consider adding
ImageMagick
(and in particular theconvert
command) to the set of external programs to be tested.
For this ticket, I will stop at providing the framework for tests requiring external softwares. Thus
(1) Any expert is cordially invited to contribute commits for better and new has_xxx
tester for the software xxx
.
(2) Turning on external
by default should be dealt with in another ticket.
(3) Fixing all doctest failures that result from changing the default should be dealt with separate tickets.
comment:50 Changed 7 years ago by
Commit:  4dc6df1f078cd58a8cc1c0f25dd941b94dbe5664 → 82b20cac6b47ecebc3852a21bfad1142635a73bd 

Branch pushed to git repo; I updated commit sha1. New commits:
82b20ca  Added a doctest

comment:51 followup: 53 Changed 7 years ago by
Or perhaps you should indeed define make (p)testall(long)
to use optional=sage,optional,external
comment:52 Changed 7 years ago by
Commit:  82b20cac6b47ecebc3852a21bfad1142635a73bd → 36b4f7aefaab9453600f93e0bce43998e19da50d 

Branch pushed to git repo; I updated commit sha1. New commits:
36b4f7a  Define make (p)testall(long) to use optional=sage,optional,external

comment:53 Changed 7 years ago by
Replying to jdemeyer:
Or perhaps you should indeed define
make (p)testall(long)
to useoptional=sage,optional,external
Done. What about make (p)testoptional(long)
? Perhaps optional=sage,optional
?
comment:54 Changed 7 years ago by
I think that the
internet
doctests have to be robust enough that if an external website changes something, our tests will still pass. I tried to argue for this at #16252, but there was disagreement. I think it's either that or we don't enableinternet
testing by default.
Agreed. (In your example, presumably 'core' in f.keywords()
would be better?) But I also think we must enable the internet testing within some framework people will test relatively often  if make testall
is that place, that's fine. Otherwise untested is broken  between OEIS and some of the timeseries stuff there is probably a lot eventually broken.
comment:55 Changed 7 years ago by
Commit:  36b4f7aefaab9453600f93e0bce43998e19da50d → 6d7d3d5b9bb1f1ba3e068fdd5a3766a5415dd41c 

Branch pushed to git repo; I updated commit sha1. New commits:
6d7d3d5  Define make (p)testoptional(long) to use optional=sage,optional

comment:56 Changed 7 years ago by
Status:  new → needs_review 

comment:57 Changed 7 years ago by
Milestone:  sage7.1 → sage7.2 

comment:58 Changed 7 years ago by
Why the patchbot is not working on this ticket? Since I made changes on Makefile
?
comment:59 Changed 6 years ago by
comment:60 Changed 6 years ago by
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.
comment:61 Changed 6 years ago by
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".
comment:62 followup: 64 Changed 6 years ago by
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
comment:63 Changed 6 years ago by
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
?
comment:64 Changed 6 years ago by
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.
comment:65 Changed 6 years ago by
Commit:  6d7d3d5b9bb1f1ba3e068fdd5a3766a5415dd41c → b243bd6365aebb2bac096eedf262ed961a862f07 

comment:66 Changed 6 years ago by
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) 
comment:67 Changed 6 years ago by
Commit:  b243bd6365aebb2bac096eedf262ed961a862f07 → 308643a1d25c8683d1945d94f5ae182035f32025 

Branch pushed to git repo; I updated commit sha1. New commits:
308643a  Add and trim docstrings

comment:68 Changed 6 years ago by
Commit:  308643a1d25c8683d1945d94f5ae182035f32025 → 08aca6e4b275abe7f86adffc6deeb047a68dedab 

Branch pushed to git repo; I updated commit sha1. New commits:
08aca6e  trac 20182: minor rewording, remove "random" from one doctest

comment:69 Changed 6 years ago by
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?
comment:70 Changed 6 years ago by
Commit:  08aca6e4b275abe7f86adffc6deeb047a68dedab → c4e48bd22aa1f018436795c14efbdc78a47673e1 

comment:71 Changed 6 years ago by
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!
comment:72 Changed 6 years ago by
Reviewers:  → John Palmieri 

Status:  needs_review → positive_review 
comment:73 Changed 6 years ago by
Branch:  public/20182 → c4e48bd22aa1f018436795c14efbdc78a47673e1 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
Rewrite of automatic doctests for nonpackages