Skip to content

Change Install to pip in .readthedocs.yaml #154

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

Merged
merged 13 commits into from
Sep 16, 2021

Conversation

jukent
Copy link
Contributor

@jukent jukent commented Sep 15, 2021

Our Read the Docs builds are often failing due to excessive memory consumption. According to this Read-the-Docs Doc a recommended work around is to use pip instead of conda for install whenever possible.

Example configuration file documentation

This method seems to only work with a .txt file, and fails with ERROR: Invalid requirement: . . . when I use the existing .yaml. I wonder if there is another way.

@jukent jukent requested a review from a team as a code owner September 15, 2021 17:52
@jukent jukent requested review from dcamron and mgrover1 September 15, 2021 17:52
@jukent jukent requested a review from kmpaul September 15, 2021 18:14
@jukent jukent added the infrastructure Infrastructure related issue label Sep 15, 2021
@brian-rose
Copy link
Member

Hmmm it's too bad we're running into limitations building on readthedocs. Ideally we could preview a build of each PR using the same build system that the actual portal site is using. This change would move us farther away from that, since the pip environment won't be identical to the conda-forge environment we're using to build on GitHub.

I wonder if there's another approach. For the Foundations site, we don't use Readthedocs at all. Rather, we build the PR on GitHub Actions (using the same build environment as our main build) and then display the rendered site on Netlify.

Would the same solution work for the sphinx-based portal site? Would we lose some important functionality that readthedocs provides? I'm not sure.

@jukent
Copy link
Contributor Author

jukent commented Sep 16, 2021

I wonder if there's another approach. For the Foundations site, we don't use Readthedocs at all. Rather, we build the PR on GitHub Actions (using the same build environment as our main build) and then display the rendered site on Netlify.

Would the same solution work for the sphinx-based portal site? Would we lose some important functionality that readthedocs provides? I'm not sure.

That sounds better to me, perhaps we discuss this at the next IWG meeting. Unfortunately this is holding up some content PRs as well #147 and #150

@brian-rose
Copy link
Member

That sounds better to me, perhaps we discuss this at the next IWG meeting. Unfortunately this is holding up some content PRs as well #147 and #150

Ok, so maybe we should merge this change as a temporary fix but discuss a new strategy for rendering PRs at the next IWG.

@jukent
Copy link
Contributor Author

jukent commented Sep 16, 2021

Ok, so maybe we should merge this change as a temporary fix but discuss a new strategy for rendering PRs at the next IWG.

That sounds like a good plan to me, maybe we can check in with some other people today before we merge.

Copy link
Contributor

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

This looks good to me! Thanks for making these changes @jukent

@kmpaul kmpaul changed the title Change Install to pip in .eadthedocs.yaml Change Install to pip in .readthedocs.yaml Sep 16, 2021
matplotlib
pandas
pyyaml
pre-commit
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need precommit for readthedocs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I just copied all of the dependencies from the environment.yaml but maybe if we're having 2 locations for dependencies anyway - they can be different?

Copy link
Collaborator

@kmpaul kmpaul left a comment

Choose a reason for hiding this comment

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

Thanks, @jukent!

@kmpaul kmpaul merged commit 3d77d2d into ProjectPythia:main Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Infrastructure related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants