turn on Sphinx warnings as errors #13544

aaronmarkham · 2018-12-05T02:49:17Z

Description

This PR updates the website build trigger sphinx-build with the -W switch. This turns on the "warnings as errors" feature in Sphinx.

edit: This needs to be considered carefully as it may impact several people as they get used to the change. Also, the website build process exempts the old versions as they are still error-riddled.
Are there any other exemptions needed before proceeding with this PR?

What This Means

Once merged, anyone that submits a PR that breaks the docs - particularly the Python docs - their PR will fail CI.

If you create a C operator that has docs strings that bubble up into the Python docs and break there, your PR will fail CI.

Testing Your Code Prior to a PR

But let's not fail in CI all of the time. You can test it out first.

Turn off the doc sets you don't need to test in docs/settings.ini. Most of the time you just need to update the [document_sets_default] section. I tend to turn everything off for faster cycles on testing the build, then turn everything back on for a final test if my change messed with something that might trigger a Doxygen issue (C or C++ change) or Scala/Java docs issue (.scala or .java change).
Run make html USE_OPENMP=1 SPHINXOPTS=-W from the docs folder. (or make docs with the same parameters from mxnet root)
If it fails, read the error and fix it. Once you get a successful build, you're ready to submit your PR.

For documentation help, check out the documentation guide.
For details on building the website, check out how to build the website

Testing & CI Deployment

I tested this with ./build_all_version.sh "v1.4.x;master" "1.4.0;master". The script will run the older version branch (that still has docs errors in it) and then runs master. Since the script only enables -W for master, it will show the warnings, but not stop the build when building the v1.4.x branch. When it eventually works on master, the -W flag is enabled, and if there are any Sphinx warnings it stops the build with an error.

Currently the Jenkins job for the website deployment builds master first. I could switch that to last, but actually, I think it is better that the job fails quickly if it is going to fail.

If you have questions, please ask!

aaronmarkham · 2018-12-05T02:51:42Z

@mli @anirudhacharya @lupesko @marcoabreu @larroy @sergeykolychev @lebeg - Please review. Not sure if this is the best time to do this, but we've shored up the current bugs in the docs, so if this goes through we can keep the Python API docs/website from breaking.

aaronmarkham · 2018-12-06T00:57:32Z

This is time sensitive. As it ages, we'll have another round of cleaning up bugs that are introduced to get a clean start.

marcoabreu · 2018-12-06T02:56:16Z

docs/build_version_doc/build_all_version.sh

@@ -117,6 +120,8 @@ function checkout () {
  git checkout "$repo_folder" || git branch $repo_folder "upstream/$repo_folder" && git checkout "$repo_folder" || exit 1
  if [ $tag == 'master' ]; then
    git pull
+    # master gets warnings as errors for Sphinx builds


Shouldn't we enable this in general? Otherwise a release branch might run into problems.

Also, is this script executed as part of the pr pipeline? We want to fail PRs ideally

I added the -W option to the new docs pipeline. This should force it to happen any time that runs. And the website build flow will still skip it for old versions. I'm not totally familiar with new pipelines, so please let me know if you think this is going to be an issue or not.

anirudhacharya · 2018-12-06T03:31:24Z

At what stage in the CI is this check performed? Can you please point to the logs where we can check and confirm?

aaronmarkham · 2018-12-06T05:34:00Z

At what stage in the CI is this check performed? Can you please point to the logs where we can check and confirm?

I just added it to the website pipeline in the deploy_docs() function. Look for the following test in a PR, and that's where the errors will pop up.

I see that the website validation job has a child 1.4.x job in addition to master. I imagine that will break if it also runs the deploy_docs() function, because many bugs that we've recently fixed in master have not been ported over to that branch.

aaronmarkham · 2018-12-06T06:09:56Z

It failed on the website pipeline. So, yay?
The failure is on a docs bug we already fixed here:
https://github.com/apache/incubator-mxnet/pull/13539/files#diff-631586dbe3b11092920c7268fdc499fc
Looks like I need a rebase and to try again. Actually a good test, assuming it passes after this.

aaronmarkham · 2018-12-06T06:22:05Z

Well that was a bit baffling... I rebased and the bug fixes were gone. Looks like this commit for 1.4.x reverted at least one of the changes in master, including the docs bug fixes. I think that commit should be reverted/fixed as that was probably not the intention.

@srochel - please take a look. docs/api/python/symbol/contrib.md shouldn't be in there. I also see some mkldnn stuff in there that isn't really a version bump.

TaoLv · 2018-12-06T07:52:03Z

@aaronmarkham I have submitted a PR to revert #13478 and am waiting for the CI finishing.

aaronmarkham · 2018-12-07T01:57:51Z

After patching the docs errors that came in from another PR, this PR passed fine.
I also tested another run through the CI and it passed fine.

I'll keep this open for a bit and rebase and catch some bugs, refer them back to their authors to help ease this in. Then around the 15th, we can go for merge? Sound good?

In the meantime, everyone doing docs/website updates, please use make html USE_OPENMP=1 SPHINXOPTS=-W to build the docs to catch the errors as this PR will.

larroy

I like the idea to fail CI if the docs break, so I support this PR but I'm not very familiar with the doc generation or the problems that were fixed, so probably best to have somebody else's opinion.

I didn't understand the story about let's not fail CI. CI is there to fail if something is broken. If you want to add a local test before sending to CI I suggest to add it to the dev_menu.py. Adding more commands / instructions and manual steps is something we should avoid since everything is complex enough and we should lower barrier to entry and contributions.

LGTM

aaronmarkham · 2018-12-17T16:43:12Z

@larroy:
The CI output is pretty clear and it will tell the contributor what broke in the docs. When there's only your error, it's not so bad. When it piles up to thousands of errors from every other PR that made it through without checking docs, then that's hard.
This has been sitting around for a week, so I can imagine one or two docs error have slipped in. I'm going to rebase it and clean up whatever might have slipped in. Then it should be good to go for merging.
Adding a menu item to the dev_menu.py script to check for docs errors is a great idea. It makes it easy for people to test locally before sending a PR through CI.

* turn on warnings as errors * move warnings as error logic to build_all_version * fix typo in comment * add warning as error option for docs pipeline * bump ci to test again; use this chance to add notes on this feature * fix bugs in image.py docs

aaronmarkham requested a review from szha as a code owner December 5, 2018 02:49

aaronmarkham changed the title ~~turn on Sphinx warnings as errors~~ [WIP] turn on Sphinx warnings as errors Dec 5, 2018

aaronmarkham added Build Doc labels Dec 5, 2018

aaronmarkham changed the title ~~[WIP] turn on Sphinx warnings as errors~~ turn on Sphinx warnings as errors Dec 6, 2018

marcoabreu reviewed Dec 6, 2018

View reviewed changes

aaronmarkham force-pushed the docs_ci_error branch from 388cb7f to 4d94733 Compare December 6, 2018 05:39

aaronmarkham force-pushed the docs_ci_error branch from 4d94733 to d3175df Compare December 6, 2018 06:11

aaronmarkham added the pr-awaiting-review PR is waiting for code review label Dec 6, 2018

anirudhacharya approved these changes Dec 6, 2018

View reviewed changes

nswamy added pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress labels Dec 7, 2018

larroy approved these changes Dec 7, 2018

View reviewed changes

aaronmarkham force-pushed the docs_ci_error branch from 7ea9038 to 44eb7ee Compare December 17, 2018 19:42

aaronmarkham added 6 commits December 17, 2018 17:16

turn on warnings as errors

02cb54e

move warnings as error logic to build_all_version

9061dee

fix typo in comment

e9b6fca

add warning as error option for docs pipeline

8062d49

bump ci to test again; use this chance to add notes on this feature

2771065

fix bugs in image.py docs

985c837

aaronmarkham force-pushed the docs_ci_error branch from 4e38be4 to 985c837 Compare December 18, 2018 01:16

aaronmarkham merged commit 67a9a81 into apache:master Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

turn on Sphinx warnings as errors #13544

turn on Sphinx warnings as errors #13544

aaronmarkham commented Dec 5, 2018 •

edited

Loading

aaronmarkham commented Dec 5, 2018 •

edited

Loading

aaronmarkham commented Dec 6, 2018

marcoabreu Dec 6, 2018

aaronmarkham Dec 6, 2018

anirudhacharya commented Dec 6, 2018

aaronmarkham commented Dec 6, 2018

aaronmarkham commented Dec 6, 2018

aaronmarkham commented Dec 6, 2018 •

edited

Loading

TaoLv commented Dec 6, 2018

aaronmarkham commented Dec 7, 2018

larroy left a comment •

edited

Loading

aaronmarkham commented Dec 17, 2018

turn on Sphinx warnings as errors #13544

turn on Sphinx warnings as errors #13544

Conversation

aaronmarkham commented Dec 5, 2018 • edited Loading

Description

What This Means

Testing Your Code Prior to a PR

Testing & CI Deployment

aaronmarkham commented Dec 5, 2018 • edited Loading

aaronmarkham commented Dec 6, 2018

marcoabreu Dec 6, 2018

Choose a reason for hiding this comment

aaronmarkham Dec 6, 2018

Choose a reason for hiding this comment

anirudhacharya commented Dec 6, 2018

aaronmarkham commented Dec 6, 2018

aaronmarkham commented Dec 6, 2018

aaronmarkham commented Dec 6, 2018 • edited Loading

TaoLv commented Dec 6, 2018

aaronmarkham commented Dec 7, 2018

larroy left a comment • edited Loading

Choose a reason for hiding this comment

aaronmarkham commented Dec 17, 2018

aaronmarkham commented Dec 5, 2018 •

edited

Loading

aaronmarkham commented Dec 5, 2018 •

edited

Loading

aaronmarkham commented Dec 6, 2018 •

edited

Loading

larroy left a comment •

edited

Loading