Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#22588 closed defect (fixed)

Let "make doc" really always work

Reported by: klee Owned by:
Priority: major Milestone: sage-8.0
Component: build Keywords: days85
Cc: Merged in:
Authors: Kwankyu Lee Reviewers: Florent Hivert, Erik Bray, John Palmieri
Report Upstream: N/A Work issues:
Branch: 8227d6b (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

If we change git branches, make doc often fails. It turns out that the cache mechanism in sage doc builder caused these failures. The patch implements a robust logic to auto-update module docs using the information from the Sphinx environment rather than from the cache.

Change History (57)

comment:1 Changed 3 years ago by klee

  • Authors set to Kwankyu Lee
  • Branch set to u/klee/22588

comment:2 Changed 3 years ago by git

  • Commit set to 7e6d6c6111fb01967347bc886cea43b0e4657fd7

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

7e6d6c6Fix doc builder logic

comment:3 follow-up: Changed 3 years ago by hivert

Hi,

Thanks for this suggestion ! I've a few question for your patch.

  • Usually caching is here for performance reason. Did you test that there isn't a huge speed loss ?
  • Can you explain a little in the description of this patch how your new robust logic works ?
  • Finally, from a first glance, It seems that you don't handle the 'underscore' option anymore, do you ?

Cheers,

Florent

comment:4 follow-up: Changed 3 years ago by hivert

I got

[...]
cd ../.. && sage-logger -p './sage --docbuild --no-pdf-links all html ' logs/dochtml.log
[dochtml] 
[dochtml] Building reference manual, first pass.
[dochtml] 
[dochtml] Error building the documentation.
[dochtml] Traceback (most recent call last):
[dochtml]   File "/home/data/Sage-Install/sage-7.5.1/local/lib/python/runpy.py", line 174, in _run_module_as_main
[dochtml]     "__main__", fname, loader, pkg_name)
[dochtml]   File "/home/data/Sage-Install/sage-7.5.1/local/lib/python/runpy.py", line 72, in _run_code
[dochtml]     exec code in run_globals
[dochtml]   File "/home/data/Sage-Install/sage-7.5.1/local/lib/python2.7/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
[dochtml]     main()
[dochtml]   File "/home/data/Sage-Install/sage-7.5.1/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 1618, in main
[dochtml]     builder()
[dochtml]   File "/home/data/Sage-Install/sage-7.5.1/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 316, in _wrapper
[dochtml]     getattr(get_builder(document), 'inventory')(*args, **kwds)
[dochtml]   File "/home/data/Sage-Install/sage-7.5.1/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 510, in _wrapper
[dochtml]     build_many(build_ref_doc, L)
[dochtml]   File "/home/data/Sage-Install/sage-7.5.1/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 252, in build_many
[dochtml]     ret = x.get(99999)
[dochtml]   File "/home/data/Sage-Install/sage-7.5.1/local/lib/python/multiprocessing/pool.py", line 567, in get
[dochtml]     raise self._value
[dochtml] AttributeError: 'NoneType' object has no attribute 'all_docs'
[...]

Here is how to reproduce the problem: I went to 7.6.beta6 and compiled the doc normally. Then I switched to 22588 and issued a make doc-clean and make doc.

Also the git managed file src/doc/en/reference/misc/sagetex.rst was deleted.

Last edited 3 years ago by hivert (previous) (diff)

comment:5 Changed 3 years ago by jdemeyer

  • Keywords days85 added

comment:6 Changed 3 years ago by git

  • Commit changed from 7e6d6c6111fb01967347bc886cea43b0e4657fd7 to 839c5f23b9831f219ab35ba9a82652cfe6e5469b

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

839c5f2Quickfix for a bug in new code

comment:7 in reply to: ↑ 4 Changed 3 years ago by klee

Try the quickfix to fix the problem. Meanwhile I will think to reply to your questions.

comment:8 Changed 3 years ago by git

  • Commit changed from 839c5f23b9831f219ab35ba9a82652cfe6e5469b to 45d53c410b089d58d758c1bc7c9b3bb3c7a4d6b9

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

45d53c4Quickfix for the second bug in new code

comment:9 Changed 3 years ago by klee

Yes, the new code is to blame for the phenomenon that the innocent "sagetex.rst" disappears! Hope that the two quickfixes work.

comment:10 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by klee

Now the code with the quickfixes works fine for me. How about you?

I will try to answer your questions as far as I understand the old and the new code :-) First I describe the use of cache in the old code:

The cache stores the list of the modules for which rst files were auto-generated in the previous run of the doc builder. In a new run, the list of the modules for which rst files should be auto-generated is computed and compared with the list in the cache. The rst files of newly added modules are auto-generated. Then the cache is updated.

In the new code:

In a new run, the list of the modules for which rst files should be auto-generated is computed, and the modules are put into "new", "updated", "removed", "old" bins, comparing with the list of rst files which were auto-generated in the previous run of the doc builder (this formation is in the Sphinx environment). For modules in "new" and "updated" bins, new rst files are auto-generated. The modules in "removed" are removed. The modules in "old" are just kept.

Replying to hivert:

Usually caching is here for performance reason. Did you test that there isn't a huge speed loss ?

The merit of caching here is that supposedly we do not need to look into the "sage/..." folder to get the list of modules for which rst files were auto-generated. The new code gets the information from the Sphinx environment (.all_docs). There is no loss in speed.

Can you explain a little in the description of this patch how your new robust logic works ?

The information about the files in "sage/..." from the Sphinx environment is *independent* from git branch changing, unlike the cache. The dependence of the cache on git branch was the cause of the failures.

Finally, from a first glance, It seems that you don't handle the 'underscore' option anymore, do you?

The (new/old) code is not responsible for handling the 'underscore' option. Is it?

Thanks for asking.

comment:11 in reply to: ↑ 10 Changed 3 years ago by hivert

Replying to klee:

Now the code with the quickfixes works fine for me. How about you?

I will try to answer your questions as far as I understand the old and the new code :-) First I describe the use of cache in the old code:

Thanks for clarifying all of these

The (new/old) code is not responsible for handling the 'underscore' option. Is it?

I'm pretty sure the old code is:

if (inherit_prev is None or inherit_prev != options.inherited or
            underscore_prev is None or underscore_prev != options.underscore):

They are two options that force us to regenerate all the stub:

  • inherit -- whether we show inherited funs and attrs in the doc
  • underscore -- whether we show private (starting with ) funs and attrs

If any of those two option are changed, then every stub must be regenerated, because these option are included in the stub file as in

.. automodule:: sage.sets.integer_range
   :members:
   :undoc-members:
   :show-inheritance:

This needs to be taken care of in your logic.

comment:12 Changed 3 years ago by klee

You are right. It seems that putting the options back in work forces us to resurrect the cache file. Then the role of the cache file is just to keep the previous settings of the options. I think I can do this. Do you have any other idea?

One alternative approach could be to resurrect the cache file in full force as it was and make its pickle *not* git-tracted... I am not sure if this is really possible and sound... What do you think?

comment:13 Changed 3 years ago by klee

I somehow presumed that the pickle of the cache is git branch dependent or git-tracted, and that caused failures. It is not true: the pickle is not git-tracted! Now I think problems were that the cache lacked the modification time info (so it could not be used to detect updated module) and that the old code did not remove rst files for removed modules.

I am now experimenting with the options. I resurrected the cache and use it to store the previously used options. I will push the commits after I am confirmed that it works well...

comment:14 Changed 3 years ago by klee

With the underscore option, it ends with an error (independent from the present patch). With the inherited option, it takes forever (understandably...). So I stopped this experiment. Anyway I can confirm that the options take effect :-)

comment:15 Changed 3 years ago by git

  • Commit changed from 45d53c410b089d58d758c1bc7c9b3bb3c7a4d6b9 to 70b80c9c98013cb466083818bfc131cd5f51047d

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

e9d311eMerge branch 'develop' into trac22588_sphinx
70b80c9Make options work again

comment:16 Changed 3 years ago by jhpalmieri

This is extremely minor, but as far as I can tell, the preferred capitalization is "reST", not "ReST". The official documentation only uses "reStructuredText", not the abbreviation, but the lowercase "r" suggests "reST", and that's what others use (see http://www.sphinx-doc.org/en/stable/rest.html, for example).

comment:17 Changed 3 years ago by git

  • Commit changed from 70b80c9c98013cb466083818bfc131cd5f51047d to 0adc50e2b26b40d68c53b8cb8878c684bbc917db

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

0adc50eCorrect ReST to reST

comment:18 Changed 3 years ago by klee

  • Component changed from documentation to build
  • Status changed from new to needs_review

comment:19 Changed 3 years ago by klee

  • Description modified (diff)
  • Summary changed from Let make (doc) really always work to Let "make (doc)" really always work

comment:20 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Let "make (doc)" really always work to Let "make doc" really always work

comment:21 Changed 3 years ago by klee

One annoying problem I noticed is that we now have these warning as a side effect of __import__(module):

[dochtml] /Users/Kwankyu/GitHub/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py:909: DeprecationWarning: the module sage.rings.commutative_ring is deprecated and will be removed
[dochtml] See http://trac.sagemath.org/20011 for details.
[dochtml]   __import__(module)

Could we just ignore these or can you provide how to fix this?

As I understand, the Sphinx autodoc extension also import a module to do its work. So importing a module is not a problem. Sphinx seems not to spit out the warning messages...

comment:22 Changed 3 years ago by git

  • Commit changed from 0adc50e2b26b40d68c53b8cb8878c684bbc917db to 15e9ec01ac175b589e6bdb52629ce6bbf797269b

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

8fd3b68Minor fixes in docstrings
15e9ec0Suppress deprecation warnings

comment:23 Changed 3 years ago by klee

I decided to suppress the deprecation warnings. It works well.

comment:24 follow-up: Changed 3 years ago by jhpalmieri

I'm not sure it's a good idea to suppress deprecation warnings. The underlying issues should be fixed, instead. (There are deprecation warnings already. Does your branch create new ones?)

Last edited 3 years ago by jhpalmieri (previous) (diff)

comment:25 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Please do not make useless changes like

 def format_annotation(annotation):
-    """Return formatted representation of a type annotation.
+    """
+    Return formatted representation of a type annotation.

That will only introduce merge conflicts.

comment:26 Changed 3 years ago by git

  • Commit changed from 15e9ec01ac175b589e6bdb52629ce6bbf797269b to 1e39367abfd82bc4e41669a80acb8a5d3bfdce21

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

bbc8e0dRevert "Minor fixes in docstrings"
1e39367Just one minor fix in a log statement

comment:27 in reply to: ↑ 24 Changed 3 years ago by klee

Replying to jhpalmieri:

I'm not sure it's a good idea to suppress deprecation warnings. The underlying issues should be fixed, instead. (There are deprecation warnings already. Does your branch create new ones?)

You are right. Deprecation warnings were already there. My branch did not create new ones.

I think this is a good case where suppressing deprecation warnings is good and harmless. Those warnings are targeted for *users* of deprecated classes and methods. In our case, there is no real use of the deprecated. And the user who is building the reference manual would just wonder why he or she gets those warnings and has nothing to do to avoid them.

comment:28 Changed 3 years ago by git

  • Commit changed from 1e39367abfd82bc4e41669a80acb8a5d3bfdce21 to fb9c5a814ec67b27a1fb481c5400bb3f1140e2d5

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

fb9c5a8Change module to module_name for consistency

comment:29 Changed 3 years ago by klee

  • Status changed from needs_work to needs_review

comment:30 Changed 3 years ago by git

  • Commit changed from fb9c5a814ec67b27a1fb481c5400bb3f1140e2d5 to c52696ec0b7245d664897a687f87399dd0262ee4

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

c52696eMerge branch 'sage 7.6'

comment:31 Changed 3 years ago by klee

It is a pain to see document building fail with the branch changed. This patch should be merged as soon as possible. Please review if you agree.

comment:32 Changed 3 years ago by jdemeyer

Florent, can you review this?

comment:33 follow-ups: Changed 3 years ago by embray

  • Reviewers set to Florent Hivert, Erik Bray

Thanks for trying to fix this--it's been a huge source of irritation for me as well.

Florent is the expert on this code so he should have final say (but I'm not sure where he is--I haven't seen him in the office this week). But the logic here makes sense to me. I just have a few questions:

In the old get_newly_included_modules(self, save=False) method (which is now obsolete--it is not used anywhere and could be removed?) there is a save argument. Your new get_new_and_updated_modules() method also has the save argument but it isn't used anywhere (nor would it be).

Also I think it's a little confusing that what is now being called the "cache" is just storing the options that were used in the last docbuild run, and nothing else AFAICT (since as you point out the same information can be found in the Sphinx environment).

comment:34 Changed 3 years ago by git

  • Commit changed from c52696ec0b7245d664897a687f87399dd0262ee4 to 8b0f3ae1c60bb97a94cc8dd0538b2bd4db7c0703

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

8b0f3aeRemove unused method and argument

comment:35 in reply to: ↑ 33 Changed 3 years ago by klee

Replying to embray:

In the old get_newly_included_modules(self, save=False) method (which is now obsolete--it is not used anywhere and could be removed?) there is a save argument. Your new get_new_and_updated_modules() method also has the save argument but it isn't used anywhere (nor would it be).

I agree. Removed them just now.

Also I think it's a little confusing that what is now being called the "cache" is just storing the options that were used in the last docbuild run, and nothing else AFAICT (since as you point out the same information can be found in the Sphinx environment).

You are right. I wanted to get rid of the cache file. But unfortunately, as Florent noted, it turned out that we need to save the last options used somewhere, to decide whether to rebuild all modules or not given the new options. So the main role of the cache changed (and I started to call the cache "reference cache" -- a cache for building the reference)

comment:36 in reply to: ↑ 33 Changed 3 years ago by hivert

Replying to embray:

Florent is the expert on this code so he should have final say (but I'm not sure where he is--I haven't seen him in the office this week). But the logic here makes sense to me.

Hi Embray ! I've been sick and unable to work for one week. That's why you haven't seen me. I'm currently catching up and then I'll review this ticket.

comment:37 Changed 3 years ago by klee

I wonder which is the case: this patch is not so useful to developers as I believe or they are too busy to spare some time for this ticket...

comment:38 Changed 3 years ago by embray

I think it's definitely useful--people are just busy.

comment:39 Changed 3 years ago by git

  • Commit changed from 8b0f3ae1c60bb97a94cc8dd0538b2bd4db7c0703 to f9df7cf58c39e2c2b45e41237f3107c3aebab4d2

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

f9df7cfMerge branch 'develop' into sphinx_trac22588

comment:40 Changed 3 years ago by klee

  • Milestone changed from sage-7.6 to sage-8.0

comment:41 Changed 3 years ago by git

  • Commit changed from f9df7cf58c39e2c2b45e41237f3107c3aebab4d2 to 8227d6bf837095bf4299197d0def98a97e77937d

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

8227d6bMerge branch 'develop' into sphinx_trac22588

comment:42 Changed 3 years ago by klee

Florent, I beg your review.

comment:43 follow-up: Changed 3 years ago by kcrisman

I'm not quite competent to review this but it would be really useful ... ping!

comment:44 in reply to: ↑ 43 Changed 3 years ago by klee

Replying to kcrisman:

I'm not quite competent to review this but it would be really useful ... ping!

Yes! I repeatedly rely on this patch when I work with my branch, since without it "make" often fails while building docs. I just wonder how other developers live with the limped doc builder. I hoped this had been merged to Sage 8.0...

comment:45 follow-up: Changed 3 years ago by embray

I wonder if it would also enable speeding up patchbot builds, if we don't have to do a full make doc-clean on every build...

comment:46 in reply to: ↑ 45 Changed 3 years ago by klee

Replying to embray:

I wonder if it would also enable speeding up patchbot builds, if we don't have to do a full make doc-clean on every build...

In theory, yes. If we are 100% sure that the doc builder is totally reliable and robust against changing branches after this ticket is merged, then we may make it an option to do make doc-clean in the patchbot.

On the other hand, I once observed that some hyperlinks don't work properly in the docs built without make doc-clean.

comment:47 Changed 3 years ago by embray

I think, for the patchbot's purposes, it's fine if the docs don't build 100% correctly as long as they can be built without errors from Sphinx, or problems that impact the doctests.

If nothing else, once this (or a fix like it) is merged it would be worth updating the patchbot to make the doc-clean an option, as you suggest, and experiment with running without it for a while.

comment:48 follow-up: Changed 2 years ago by jhpalmieri

I would like to give this a positive review, but here is an issue: I tested this by deleting a file – I happened to choose homology/koszul_complex.py, along with its link in the reference manual. When I rebuilt the docs with the develop branch, it crashed because of the missing file, but with this branch, the docs built. That's great. What's not as great is that with this branch, the other pages in the homology part of the reference manual were not changed to reflect the missing page, so for example, cell_complex.html was not rewritten and so still has the line

    <link rel="next" title="Koszul Complexes" href="koszul_complex.html" />

(This means that at the top of that page, it says "Next page: Koszul complexes".) Fortunately (?), the page koszul_complex.html was not deleted, so the link still works.

Note also that there are still references to koszul_complex.html in the search indices.

The problem with docbuilding before this branch is that the auto-generated koszul_complex.rst would remain, while this branch does a good job of cleaning it up. What about these other references to koszul_complex.html? Deal with here or on another ticket, or ignore altogether?

Last edited 2 years ago by jhpalmieri (previous) (diff)

comment:49 Changed 2 years ago by jhpalmieri

A similar problem happens when you add a file: suppose you are adding a file "B.py" whose html file is supposed to go between "A.html" and "C.html" (that is, in "A.html", the next topic should point to "B.html"). If the docs have already been built, with A.html pointing to C.html, then you add B.html, the links in A.html and C.html will not change: they will point to each other as previous and next topics.

With the "develop" branch, docbuilding fails completely so you have to delete the built docs, so you never see this problem. With the branch from this ticket, you see it. Essentially the same question as in my previous comment: is the rest of this enough of an improvement that we can put off this problem to another ticket?

comment:50 in reply to: ↑ 48 Changed 2 years ago by klee

Replying to jhpalmieri:

The problem with docbuilding before this branch is that the auto-generated koszul_complex.rst would remain, while this branch does a good job of cleaning it up. What about these other references to koszul_complex.html? Deal with here or on another ticket, or ignore altogether?

This branch is about auto-generating .rst files correctly. So the problem of wrong cross-references could be dealt with separately in another ticket.

The problem of wrong cross-references seems related with how intersphinx extension works. I guess that the intersphinx inventory files should be updated somehow when the auto-generated .rst files are updated. I am not Sphinx expert though, and it would take me some time to figure out what is wrong and to find a proper fix.

comment:51 Changed 2 years ago by embray

I agree--the way I feel about this ticket is that it's not fixing every possible issue with incremental doc builds between branch changes, of which there are many. More importantly, it's just making it so that the doc build succeeds without crashing. It's not generally too important for the docs themselves to be exactly correct in this case--for perfectly correct docs they should be built from scratch. Rather, this avoids having to rebuild the docs from scratch every time a branch is switched (this is especially nice for the patchbot).

I wouldn't say this would never lead to confusion--people might want to read the development docs, especially if they're working on the docs themselves. But this isn't a showstopper. The current situation, however, is very annoying :)

comment:52 Changed 2 years ago by jhpalmieri

  • Reviewers changed from Florent Hivert, Erik Bray to Florent Hivert, Erik Bray, John Palmieri
  • Status changed from needs_review to positive_review

Okay, then let's merge this and see how it goes.

comment:53 Changed 2 years ago by klee

Here is the ticket for the problem of wrong cross references

https://trac.sagemath.org/ticket/23602

comment:54 Changed 2 years ago by vbraun

  • Branch changed from u/klee/22588 to 8227d6bf837095bf4299197d0def98a97e77937d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:55 Changed 2 years ago by kcrisman

  • Commit 8227d6bf837095bf4299197d0def98a97e77937d deleted

Nice work!

comment:56 follow-up: Changed 2 years ago by jdemeyer

There is an important regression: #23975

comment:57 in reply to: ↑ 56 Changed 2 years ago by klee

Replying to jdemeyer:

There is an important regression: #23975

I looked at #23975. As far as I understand, an issue was raised about sage-location and fixed by a patch there. Is the regression still present? What is the regression? Is there something to be done here? Help me catch up.

Note: See TracTickets for help on using tickets.