circle.yml: Grep WARNINGs in docs log and fail #6030
Conversation
* Grep WARNINGs in docs log and fail if there are any found. Signed-off-by: mr.Shu <mr@shu.io>
|
@amueller This is a work in progress, I would like to leave this PR open until I resolve all the issues with |
|
Should fix #6025 |
|
circle CI is only run on master currently. maybe @waterponey can say how he tested #5578. |
|
I am not sure that this is the right way to do it. Sphinx can be told to
error when there are warnings. See
https://github.com/nilearn/nilearn/pull/871/files
However, our experience with nilearn doing this has been a bit painful:
most people do not know how to fix the build errors (and to be fair, they
can be quite subttle), so the maintainers end up finishing of the PRs.
|
|
Thanks for the input @GaelVaroquaux. Well, the PRs don't fail, master does, as circleCI is only run on master. So the PR can be merged, and if there is an error, the log might give insights into whether it's easy to fix or a maintainer has to step in. I'm not sure this is the best way to do it, either. But I'm not very happy with a lot of the work on cleaning up the doc build falling to me by default, and I'd like there to be at least some signal when things fail. Currently people probably don't realize they broke anything. |
|
@amueller If circleCI only runs on master how will people be notified prior to merging the PR to master? Does not that break the idea of Continuous Integration? |
|
people will not be notified prior to merging to master. |
* Fixed simple WARNINGs that were reported by circleCI as part of making sure the automatic docs generation does not fail and is properly tested too. Signed-off-by: mr.Shu <mr@shu.io>
|
@amueller @GaelVaroquaux If I am reading your comments correctly then this PR achieves the state @amueller would like the circleCI to be in. My last commit to this PR's branch fixes a few easy issues I managed to spot in the log output today but I was also not able to locally reproduce many of those that are currently visible in circleCI's log output. If you are satisfied with the state of this PR I would suggest that you merge into master and see if you are satisfied with the circleCI's log output. The circleCI build should not break anything as it will most probably fail to get past the section added by this PR's first commit (there will most probably be some |
|
We could run circleci on the PRs as well. @GaelVaroquaux is that what you do?
This is what we do. But it's overall a failure: people don't reproduce on
their computers the failure (they are too lazy to run "make html", and
only run "make html-noplot") and the debugging information given by the
error messages is useless. So people just complain, but don't fix
anything. The biggest bottleneck is the poor quality of the error
messages.
I have no good answer right now, we are still experimenting to see how we
can improve things.
|
|
I was thinking about contributing upstream to improve this. But there is this book to write... So the consequence is that maintainers have to fix the broken doc build you say? is that better or worse than the current state in sklearn? |
It's actually in numpydoc that it would probably be the most useful. As
Yes.
To tell the truth, I don't know. It has slowed down hugely our release of |
Talking about our experience in nilearn, my hope is that with time we will be more aware of these sphinx issues, know how to fix the most common ones and debug the less common ones. |
|
@GaelVaroquaux coming back to this after some time, I think most of our PRs don't touch the docs, or if they do, only in a small part. I think it would be worth giving it a go. |
|
There is very little cost in trying this out, I think. If it's not working well, we can turn it off again. |
|
FWIW in my experience, most of the hardest to track warnings comes from docstring formatting (you get very little help from numpydoc sphinx extension to track down the origin of the problem) and not from rst documentation. My guess is that in most problematic cases CircleCI would not run in the PR but master would break once the PR is merged.
That would be my inclination too. |
Doesn't CircleCI run on all PRs right now? |
Off the top of my head only on the ones that are touching the doc or examples folders or have something like build-doc in the last commit message. See https://github.com/scikit-learn/scikit-learn/blob/master/build_tools/circle/check_build_doc.py for the full details. |
|
I think this is no longer applicable. |
Signed-off-by: mr.Shu mr@shu.io