Opened 17 months ago
Closed 17 months ago
#28819 closed defect (fixed)
disable problematic external doctests
Reported by:  ghmwageringel  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  doctest framework  Keywords:  matlab, mathematica 
Cc:  Merged in:  
Authors:  Markus Wageringel  Reviewers:  Kwankyu Lee 
Report Upstream:  N/A  Work issues:  
Branch:  2b5583e (Commits, GitHub, GitLab)  Commit:  2b5583eb59741e194bb06cdf84301605f6111276 
Dependencies:  Stopgaps: 
Description
This ticket marks the doctests in sage.doctest.external
involving the standard interfaces to external software as not tested
and some tests in the Mathematica interface as mathematicafrontend
.
In particular, this is a workaround for the following problem:
Whenever Sage runs the tests for src/sage/doctest/external.py, it tries to run a computation in Matlab. Even though the interface sort of works in interactive mode, it does not work at all during doctest mode, causing Matlab to crash which results in two numbered log files being written to the user home directory such as matlab_crash_dump.2811191
and java.log.18975
. When running a patchbot, the home directory gets flooded by these files.
Regardless of the Matlab issue, external software like this should not be invoked during normal test runs, unless explicitly enabled when running all
or external
tests. Especially this applies to software that requires a license to run.
The doctests this ticket disables in sage.doctest.external
invoke functions that catch all exceptions and produce random output, so there is not much value in running those tests at all.
The Matlab problem still persists when running external doctests, but in that case failures are to be expected.
A secondary problem is solved in the Mathematica interface:
Usually, the Mathematica interface interacts with the program WolframKernel
. However, a few doctests related to rendering result in the frontend program Mathematica
to be launched by the WolframKernel. This is because graphics cannot be exported without access to a frontend, according to this question.
These instances of Mathematica
do not always seem to get closed properly. When running a patchbot, this leads to a growing number of orphan processes of Mathematica
which do not seem to react to a SIGTERM signal.
Marking the doctests as mathematicafrontend
effectively disables them. This solution had already been applied by #23112 for one of the doctests in the file.
Change History (15)
comment:1 Changed 17 months ago by
 Branch set to u/ghmwageringel/28819
 Commit set to 8584c39a07107502d5a4376f515152821effd644
comment:2 Changed 17 months ago by
 Keywords matlab mathematica added
 Status changed from new to needs_review
I let the patchbot run on this ticket a couple of times and it works as intended.
comment:3 Changed 17 months ago by
 Milestone changed from sage8.9 to sage9.0
comment:4 followup: ↓ 6 Changed 17 months ago by
Regardless of the Matlab issue, external software like this should not be invoked during normal test runs, unless explicitly enabled when running all > or external tests. Especially this applies to software that requires a license to run.
Then.., isn't this a right solution for the problems?
sage: from sage.doctest.external import has_matlab sage: has_matlab() # optional  matlab True
comment:5 Changed 17 months ago by
 Commit changed from 8584c39a07107502d5a4376f515152821effd644 to 304b51e4f202a9cd68c86998d8938bf493927391
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
304b51e  28819: avoid running external software during normal test runs

comment:6 in reply to: ↑ 4 Changed 17 months ago by
Replying to klee:
Then.., isn't this a right solution for the problems?
sage: from sage.doctest.external import has_matlab sage: has_matlab() # optional  matlab True
Indeed, that also works and has the advantage that the tests are not deactivated entirely. I have updated the branch accordingly and let the patchbot run on it. It works as intended on my end.
comment:7 Changed 17 months ago by
 Reviewers set to Kwankyu Lee
 Status changed from needs_review to positive_review
Thanks. It looks good to me.
comment:8 Changed 17 months ago by
Thank you for the review.
comment:9 Changed 17 months ago by
 Status changed from positive_review to needs_work
comment:10 followup: ↓ 11 Changed 17 months ago by
I now realized that the tag should be
sage: has_matlab() # random, optional  matlab True
because the doctest may fail when we do
sage bt optional=sage,optional,matlab src/sage/doctest/external.py
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 17 months ago by
Replying to klee:
because the doctest may fail when we do
sage bt optional=sage,optional,matlab src/sage/doctest/external.py
Well, that is just because the Matlab interface is broken. Running the Matlab tests on src/sage/interfaces/matlab.py
fails for the same reason, currently. When the interface is fixed, the function has_matlab()
should always return True – otherwise the doctest would reveal an actual bug in has_matlab()
.
Until then, I think it is fine if the doctest fails when the optional matlab
flag is set, as it correctly indicates a problem. At this point, nobody seems to care much about the Matlab interface nor run its tests, anyway.
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 17 months ago by
When the interface is fixed, the function
has_matlab()
should always return True – otherwise the doctest would reveal an actual bug inhas_matlab()
.
No. If you don't have matlab, then it should return False. Not only for matlab. Let's say you don't have magma, then
sage: has_magma() # optional  magma True
would fail if you run, for whatever reason,
sage bt optional=sage,optional,magma src/sage/doctest/external.py
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 17 months ago by
 Branch changed from u/ghmwageringel/28819 to u/ghmwageringel/28819v2
 Commit changed from 304b51e4f202a9cd68c86998d8938bf493927391 to 2b5583eb59741e194bb06cdf84301605f6111276
 Status changed from needs_work to needs_review
Replying to klee:
Let's say you don't have magma, then
sage: has_magma() # optional  magma Truewould fail
Every doctest in Sage tagged optional  magma
will fail if one does not have Magma, so this is expected. I do not have a strong opinion about this though, as it still solves the problem reported in this ticket, so here is a branch where the doctests in question are marked random
.
New commits:
2b5583e  28819: avoid running external software during normal test runs

comment:14 in reply to: ↑ 13 Changed 17 months ago by
 Status changed from needs_review to positive_review
Replying to ghmwageringel:
Replying to klee:
Let's say you don't have magma, then
sage: has_magma() # optional  magma Truewould fail
Every doctest in Sage tagged
optional  magma
will fail if one does not have Magma
But unlike every other doctest with optional  magma
, by its nature, has_magma()
should work without Magma. I think optional  magma
is used here simply as a trick to turn it off unless one explicitly wants to test it.
I do not have a strong opinion about this though, as it still solves the problem reported in this ticket, so here is a branch where the doctests in question are marked
random
.
Thanks.
comment:15 Changed 17 months ago by
 Branch changed from u/ghmwageringel/28819v2 to 2b5583eb59741e194bb06cdf84301605f6111276
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
28819: avoid running mathematica frontend in doctests
28819: avoid running external software during normal test runs