Sphinx build: catch warnings for style errors

[alex-l-kong]

If you haven't already, please read through our contributing guidelines before opening your PR

What is the purpose of this PR?

Addresses and closes #197. Currently, our Sphinx build only fails in the case of an error (ex. syntax error in conf.py). However, we need to make sure it fails if a docstring is malformed as well. The reason is because documentation will change overtime. Either new functions will be defined or existing functions will change. We don't want to commit broken documentation to master because that will appear malformed on readthedocs.

How did you implement your changes

From what I've seen, it looks like all we need is to add a certain command in a .readthedocs.yml file. If not, then I'll have to look more into it. If yes, there may be some more documentation/import-related issues we need to fix.

Remaining Issues

Is there a way we can get better Google-style docstring checks (ex. something equivalent to pylint for this)? It seems like the general consensus from the sample I've read online is that this is something that one shouldn't completely rely on ReadTheDocs to do that for you. Sounds kind of silly but it is what it is if this is really the case. That or we ask people to really test the documentation manually before requesting reviews/merging into master.

[alex-l-kong]

With the new Makefile and .readthedocs.yml config attached, ReadTheDocs now will fail the build if a warning is raised (ex. one of your docstrings is misformatted).

Here's the problem: Sphinx is extremely sloppy when it comes to throwing warnings for Google-style documentation. As in, almost non-existent. I've updated the Remaining Issues section on top but essentially sphinx-build with Napoleon really sucked at catching several misformed docstrings. The only warnings I was able to consistently get were unexpected indentations caused by having one too many spaces in the args and returns list.

Regardless, suggestions are welcome.

[ngreenwald]

Got it. May just be something we have to remember to check.
What causes an issue like this to happen? https://deepcell.readthedocs.io/en/master/API/deepcell.callbacks.html#deepcell.callbacks.RedirectModel
And would that be caught?

[alex-l-kong]

Got it. May just be something we have to remember to check.
What causes an issue like this to happen? https://deepcell.readthedocs.io/en/master/API/deepcell.callbacks.html#deepcell.callbacks.RedirectModel
And would that be caught?

I looked at the docstring and what seems to screw it up is their attempt to include code. From what I've read, using ``` isn't the proper way to do this. You would need to do something like what is mentioned here: https://stackoverflow.com/questions/44577360/google-style-docstring-example-section-is-not-rendering-as-a-code-snippet.

As for the entire code block going off the page, if using the double colon doesn't already fix this, you may need to include additional formatting prior (ex. a pipe |) before each line to force a break. Not tried it out myself yet but I read somewhere that this was the Sphinx way to do it.

No, I do not think ReadTheDocs would catch this because it seems like all sphinx-build checks for are indentation issues, and there weren't any in the docstring which generated the example you showed. Even then, it's pretty bad at that. Perhaps @ackagel can comment further to see if there were any other warnings Sphinx was generating when he updated the docstrings, but on my end, that's the only I could find.

[ackagel]

I didn't catch any particularly helpful warnings when I was reformatting the doc-strings.

That being said, there is a way we can roughly check for correct formatting and throw an error/warning otherwise. It should be possible to do some regex matching to verify the general structure of the docstrings, but this leaves argument's unchecked. Luckily, we can actually get the argument names programmatically and verify that they are all formatted correctly. This can be done in conf.py as follows:

import inspect


def check_format(app, what, name, obj, options, lines):
    if what == 'function':
        argnames = inspect.getargspec(obj)[0]
        if len(argnames) > 0:
            # loop through args+lines to find all mentions of each arg and assure one line matches w/ correct structure
            # otherwise break and raise error/warning
            # after looping over args, construct regex match for general structure
        else:
            # construct regex match w/o args requirement

        # compile and run the regex, throw error/warning if no match


def setup(app):
    app.connect('autodoc-process-docstring', check_format)

On the other hand, there's no non-overkill way to check for correct types, Returns, and Raises formatting. Regardless, I'd imagine strict argument format requirements will encourage people to correctly format their Returns and Raises.

[ackagel]

finding this functionality in the sphinx docs was ironically difficult :)

[ngreenwald]

That's great. I think if we have this in place it will go a long way towards fixing things. Also, we can add a section to the PR template asking people to check the documentation build. And this is only an issue if people modify the docstring, which doesn't happen as often as other code changes.

…

On Wed, Sep 2, 2020 at 11:54 AM vacuous_planet ***@***.***> wrote: finding this functionality in the sphinx docs was ironically difficult :) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#208 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJB47N3LRHD5Q567Z2XPLDSD2IFTANCNFSM4QQ2PLSA> .

[alex-l-kong]

So from my other branch on context spatial setup, I discovered that part of the reason we may not have been getting errors is because we're not building our docstrings correctly on pushing to readthedocs. You have to rebuild them using something like this: readthedocs/readthedocs.org#1139 (comment). I've updated the Makefile to do this locally for you, but you'd have to push those changes in the _markdown folder for them to be visible on ReadTheDocs. This solution should at least prevent us from having to store the actual _markdown files, basically, hiding the sphinx-apidoc call in conf.py to allow readthedocs to build the documentation for us. If needed, as @ackagel suggested, we can also add a docstring check on top of that.

[ngreenwald]

So in order for the build to fail, the user would have to build the docs locally and then push those as part of the PR?

[alex-l-kong]

So in order for the build to fail, the user would have to build the docs locally and then push those as part of the PR?

To guarantee it, yes, especially if you added a new docstring. I haven't tried the new Makefile on an intentionally horrendous docstring yet but my hope is that sphinx-apidoc can actually catch misformatted docstrings better than sphinx-build.

[ngreenwald]

What currently happens when you modify a docstring? Is there a local version that gets updated and needs to be checked in? Or we don't currently store a local version of the docs, and this change will a) build a local version and b) have that local version get checked? I'm hesitant to require people to run a separate make command and then also check in additional documentation related files

[alex-l-kong]

What currently happens when you modify a docstring? Is there a local version that gets updated and needs to be checked in? Or we don't currently store a local version of the docs, and this change will a) build a local version and b) have that local version get checked? I'm hesitant to require people to run a separate make command and then also check in additional documentation related files

So clarifications and corrections. When you push to readthedocs, currently all that is happening is that it runs sphinx-build for you on your most recent commit. sphinx-build is what actually builds the HTML from the .md files you have in your most recent commit (which in our case, we have to push ourselves because we have not configured readthedocs to create the .md files for us on each commit using sphinx-apidoc). Think of the .md files as sort of a map that helps sphinx-build locate each module, the functions within, and the associated docstrings.

To clarify further, when you run the Makefile on your machine, you generate local documentation that you can view using open _build/html/index.html in the docs folder. That's all cool...except when you push to GitHub, ReadTheDocs creates its own copy based on the built-in commands it uses. And by default, all it runs is sphinx-build. Meaning that if our Makefile has commands other than sphinx-build to generate updated .md files (which it does), they will not be seen unless you explicitly push the new .md files along with all your updated code. That way, sphinx-build can use the updated roadmap on each push to generate the documentation that will be seen by the public. Just remember that the Makefile is completely for local use and ReadTheDocs does not care to use it.

That's why we need to explicitly configure our conf.py so that it tells ReadTheDocs to run the sphinx-apidoc command to regenerate the .md roadmaps before it tries to build them. This will also have the side effect of us not having to store and push .md files to the repo ourselves, because ReadTheDocs will do it on its end. All we need to provide is an index.rst and a conf.py.

Unfortunately, it does not look like sphinx-apidoc actually checks the formatting of the docstrings when building the .md roadmaps. sphinx-build is the one that is supposed to do that, but the Napoleon extension, while great for parsing Google (and Numpy)-style docstrings, is also pretty bad at identifying ill-formatted ones from my experience. So we also will likely have to tell ReadTheDocs through conf.py to run a separate check outside of the warnings sphinx-build throws to identify these docstring offenders (see Adam's comment on how to do that).

[ngreenwald]

Okay awesome, that all makes sense.

Think of the .md files as sort of a map that helps sphinx-build locate each module, the functions within, and the associated docstrings.

This is because we're telling it that we're using google style docstrings, correct? There's not any actual docstring content in the .md files, just instructions on how to appropriately locate them?

This means we'll be able to remove the formatting specific .md files that can be globally specified in the conf.py instead, and then it's just a question of writing the appropriate parsing logic to catch the errors the RTD isn't catching?

[alex-l-kong]

So with the new conf.py the build does indeed catch docstring errors as evidenced in the above build. There are a few more cases that need to be handled (ex. return values) but overall it's looking super nice. Helped me locate at least three major documentation oopsies in the args list. And we don't need the _markdown folder anymore.

[ngreenwald]

That's great!

I feel bad for complaining about the coveralls UI: the UI for RTD failures is like 100x worse 😆

[ngreenwald]

Got it. I think it's much better to have up-to-date documentation with the full name of the function, as opposed to prettier looking docstrings that become out of date quickly. We should avoid at all costs the necessity of people needing to do anything locally to get the docstrings updated. As long as we have links to the right section of the docs sprinkled throughout the readme, I think the long names won't be a problem

…

On Sat, Sep 5, 2020 at 12:58 PM alex-l-kong ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In docs/conf.py <#208 (comment)> : > @@ -146,3 +150,84 @@ # set a maximum number of days to cache remote inventories intersphinx_cache_limit = 0 + +# this we'll need to build the documentation from sphinx-apidoc ourselves +# TODO: is the actually the best way to do it? Saves us a lot of trouble but the documentation +# cannot be changed by us if we completely rely on the ReadTheDocs backend to run sphinx-apidoc for us The problem is that sphinx-apidoc is very rigid when it comes to how it builds the .md roadmaps. That's why the table of contents (aka the toctree) on the left, as well as each individual module and function name, looks something like ark.utils.spatial_analysis_utils.compute_close_cell_num. If you allow ReadTheDocs to run sphinx-apidoc for you on their backend, you do no work outside of pushing to GitHub, but you have no way to change how sphinx-apidoc builds your documentation. On the other hand, if we force each member of our team to be very uptight about running the Makefile locally (which includes the sphinx-apidoc call) and committing our _markdown files to the Git repo directly, then that's a lot more work for us. However, we have the advantage of changing the style of our documentation to look much prettier. Problem is, we have to do that *each* time we run the Makefile because lo and behold, sphinx-apidoc will overwrite your old nicely-formatted documentation with its new version with ugly headings. VVL seems to be using something like the second method as I can clearly see their .rst files in a separate folder (API). However, while they have set up a nicer table of contents system, it also looks like they got really lazy and just stopped reformatting the doc headings each time they ran sphinx-apidoc. I mean, who *really* wants to go through those ugly-looking rst files (md files in our case) and reformat them every time? I'm not sure how other projects do it, but I imagine some teams only run sphinx-apidoc once to build their initial version of the documentation, reformat it afterward, and then never run it again, instead, having some people add to and change the documentation manually to preserve the sanity of how their documentation displays on ReadTheDocs. It probably works really well if you don't expect to be committing a ton of new documentation each commit. I personally don't mind our method, but that's because I can actually make sense of the whole blah.blah.blah mess. It is possible that some of the other members of the lab might find it somewhat cumbersome to have to sort through that. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#208 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJB47ICCXWFRACEYATFL6DSEKJ7FANCNFSM4QQ2PLSA> .

[alex-l-kong]

Sounds good, we'll move forward doing that then.

[ngreenwald]

Looks great! I'm gonna wait till @ackagel takes a look since my ability to give useful feedback on this sphinx stuff is fairly limited. I'v put together an issue for the next tasks for documentation #213

[ackagel]

Looks fantastic! Just one optional comment on the argnames vs params checks.