sage.doctest.control: Exclude doctests in files via file directives ''# sage.doctest: optional - xyz'
mkoeppe opened this issue · 65 comments
When a file is marked # sage.doctest: optional - xyz, we omit it from doctesting unless --optional=xyz is given.
This will save us from having to add lots of # optional - ... tags to files in the course of modularization (#29705)
We do this by extending sage.doctest.control.skipfile, which already parses files for # nodoctest file directives.
Previous related proposals/discussions: #3260, #20427
Also related: #30746
CC: @simon-king-jena @kiwifb @roed314 @saraedum @nthiery @videlec
Component: doctest framework
Keywords: sd111
Author: Matthias Koeppe, John Palmieri
Branch/Commit: 5cc1288
Reviewer: John Palmieri, Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/30778
Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111
Changed keywords from none to sd111
Description changed:
---
+++
@@ -1,3 +1,4 @@
-When a file is marked `sage_setup: distribution = ...` (#29705), we omit it from doctesting.
+When a file is marked `sage_setup: distribution = ...` (#29705) and this distribution is not installed, we omit it from doctesting.
+This will save us from having to add lots of `# optional - ...` tags to files in the course of modularization (#29705)
Description changed:
---
+++
@@ -2,3 +2,4 @@
This will save us from having to add lots of `# optional - ...` tags to files in the course of modularization (#29705)
+We do this by extending `sage.doctest.control.skipfile`, which already parses files for `# nodoctest` file directives.We could also parse a directive "# doctest: optional - ..." in the file header
Replying to @mkoeppe:
We could also parse a directive "# doctest: optional - ..." in the file header
This has been discussed in the past and rejected, on the grounds that the directive is hidden when people look at the reference manual or just do x.method?, so they may expect those tests to work all the time. For any of these proposed directives, the corresponding page in the reference manual should have a clear warning. I don't know what to do about the help string for each relevant method/function/class. It's overkill to modify the help string based on such a directive, right?
Changed branch from u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions to none
New commits:
b92e036 | sage.doctest: Handle file directives '# sage.doctest: optional - xyz' |
Commit: b92e036
Description changed:
---
+++
@@ -1,4 +1,4 @@
-When a file is marked `sage_setup: distribution = ...` (#29705) and this distribution is not installed, we omit it from doctesting.
+When a file is marked `# sage.doctest: optional - xyz`, we omit it from doctesting unless `--optional=xyz` is given.
This will save us from having to add lots of `# optional - ...` tags to files in the course of modularization (#29705)
Changed branch from u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions to none
Replying to @jhpalmieri:
Replying to @mkoeppe:
We could also parse a directive "# doctest: optional - ..." in the file header
This has been discussed in the past and rejected
I was trying to find the ticket with this discussion but did not succeed
New commits:
b92e036 | sage.doctest: Handle file directives '# sage.doctest: optional - xyz' |
There is certainly an open question here. I don't think it's only a matter of marking up the doctests with "optional" tags. With modularization going forward, we need to find a way to inform the user that some Python module is provided by a particular distribution package. I think this information should be attached to the module name instead of cluttering the doctests.
Until we have a more systematic solution for this, I would just suggest that we add an admonition to the module documentation that says something like: "The classes in this module require the optional package rpy2 to be installed"
Looks like this topic is showing up every 4-6 years.
What has changed now is that modularization (#29705) would introduce LOTS of new optional tags if we don't find a systematic solution.
Description changed:
---
+++
@@ -3,3 +3,5 @@
This will save us from having to add lots of `# optional - ...` tags to files in the course of modularization (#29705)
We do this by extending `sage.doctest.control.skipfile`, which already parses files for `# nodoctest` file directives.
+
+Previous related proposals/discussions: Description changed:
---
+++
@@ -4,4 +4,7 @@
We do this by extending `sage.doctest.control.skipfile`, which already parses files for `# nodoctest` file directives.
-Previous related proposals/discussions:
+Previous related proposals/discussions: #3260, #20427
+
+Also related: #30746
+Author: Matthias Koeppe
How about a change like this?
diff --git a/src/sage/doctest/control.py b/src/sage/doctest/control.py
index 6e75131fb3..0b10e19552 100644
--- a/src/sage/doctest/control.py
+++ b/src/sage/doctest/control.py
@@ -216,6 +216,10 @@ def skipfile(filename, tested_optional_tags=False):
- ``tested_optional_tags`` - a list or tuple or set of optional tags to test,
or ``False`` (no optional test) or ``True`` (all optional tests)
+ If ``filename`` contains a line of the form ``"# sage.doctest:
+ optional - xyz")``, then this will return ``False`` if "xyz" is in
+ ``tested_optional_tags``. Otherwise, it returns the matching line.
+
EXAMPLES::
sage: from sage.doctest.control import skipfile
@@ -231,9 +235,11 @@ def skipfile(filename, tested_optional_tags=False):
sage: with open(filename, "w") as f:
....: _ = f.write("# sage.doctest: optional - xyz")
sage: skipfile(filename, False)
+ '# sage.doctest: optional - xyz'
+ sage: bool(skipfile(filename, False))
True
sage: skipfile(filename, ['abc'])
- True
+ '# sage.doctest: optional - xyz'
sage: skipfile(filename, ['abc', 'xyz'])
False
sage: skipfile(filename, True)
@@ -252,11 +258,11 @@ def skipfile(filename, tested_optional_tags=False):
m = optionalfiledirective_regex.match(line)
if m:
if tested_optional_tags is False:
- return True
+ return m.group(0)
optional_tags = parse_optional_tags('#' + m.group(2))
extra = optional_tags - set(tested_optional_tags)
if extra:
- return True
+ return m.group(0)
line_count += 1
if line_count >= 10:
break
diff --git a/src/sage/misc/sageinspect.py b/src/sage/misc/sageinspect.py
index a37edbbffc..670a4aa4e6 100644
--- a/src/sage/misc/sageinspect.py
+++ b/src/sage/misc/sageinspect.py
@@ -2031,6 +2031,15 @@ def sage_getdoc(obj, obj_name='', embedded_override=False):
return ''
r = sage_getdoc_original(obj)
s = sage.misc.sagedoc.format(r, embedded=(embedded_override or EMBEDDED_MODE))
+ f = sage_getfile(obj)
+ if f:
+ from sage.doctest.control import skipfile
+ skip = skipfile(f)
+ if skip:
+ warn = """WARNING: the enclosing module is marked '{}',
+so doctests may not pass.""".format(skip)
+ s = warn + "\n\n" + s
+ pass
# Fix object naming
if obj_name != '':Then the help message printed by obj? will include a warning if the file is tagged to be skipped. Is it okay if skipfile returns a string instead of just True?
I like this idea. How about using m.group(2) instead? I don't think the reader needs to know about the syntax of the # sage.doctest: directive
How about this? I also added a bit to the developer's guide.
New commits:
07818e6 | trac 30778: when a module is labeled "optional - xyz", |
Are there any files in the Sage library now that we want to use this feature on? I think having some examples (in this ticket or a followup) would be good.
There are also a couple test failures reported by the patchbot.
Replying to @roed314:
Are there any files in the Sage library now that we want to use this feature on? I think having some examples (in this ticket or a followup) would be good.
It makes sense for optional extension modules, such as sage.matrix.matrix_gfpn_dense (but I don't know if there are others). Indeed the doctests are one reason why my optional package p_group_cohomology is not part of the sage library, although is is mostly written in Python and Cython and would thus fit well into the Sage library (but should still be an optional package).
Replying to @roed314:
Are there any files in the Sage library now that we want to use this feature on? I think having some examples (in this ticket or a followup) would be good.
Indeed it would be best to test this on files that are in tree but could be made "optional" via the same mechanism that we already use for extensions modules depending on optional packages. Candidates are:
- modules depending on
giac - modules depending on
brial - modules depending on
ntl - more - see #30666
Since this will require modification of setup.py, it's best to do this on top of #31377, #30912
Changed author from Matthias Koeppe to Matthias Koeppe, John Palmieri
... and on top of #30383 because we need to make it possible to use configure --disable-... for at least one standard package.
Setting a new milestone for this ticket based on a cursory review.
This could still use some examples, as mentioned in comment:23 and comment:26.
As an example, we could apply it to src/sage/matrix/matrix_gfpn_dense.pyx and remove the line-by-line annotations "# optional: meataxe".
However, as git grep 'sage:' src/sage/matrix/matrix_gfpn_dense.pyx shows, there are some doctest lines that are not marked optional. Not sure if they have any value by themselves.
@simon-king-jena: would you object if we skipped all tests in src/sage/matrix/matrix_gfpn_dense.pyx unless meataxe is installed? I've skimmed through and don't see anything that (I hope) isn't tested elsewhere for other matrix implementations, but you know this file pretty well.
Replying to @jhpalmieri:
@simon-king-jena: would you object if we skipped all tests in
src/sage/matrix/matrix_gfpn_dense.pyxunlessmeataxeis installed? I've skimmed through and don't see anything that (I hope) isn't tested elsewhere for other matrix implementations, but you know this file pretty well.
I did not intend to create new tests for other matrix implementations. So, I wouldn't object to make all the module's tests optional. In fact, this is what I asked for since I first created the module.
New commits:
1276609 | src/sage/matrix/matrix_gfpn_dense.pyx: Use # sage.doctest: optional - meataxe |
(untested)
This is working for me the way that I think it should: commands like make ptestlong or ./sage -tp src/sage/matrix/ ignore matrix_gfpn_dense.pyx, but if you specify the file explicitly (or let the shell specify it with ./sage -tp src/sage/matrix/matrix_g*), then it gets tested, and of course many tests fail. I think that's the right behavior.
Remains to put something in the documentation of matrix_gfpn_dense.pyx, as discussed in comment:8, comment:13, comment:14
Replying to @mkoeppe:
Remains to put something in the documentation of
matrix_gfpn_dense.pyx, as discussed in comment:8, comment:13, comment:14
This is easy enough to do manually. It would be nice to autodetect the line # sage.doctest: optional - meataxe, but do we want to do that here or a follow-up ticket?
Let's do this manually for this ticket first
In #32614 I now define some new optional tags, in particular a tag sage.matrix.matrix_gfpn_dense. So if we depend on that ticket, we could now write # sage.doctest: optional - sage.matrix.matrix_gfpn_dense; and in the module documentation we could refer to the class providing the feature (sage__matrix__matrix_gfpn_dense)
Branch pushed to git repo; I updated commit sha1. New commits:
5cc1288 | src/sage/matrix/matrix_gfpn_dense.pyx: Add reference to meataxe spkg |
I've kept it simple and just added a note in the docstring.
But do we actually build HTML documentation for optional compiled modules at all?
I'm happy with your changes on the ticket: positive review for those parts.
Reviewer: John Palmieri, ...
Thanks!
Changed reviewer from John Palmieri, ... to John Palmieri, Matthias Koeppe