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 readthedocs build #1561

Merged
merged 16 commits into from Dec 15, 2020
Merged

Fix readthedocs build #1561

merged 16 commits into from Dec 15, 2020

Conversation

dsherry
Copy link
Collaborator

@dsherry dsherry commented Dec 15, 2020

Fix #1520

@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #1561 (557f45d) into main (39ce817) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1561   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         234      234           
  Lines       16827    16827           
=======================================
  Hits        16819    16819           
  Misses          8        8           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ce817...557f45d. Read the comment docs.

@@ -19,8 +19,7 @@ formats:

# Optionally set the version of Python and requirements required to build your docs
python:
version: 3.7
version: 3.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.

I don't think this was necessary but it seemed like a good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so too!

@@ -262,7 +268,7 @@ jobs:
python_version: "3.8"
steps:
- checkout
- install_dependencies_dev
- install_dependencies_docs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did this just so that the circleci build_docs job would have the same deps installed by us as on RTD.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the goal is to have the same deps as RTD, shouldn't we install setuptools>=45.1.0 before installing our package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@freddyaboulton I'd say my goal here was to have the circleci job have deps which are closer to whats used on the RTD docker image.

If we really wanted to go the extra mile, I think we should write a job which duplicates the RTD build in its entirety, including the docker image. We could write that automation ourselves, or we could see if there's something in the community which we can use.

Given that we're considering migrating to github actions early next year, I think we should punt on this question. But great thinking 😁

pydata-sphinx-theme>=0.3.1
Sphinx>=2.0.1
nbconvert>=5.5.0
nbsphinx>=0.4.2
Copy link
Collaborator Author

@dsherry dsherry Dec 15, 2020

Choose a reason for hiding this comment

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

I made two changes here:

  • Separated this out from dev-requirements.txt so that .readthedocs.yml can install these deps without having to install the others.
  • Changed these versions to be minimums instead of pinning. This means we use Sphinx 3.3.1 now.

I didn't confirm that these changes were necessary in order to fix the bug, it just felt like a good way to reduce complexity, speed up the build, to only install the minimum required deps in each RTD build, and to potentially get some Sphinx/other bugfixes.

install:
- requirements: docs/readthedocs-requirements.txt
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line is the fix: install setuptools>=45.1.0 before installing the docs requirements (sphinx etc) and our package itself.

@@ -1,10 +1,6 @@
-r requirements.txt
-r test-requirements.txt
-r docs-requirements.txt
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just moved the deleted lines into docs-requirements.txt below

.PHONY: installdeps-docs
installdeps-docs:
pip install -e . -q
pip install -r docs-requirements.txt -q
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is used by the circleci build_docs code at the top and isn't part of the bugfix itself.

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

@dsherry Heroic! Thanks for fixing this before our next release. Please let us know what RTD says about this. I'm curious why they were installing an older version of setuptools.

@@ -262,7 +268,7 @@ jobs:
python_version: "3.8"
steps:
- checkout
- install_dependencies_dev
- install_dependencies_docs
Copy link
Contributor

Choose a reason for hiding this comment

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

If the goal is to have the same deps as RTD, shouldn't we install setuptools>=45.1.0 before installing our package?

@@ -19,8 +19,7 @@ formats:

# Optionally set the version of Python and requirements required to build your docs
python:
version: 3.7
version: 3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

I think so too!

Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

nice!

@dsherry
Copy link
Collaborator Author

dsherry commented Dec 15, 2020

RTD came back green 💹 🥬 🍏 ♻️ 😂, merging!

@dsherry dsherry merged commit af44074 into main Dec 15, 2020
1 check passed
@dsherry dsherry deleted the ds_1520_rtd_build_3 branch December 15, 2020 22:58
@dsherry dsherry mentioned this pull request Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTD builds fail with numpy version RuntimeError
3 participants