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

Let readthedocs build the documentation environment based on env_climada.yml #687

Merged
merged 13 commits into from
Apr 16, 2024

Conversation

peanutfun
Copy link
Member

@peanutfun peanutfun commented Mar 30, 2023

Changes proposed in this PR:

  • Remove requirements/env_docs.yml
  • Use requirements/env_climada.yml for building the docs. This will take a bit longer because the dependency solver actually has to resolve the environment.
  • Add notes on how to revert these changes in the future in case something goes wrong.

This PR fixes #654

PR Author Checklist

PR Reviewer Checklist

@emanuel-schmid
Copy link
Collaborator

@peanutfun do you know why the Read the Docs build failed?

@peanutfun
Copy link
Member Author

Yes, I am trying to install the additional dependencies in the post-install step of the readthedocs build, but the environment is not named climada_env. I will give this another try but I do not want to dig any deeper. To make this cleaner, we would need to join the env_climada.yml and env_developer.yml into a single file

.readthedocs.yml Outdated Show resolved Hide resolved
@peanutfun peanutfun marked this pull request as draft June 13, 2023 08:44
@peanutfun peanutfun marked this pull request as ready for review August 3, 2023 18:25
@peanutfun
Copy link
Member Author

The readthedocs build now takes 6 minutes, about the time of the unit tests. So I think the longer duration is no issue at this point.

@peanutfun
Copy link
Member Author

@chahank @emanuel-schmid This is working! We can ditch env_docs.yml and always build the documentation in an up-to-date environment.

@peanutfun
Copy link
Member Author

@emanuel-schmid I recall you were a bit opposed to the idea of having a "flexible" environment for the docs (right?). I would be happy about your approval as well before merging! 🙏

@emanuel-schmid
Copy link
Collaborator

🤷 Neither I'm I opposed to the idea of a flexible environment nor am I keen on keeping yet another yml file up to date. It's just that we didn't introduce the env_docs.yml file for the sheer fun of it. Back then, it solved a problem that came up as a surprise, caused by the the straw that breaks the camel's back so to say. At the very moment the problem is not present anymore but there is no guarantee that it won't re-appear one day.

In principle, I'm happy to merge this PR nevertheless: when the problems comes back we still can solve it in the same manner.

But - right now, the PR doesn't solve a present problem and while we wait with merging we save about 5 minutes of cpu time with each commit on any PR branch and whenever a PR is merged.
Can't we just wait then until the current env_docs.yml file eventually causes a problem? It certainly will. 😉

@peanutfun
Copy link
Member Author

@emanuel-schmid I don't think I agree. For now, there is no apparent problem, but the documentation will break as soon as a branch, e.g., adds a dependency. This PR avoids such issues in the future. Additionally, the build time does not suffer much. The last RTD build on this branch took 575s against 517s on the latest develop build.

Anyway, the approach is somewhat appealing. We can merge this when something with the docs breaks. However, we might then have to re-review this PR.

@emanuel-schmid
Copy link
Collaborator

the documentation will break as soon as a branch, e.g., adds a dependency.

👍

The last RTD build on this branch took 575s against 517s on the latest develop build.

Yes, but unlike the PR builds the develop build does the latex/pdf stuff too on RTD. PR builds take something between 350s and 400s

However, we might then have to re-review this PR.

I'll take that upon me. 😁

@peanutfun
Copy link
Member Author

Very good. Then let's go with your approach and merge (only) as soon as something breaks 🙌

@peanutfun peanutfun marked this pull request as draft February 21, 2024 10:07
# Conflicts:
#	CHANGELOG.md
@emanuel-schmid emanuel-schmid marked this pull request as ready for review April 16, 2024 12:31
@emanuel-schmid emanuel-schmid merged commit 7ce9484 into develop Apr 16, 2024
11 of 12 checks passed
@emanuel-schmid emanuel-schmid deleted the remove-env-docs branch April 16, 2024 12:31
@peanutfun
Copy link
Member Author

I'm glad this finally became useful 😇

gdeskos pushed a commit to gdeskos/climada_python that referenced this pull request Sep 16, 2024
…ada.yml (CLIMADA-project#687)

* Use env_climada.yml as environment in readthedocs

Remove requirements/env_docs.yml

* Fix wrong path in doc/README.md

* Add note on the removal of requirements/env_docs.yml

* Try installing additional packages in readthedocs environment

* Try not specifying the readthedocs environment name

* Remove custom package installation from RTD config

* Fix Python version in RTD build process

* Update CHANGELOG.md

---------

Co-authored-by: emanuel-schmid <schmide@ethz.ch>
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.

3 participants