sage.doctest.control: Exclude doctests in files via file directives ''# sage.doctest: optional - xyz'

[mkoeppe]

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

[mkoeppe]

Hoping we can make progress on this ticket this week - ​https://wiki.sagemath.org/days111

[mkoeppe]

We could also parse a directive "# doctest: optional - ..." in the file header

[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, 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?

[jhpalmieri]

New commits:

​b92e036

sage.doctest: Handle file directives '# sage.doctest: optional - xyz'

[mkoeppe]

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'

[mkoeppe]

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.

[mkoeppe]

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"

[mkoeppe]

OK, found some previous discussion: #3260, #20427

[mkoeppe]

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.

[jhpalmieri]

How about a change like this?

src/sage/doctest/control.py

@@ -216 +216 @@
     - ``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 +235 @@
         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 +258 @@
                 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

src/sage/misc/sageinspect.py

@@ -2031 +2031 @@
         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?

[mkoeppe]

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

[jhpalmieri]

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",

[roed]

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.

[SimonKing]

Replying to roed:

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).

[mkoeppe]

Yes, so let's do #30787...

[mkoeppe]

Replying to roed:

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

[mkoeppe]

... and on top of #30383 because we need to make it possible to use configure --disable-... for at least one standard package.

[mkoeppe]

Setting a new milestone for this ticket based on a cursory review.

[git]

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

​f35123c	Merge branch 'develop' into t/30778/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions
​94727df	trac 30778: patch sageinspect to not print warning when skipfile returns True.

[jhpalmieri]

This could still use some examples, as mentioned in comment:23 and comment:26.

[mkoeppe]

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.

[jhpalmieri]

@SimonKing?: 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.

[SimonKing]

Replying to jhpalmieri:

@SimonKing?: 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.

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.

[mkoeppe]

New commits:

​1276609

src/sage/matrix/matrix_gfpn_dense.pyx: Use # sage.doctest: optional - meataxe

[mkoeppe]

(untested)

[jhpalmieri]

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.

[mkoeppe]

Remains to put something in the documentation of matrix_gfpn_dense.pyx, as discussed in comment:8, comment:13, comment:14

[jhpalmieri]

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?

[mkoeppe]

Let's do this manually for this ticket first

[mkoeppe]

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)

[git]

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

​5cc1288

src/sage/matrix/matrix_gfpn_dense.pyx: Add reference to meataxe spkg

[mkoeppe]

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?

[jhpalmieri]

I'm happy with your changes on the ticket: positive review for those parts.

[mkoeppe]

Thanks!