Docbuilding without TESTS: blocks fails

[jm58660]

./sage --docbuild --no-tests all html

gives

[libs     ] docstring of sage.libs.pari.pari_instance:162: WARNING: Definition list ends without a blank line; unexpected unindent.
Error building the documentation.

Component: documentation

Keywords: --no-tests --include-tests-blocks SAGE_SKIP_TESTS_BLOCKS

Author: John Palmieri

Branch/Commit: f4c3edb

Reviewer: Jori Mäntysalo

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

[nexttime]

Changed keywords from none to --no-tests

[jdemeyer]

comment:2

I thought that docbuilding without TESTS was the default? But apparently not...

[jm58660]

comment:3

Replying to @jdemeyer:

I thought that docbuilding without TESTS was the default? But apparently not...

I would like it to be default for most users. But then for developers it might be easier if it is not.

Anyways, it seems that fix for this problem is removing two empty lines. Someone with more time should check what is the real problem.

[nexttime]

comment:4

Replying to @jm58660:

Replying to @jdemeyer:

I thought that docbuilding without TESTS was the default? But apparently not...

I would like it to be default for most users. But then for developers it might be easier if it is not.

Yes. It would be good anyway for different purposes to have something like a "Sage developer mode" which is not enabled by default (or in stable releases, say).

Anyways, it seems that fix for this problem is removing two empty lines. Someone with more time should check what is the real problem.

Is it something that should go into the Sage Developer Guide (in order to avoid it in the future)?

[jm58660]

comment:5

Replying to @nexttime:

Yes. It would be good anyway for different purposes to have something like a "Sage developer mode" which is not enabled by default (or in stable releases, say).

Hmm... yes and no. Would be good, but OTOH dangerous as something might work in developer mode only. Actually this one is just a case of that! I compiled docs for several betas of 7.3 in a desktop, but for final version I installed it with --no-tests to our server.

Anyways, it seems that fix for this problem is removing two empty lines. Someone with more time should check what is the real problem.

Is it something that should go into the Sage Developer Guide (in order to avoid it in the future)?

Some patchbot should compile with the flag and another without flag.

I doubt how many developer or reviewer would compile documentation twise just to be sure.

[jdemeyer]

comment:6

Replying to @jm58660:

I would like it to be default for most users. But then for developers it might be easier if it is not.

And how many developers actually compile their own documentation and view that? I guess not many. I do it rarely in case I'm working on a documentation ticket and want to see the output. And even if I do that, I don't care about the TESTS blocks. Those TESTS blocks are mostly useful when reading/writing the code.

I don't see the point of "optionally" removing TESTS blocks. That creates an interesting paradox: the people who know how to remove the TESTS blocks are precisely the people who would be most interesting in seeing the TESTS blocks.

Even the online documentation still has the TESTS blocks and that's the single place where they should not be shown.

(I know this is all orthogonal to this ticket, but I just don't understand why somebody would add an option to remove TESTS blocks but then not make it the default).

[jdemeyer]

comment:7

Replying to @jm58660:

Anyways, it seems that fix for this problem is removing two empty lines.

If you know the fix, can you put it on this ticket?

[jhpalmieri]

comment:8

I think this problem occurs because lines like the following (from Sage output in doctests around line 162 in sage.libs.pari.pari_instance)

    IFAC: cracking composite

match the regular expression which is supposed to signify the end of a TESTS block: a word all in caps followed by a colon (to match things like "ALGORITHM:" or "REFERENCES:" which we want to include in the documentation). As I think someone said, probably on the original ticket for skipping TESTS blocks, rather than regular expression matching, we should have a more sophisticated parser.

[nexttime]

comment:9

Replying to @jdemeyer:

Replying to @jm58660:

I would like it to be default for most users. But then for developers it might be easier if it is not.

And how many developers actually compile their own documentation and view that? I guess not many. I do it rarely in case I'm working on a documentation ticket and want to see the output. And even if I do that, I don't care about the TESTS blocks. Those TESTS blocks are mostly useful when reading/writing the code.

I don't see the point of "optionally" removing TESTS blocks. That creates an interesting paradox: the people who know how to remove the TESTS blocks are precisely the people who would be most interesting in seeing the TESTS blocks.

Even the online documentation still has the TESTS blocks and that's the single place where they should not be shown.

(I know this is all orthogonal to this ticket, but I just don't understand why somebody would add an option to remove TESTS blocks but then not make it the default).

I fully agree (and by the way think this belongs to this ticket).

Add --include-test-blocks for docbuilding and make --no-tests the default, probably completely removing that option.

IMHO TESTS: should never end up in PDFs for example; it might be convenient for some to review code / docstrings using HTML output, but those could explicitly ask for including them when building their docs. (Just wondering whether that's a "global" option that eventually triggers complete rebuild when changed.)

The problem I see (OTOH) is that EXAMPLES: and TESTS: are often rather randomly used; many doctests that no user would ever be interested in are in EXAMPLES:, while some TESTS: sections on the other hand contain examples that would be valuable to users as well. But that's just my uneducated impression I got over the last years, and I cannot give any recent examples, right now at least.

[jm58660]

comment:10

Replying to @jdemeyer:

I don't see the point of "optionally" removing TESTS blocks. That creates an interesting paradox: the people who know how to remove the TESTS blocks are precisely the people who would be most interesting in seeing the TESTS blocks.

I think that it was discussed when add the switch. But I don't remember what was the reason to not make it default.

[nexttime]

comment:11

For the record:

The online (terminal) help for sage.libs.pari.pari_instance gives the following:

...
Elliptic curves and precision: If you are working with elliptic
curves, you should set the precision for each method:

   sage: e = pari([0,0,0,-82,0]).ellinit()
   sage: eta1 = e.elleta(precision=100)[0]
   sage: eta1.sage()
   3.6054636014326520859158205642077267748
   sage: eta1 = e.elleta(precision=180)[0]
   sage: eta1.sage()
   3.60546360143265208591582056420772677481026899659802474544

Number fields and precision: TODO

   IFAC: cracking composite
      49191317529892137643

   IFAC: factor 6713103182899
      is prime

   IFAC: factor 7327657
      is prime

   IFAC: prime 7327657
      appears with exponent = 1

   IFAC: prime 6713103182899
      appears with exponent = 1

   IFAC: found 2 large prime (power) factors. [3, 1; 7327657, 1;
   6713103182899, 1] sage: pari.default("debug", 0) sage:
   pari(2^67+1).factor() [3, 1; 7327657, 1; 6713103182899, 1]

Class docstring:
module(name[, doc])

Create a module object. The name must be a string; the optional doc
argument can have any type.
Init docstring:  x.__init__(...) initializes x; see help(type(x)) for signature
(END) 

Note that everything below TODO (only up to Class docstring: of course) belongs to the TESTS: block. :-)

[jm58660]

comment:12

Replying to @nexttime:

The problem I see (OTOH) is that EXAMPLES: and TESTS: are often rather randomly used; many doctests that no user would ever be interested in are in EXAMPLES:, while some TESTS: sections on the other hand contain examples that would be valuable to users as well.

I agree. Most common problem is to not have a TESTS block at all. I have changed those in my Grand Poset Documentaion Polishing Project. :=)

I guess this is partly because --no-tests is new and not the default. It makes much less sense to differentiate between examples and tests when both are shown to the user.

[nexttime]

comment:13

Well, I do not know whether this was introduced at the same time, but the online help (foo?) meanwhile suppresses the TESTS: section by default, or at least tries to, as you can see above. ;-)

[nexttime]

comment:14

Couldn't resist:

diff --git a/src/doc/en/installation/source.rst b/src/doc/en/installation/source.rst
index 799b546..fa4a42e 100644
--- a/src/doc/en/installation/source.rst
+++ b/src/doc/en/installation/source.rst
@@ -999,7 +999,7 @@ Here are some of the more commonly used variables affecting the build process:
   you run ``make``, ``make doc``, or ``make doc-pdf``.
   For example, you can add ``--no-plot`` to this variable to avoid building
   the graphics coming from the ``.. PLOT`` directive within the documentation,
-  or you can add ``--no-tests`` to omit all "TESTS" blocks in the
+  or you can add ``--include-tests-blocks`` to include all "TESTS" blocks in the
   reference manual. Run ``sage --docbuild help`` to see the full list
   of options.
 
diff --git a/src/sage_setup/docbuild/__init__.py b/src/sage_setup/docbuild/__init__.py
index 1fc5ccb..e7363e7 100644
--- a/src/sage_setup/docbuild/__init__.py
+++ b/src/sage_setup/docbuild/__init__.py
@@ -1445,9 +1445,9 @@ def setup_parser():
     standard.add_option("--no-plot", dest="no_plot",
                         action="store_true",
                         help="do not include graphics auto-generated using the '.. plot' markup")
-    standard.add_option("--no-tests", dest="skip_tests", default=False,
-                        action="store_true",
-                        help="do not include TESTS blocks in the reference manual")
+    standard.add_option("--include-tests-blocks", dest="skip_tests", default=True,
+                        action="store_false",
+                        help="include TESTS blocks in the reference manual")
     standard.add_option("--no-pdf-links", dest="no_pdf_links",
                         action="store_true",
                         help="do not include PDF links in DOCUMENT 'website'; FORMATs: html, json, pickle, web")

[jhpalmieri]

comment:15

Replying to @nexttime:

For the record:

The online (terminal) help for sage.libs.pari.pari_instance gives the following:

[snip]

Yes, and this is why I made comment above (plus looking at the documentation and source code in misc/sagedoc.py).

The original ticket #19503 was precisely about skipping TESTS blocks in introspection; the focus was not on the reference manual, and that's one reason the default is the way it is. Plus there was some discussion of the concern Leif expressed above, that some TESTS blocks have useful information, so if we remove them from the reference manual, we lose helpful information.

[nexttime]

comment:16

So here's the culprit (src/sage/misc/sagedoc.py, line 298 ff.):

    # end_of_block: match a line starting with whitespace, then Sphinx
    # directives of the form ".. foo:". This will match directive
    # names "foo" containing letters of either case, hyphens,
    # underscores.
    # Also match uppercase text followed by a colon, like
    # "REFERENCES:" or "ALGORITHM:".
    end_of_block = re.compile('[ ]*(\.\.[ ]+[-_A-Za-z]+|[A-Z]+):')
    # header: match a string of hyphens, or other characters which are
    # valid markers for ReST headers: - = ` : ' " ~ _ ^ * + # < >

Currently, only another TEST[S]:[:] doesn't terminate the block to skip:

...
        if not skip:
            m = tests_block.match(l)
            if m:
                skip = True
                indentation = m.group(1)
            else:
                s += "\n"
                s += l
        else:
            if end_of_block.match(l) and not tests_block.match(l):
                skip = False
                s += "\n"
                s += l
...

So we'd either have to whitelist PARI's IFAC: to not end a TESTS block, or (more seriously) have to explicitly look for all possible / "valid" section names (like AUTHORS, REFERENCES, etc.).

Not sure whether there is at all a complete list of such. (AFAIK, it's just informal convention to use this or that.)

[jhpalmieri]

comment:17

Or check the indentation level: to terminate the block, a string like AUTHORS: should be indented no more than the original TESTS:, whereas IFAC (and anything in the output of a Sage test) is indented more.

[jhpalmieri]

comment:18

Maybe something like

diff --git a/src/sage/misc/sagedoc.py b/src/sage/misc/sagedoc.py
index 968a44c..77d6984 100644
--- a/src/sage/misc/sagedoc.py
+++ b/src/sage/misc/sagedoc.py
@@ -322,6 +322,8 @@ def skip_TESTS_block(docstring):
                 s += l
         else:
             if end_of_block.match(l) and not tests_block.match(l):
+                if l.find(indentation) == 0:
+                    continue
                 skip = False
                 s += "\n"
                 s += l

[nexttime]

comment:19

I've tried another way:

diff --git a/src/sage/misc/sagedoc.py b/src/sage/misc/sagedoc.py
index a6c5568..dc5eb13 100644
--- a/src/sage/misc/sagedoc.py
+++ b/src/sage/misc/sagedoc.py
@@ -302,6 +302,13 @@ def skip_TESTS_block(docstring):
     # Also match uppercase text followed by a colon, like
     # "REFERENCES:" or "ALGORITHM:".
     end_of_block = re.compile('[ ]*(\.\.[ ]+[-_A-Za-z]+|[A-Z]+):')
+    # Only recognize commonly used section names, not e.g. output
+    # from doctests incidentally matching '[A-Z]+:':
+    SECTION_NAMES = set([
+        "AUTHOR", "AUTHORS",
+        "ALGORITH", "ALGORITHMS",
+        "REFERENCE", "REFERENCES"
+    ])
     # header: match a string of hyphens, or other characters which are
     # valid markers for ReST headers: - = ` : ' " ~ _ ^ * + # < >
     header = re.compile(r'^[ ]*([-=`:\'"~_^*+#><])\1+[ ]*$')
@@ -320,7 +327,9 @@ def skip_TESTS_block(docstring):
                 s += "\n"
                 s += l
         else:
-            if end_of_block.match(l) and not tests_block.match(l):
+            m = end_of_block.match(l)
+            if m and not tests_block.match(l) \
+                 and m.group(1) in SECTION_NAMES:
                 skip = False
                 s += "\n"
                 s += l

Works for me modulo adding more section names.

[nexttime]

comment:20

... and modulo spelling of course. ;-)

[jhpalmieri]

comment:21

Speling is an issue, though ;) The likelihood of someone misspelling "ALGORITM", for example, seems pretty high, and it's only going to make a difference when they do it right after a TESTS block. Troubleshooting that will be difficult. Isn't indentation safer than whitelisting?

[jhpalmieri]

comment:22

With your proposal, we at least need to add "EXAMPLE" and "EXAMPLES", by the way.

[nexttime]

comment:23

Replying to @jhpalmieri:

Maybe something like

diff --git a/src/sage/misc/sagedoc.py b/src/sage/misc/sagedoc.py
index 968a44c..77d6984 100644
--- a/src/sage/misc/sagedoc.py
+++ b/src/sage/misc/sagedoc.py
@@ -322,6 +322,8 @@ def skip_TESTS_block(docstring):
                 s += l
         else:
             if end_of_block.match(l) and not tests_block.match(l):
+                if l.find(indentation) == 0:
+                    continue
                 skip = False
                 s += "\n"
                 s += l

I was about to write that l.find(indentation) is generally wrong (and should be l.startswith(indentation)), but then suddenly realized that .find() is not C's strstr() and even works for the empty string... m)

I'm not entirely sure yet your approach works though. Deeeeeep thinking required...

But anyway, I'll (ab)use my approach to find all kinds of section names (and misspellings) later...

(We could eventually also correct then.)

[jhpalmieri]

comment:64

Fixed.

[jdemeyer]

comment:65

Why this?

@@ -278,8 +279,8 @@ def skip_TESTS_block(docstring):
         sage: skip_TESTS_block(start + test + refs + test2 + directive).rstrip() == (start + refs + directive).rstrip()
         True
 
-        sage: header = 'Header:\n~~~~~~~~'
-        sage: fake_header = 'Header:\n-=-=-=-=-='
+        sage: header = ' Header:\n ~~~~~~~~'
+        sage: fake_header = ' Header:\n -=-=-=-=-='
         sage: skip_TESTS_block(start + test + header) == start + header
         True
         sage: skip_TESTS_block(start + test + fake_header).rstrip() == start.rstrip()

I think this doctest should work as it was before.

[jhpalmieri]

comment:66

The old test was not actually testing the "header" part of the code, since it was unindented compared to " TESTS:". Also, both header and fake_header were unindented, so both ended the TESTS block. So I added leading spaces.

[sagetrac-git]

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

20e5e59

trac 21172: rearrange doctests

[sagetrac-git]

Changed commit from 570fb47 to 20e5e59

[jhpalmieri]

comment:68

The doctests are organized better now (in my opinion). The PDF docs are not building right now, so "needs work". I'll try to figure out what's going on.

[jhpalmieri]

comment:69

It's working now. Maybe I forgot to do make doc-clean earlier.

[jm58660]

comment:70

The branch seems to be red again.

[jhpalmieri]

comment:71

I'll try to fix it after the next beta (although it is not clear that anyone is ever going to review it, so I may stop bothering after a while).

[jm58660]

comment:72

Replying to @jhpalmieri:

I'll try to fix it after the next beta (although it is not clear that anyone is ever going to review it, so I may stop bothering after a while).

I guess I must do it. I know about nothing about Sphinx, and actually don't even want to know, but...

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

f0e15e4	Minor changes to the code
da01272	Fixed merge conflict. Doctests pass.
804d2f2	Removed deprecation alias for min_wt_vec_gap
7210501	Trac #20953: Improve minimum_distance for linear codes
bf876cb	Updated [SageMath](../wiki/SageMath) version to 7.6.beta3
a7de051	trac 21172: TESTS blocks should end not on any line starting "CAPITALS:", but
9dd7e4c	trac 21172: TESTS blocks should also end on a line indented less than
5374ad5	trac 21172: update documentation
ef59f47	trac 21172: fix a problem with detecting headers
f4c3edb	trac 21172: rearrange doctests

[sagetrac-git]

Changed commit from 20e5e59 to f4c3edb

[jhpalmieri]

comment:74

Rebased to 7.6.beta3.

To help with reviewing: this does nothing explicitly with Sphinx, because that work was done in the earlier ticket dealing with TESTS blocks. This modifies the function that detects when TESTS blocks start and end, so it is plain Python dealing with regular expressions. Sphinx then uses this to process docstrings, but it was already doing that.

Oh, it does change one thing with Sphinx: it changes the default from including TESTS blocks to omitting them.

[jm58660]

Reviewer: Jori Mäntysalo

[jm58660]

comment:76

Replying to @jhpalmieri:

To help with reviewing: this does nothing explicitly with Sphinx, because that work was done in the earlier ticket dealing with TESTS blocks. This modifies the function that detects when TESTS blocks start and end, so it is plain Python dealing with regular expressions. Sphinx then uses this to process docstrings, but it was already doing that.

Thanks for the explanation. This was the first time I look at Python implementation of regexps, but it seems to be clear. I mark this as a positive review.

(Sidenote: We should have something better! I mean a structured way so that Sphinx (or whatever we would use) would know the blocks and one could say something like --docskip=tests,algorithm or like.)

[vbraun]

Changed branch from u/jhpalmieri/TESTS to f4c3edb