Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix docs build and reduce warnings #231

Merged
merged 7 commits into from
Apr 29, 2021

Conversation

uri-granta
Copy link
Collaborator

@uri-granta uri-granta commented Apr 22, 2021

The docs build was temporarily broken due to a nbsphinx bug. This PR:

  • updates nbsphinx to a version without the bug (see failing to parse long lines with docutils 0.17 spatialaudio/nbsphinx#549) [in fact this was done independently by another commit, so doesn't show up in the diff]
  • fixes the remaining sphinx build warnings
  • updates the docs build to error on warnings
  • creates a new tox job for building the docs (and updates the documentation)
  • creates a new github quality check job to run this job

Note that I may need to separately update the repository config to mark the new check as required for merges to develop.

@uri-granta uri-granta linked an issue Apr 22, 2021 that may be closed by this pull request
@hstojic hstojic added bug Something isn't working documentation Improvements or additions to documentation labels Apr 22, 2021
google-auth-oauthlib==0.4.4
google-pasta==0.2.0
gpflow==2.1.4
gpflow==2.1.5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think upgrading GPflow should be left to some other PR, probably it won't break any of the code, but just in case...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bear in mind this is the constraints for CI, not for the library itself, so this number isn't used by users

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevertheless, it might make sense to pin the versions of key libraries in requirements.txt, at least with ~. Though I'd certainly hope that a minor point revision change to gpflow would never break anything!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gpflow doesn't use semver. Patch versions are meant to be backwards compatible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already pin gpflow to 2.1.x in setup.py, so I would personally be happy with this update. Also I'd be very sad if the tests didn't catch a break.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, if patches should be backward compatible lets go with the upgrade

Copy link
Collaborator

@hstojic hstojic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far... why some notebooks didnt appear in manual docs build at the end?

@uri-granta
Copy link
Collaborator Author

Looks good so far... why some notebooks didnt appear in manual docs build at the end?

A change to docutils meant nbsphinx was automatically ignoring notebook files with lines longer than a (not very big) limit. Upgrading nbsphinx fixed it.

@@ -1,3 +1,8 @@
# ---
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joelberkeley You don't happen to know why this isn't working? I'm trying to mark this notebook as orphan (as described in https://nbsphinx.readthedocs.io/en/0.8.3/orphan.html) so we don't get a warning that it's not part of a TOC (it's only included as a doc link in the FAQ).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea. I've never worked with headers for notebooks

@uri-granta uri-granta requested a review from hstojic April 23, 2021 10:08
Copy link
Collaborator

@hstojic hstojic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good, tox build successful, no warnings shown and all notebooks seem to be there

Copy link
Collaborator

@johnamcleod johnamcleod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@uri-granta uri-granta merged commit c92930e into secondmind-labs:develop Apr 29, 2021
@uri-granta uri-granta deleted the uri/fix-docs-build branch April 29, 2021 08:29
@uri-granta uri-granta linked an issue Jul 1, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix docs build issues and improve testing Add doc build to CI
4 participants