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

Adapt to new RTD configuration style #491

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Aug 7, 2023

This PR aims to get our current RTD builds in line with the upcoming changes to the required configuration file and prevent future build issues due to dependency changes at the same time.

Looking into our docs settings in more detail, I found that the upcoming changes do not just mean that a RTD config file v2 is required, but some options that are now specified at -> Advanced Settings -> Default Settings will be removed in favor of options in the config file, which already overwrite them if such a file is present, even if they are not specified there. That should explain why no PDF builds are produced at the moment. So step 1 of this PR is to append the config file such that these options are active again.

Recently, our builds of the message_data docs have started to fail. The reason for that, as far as I can tell, is that the default pip installation process on RTD used to include upgrade-strategy eager, but recently changed to upgrade-strategy only-if-needed. This seems to be in line with popular demand. Since I couldn't find any mention in the docs on how to enable the upgrade strategy we are used to again (apart from taking complete control and specifying every step of the workflow ourselves), I decided it might be best to follow RTD's recommendation for reproducible builds by pinning the package versions we require. For internal consistency, I want to do so on ixmp, message_ix, message-ix-models, and message_data.
Following the recommendation, I performed the following steps to reach the version of doc/requirements.txt present here:

  1. Activate venv, python -m pip install pip-tools
  2. pip-compile --allow-unsafe --extra docs -o doc/requirements.txt pyproject.toml
  3. Delete the lines following lines from the file manually:
ixmp @ file:///home/fridolin/ixmp
    # via ixmp (pyproject.toml)

Some notes on this process:

  • --allow-unsafe is going to be the default soon and so it is recommended to already run pip-compile with it now
  • I haven't figured out how to automatically exclude the lines regarding ixmp, they seem to be too localized to work on RTD and unnecessary, too, since we are installing ixmp from the local git clone, either way. This, however, means that one cannot expect to run ixmp after installing all packages listed in doc/requirements.txt, since ixmp itself will be missing. The file only provides all dependencies that would be installed with pip install ixmp[docs] now. @khaeru, do you think it's enough that this file is hidden in doc/ or should we add a comment at the top stating this exact limitation?

Finally, I still have to check and figure out how/why the bulleted lists are not rendering correctly. If this issue exists here as well, as I expect, I might use this PR to add commits with related fixes.

How to review

  • Read the diff and note that the CI checks all pass.
  • Activate build on RTD for this branch and check that the builds are successful (note: formats other than HTML are too resource-expansive and will only be built after a merge)
  • Build the documentation and look at the bullet lists.

PR checklist

  • Continuous integration checks all ✅
  • [ ] Add or expand tests; coverage checks both ✅ Internal, docs-related, only need them to build/render correctly
  • [ ] Add, expand, or update documentation. Only related to config of docs.
  • [ ] Update release notes. Only related to config of docs.

* Migrate Advanced Settings -> Default Settings to config file
* Pin versions of required packages to install .[docs]
@glatterf42 glatterf42 added enh New features & functionality docs Documentation, tutorials, etc. labels Aug 7, 2023
@glatterf42 glatterf42 self-assigned this Aug 7, 2023
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #491 (982cd52) into main (5c55eec) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #491   +/-   ##
=====================================
  Coverage   98.5%   98.5%           
=====================================
  Files         42      42           
  Lines       4506    4506           
=====================================
  Hits        4442    4442           
  Misses        64      64           

@glatterf42
Copy link
Member Author

I made the same mistake here as I did for #483: since this PR originates from my fork, I can't easily enable RTD builds for it. The same holds for iiasa/message_ix#733. Luckily, though, I don't (yet) use forks for message-ix-models and message_data, so I was able to build the docs for their PRs:

Both of them are looking good, in particular the bullet lists. Thus, I would trust the changes proposed here and for message_ix to have the same effect as they are essentially the same.

Note, though, that I wasn't able to render bullet lists correctly locally, despite updating all packages that I understood to be a dependency of sphinx to their versions suggested in doc/requirements.txt (this is for the message-ix-model build; the message_data one failed but see its PR for further details). I can try updating all packages or this issue might be related to my local css stylesheets being out-of-date, but that is an issue for another day.

@glatterf42 glatterf42 requested a review from khaeru August 7, 2023 16:00
@khaeru
Copy link
Member

khaeru commented Aug 7, 2023

Thanks for all the details here. Without addressing all of them yet, one operational question comes up based on the tip in the RTD page on "Reproducible Builds" page:

Remember to update your docs’ dependencies from time to time to get new improvements and fixes. It also makes it easy to manage in case a version reaches its end of support date.

Namely: how will we know when we need to do this, and how can we minimize the friction involved? In the current process, we don't have to think about it at all or do anything (of course that leads to things like bullet lists mysteriously breaking without our notice, which it will be good to prevent).

I'm thinking that if we can automate the steps including "Delete the lines following lines from the file manually" (perhaps with a simple script that lives in doc/), we could automatically generate an updated version (for instance here) and then see when we need/want to commit it.

@glatterf42
Copy link
Member Author

We might not have to find a way to exclude these lines automatically. If we add a workflow step to create doc/requirements.txt on each run, the runner will have access to the local copy of the package that pip-compile will write into the file. However, it took a few minutes locally (<5) to create the files, so it might be good to avoid this; I will therefore check again to see if the lines for the package itself can be excluded easily.

@glatterf42
Copy link
Member Author

I've gathered several strings of information:

Why does doc/requirements.txt include the package itself?

According to their docs, pip-compile judges based on project.dependencies and project.optional-dependencies when called on a pyproject.toml file. E.g. for message-ix-models, these sections only contain a single reference to message-ix-model itself, namely line 58: "message_ix_models[tests]",. These nested dependencies cause the project to consider itself a dependency of itself. There doesn't seem to be any elegant way to mitigate this; maybe poetry could, but I'd have to check; simply stating all dependencies explicitly will cause noise. So there might not be an easy way to exclude the target lines from a pip-compile-created doc/requirements.txt automatically.

What about Poetry?

Tools like poetry might be able to control the dependencies in an elegant and modern way, but I'd have to look into it more. This might be worthwhile, though, as our stack will at least partly employ poetry with the new ixmp4. Is this something we might be interested in for the rest of the stack as well?

How can we truly enable PR builds for RTD?

What I activated for iiasa/message-ix-models#114 and iiasa/message_data#466 should probably not be called a 'PR build'. The RTD docs specify how to enable builds for PRs and this is not what I have done. I have activated builds for the branch fix/docs, but the project settings are still such that we don't build the html docs for all PRs. I'm also not sure we would want something like this, but I wanted to leave a note here. It should theoretically be possible to activate this option for a project and push a commit to a branch to trigger a docs-build for this PR.

How can we keep pinned dependencies up-to-date?

Last but not least, how do we want to keep the pinned packages up to date? RTD suggests

You do still need to upgrade your dependencies from time to time, but you should do that on your own schedule.

There are services to get notifications on new releases, but generally speaking, that does not seem to be considered best practice. It creates a lot of manual work and can be automated instead. Some people suggest using renovate, which is available on the GitHub marketplace and should be free, but it belongs to mend, which is not free in general, so I'm not entirely convinced. I could only install Renovate for myself or the whole IIASA organization, which don't seem like the best scopes here.
One alternative that is integrated with GitHub is Dependabot. Dependabot is available for all repositories, but not necessarily all package management systems. All pip, pip-compile, and poetry are not supported by Dependabot on private repositories. It might suffice, however, to update the dependencies of message_data in sync with e.g. message-ix-models, though this would still need to be done manually. The latest version of pip that is supported is 21.1, while the latest one is 23.2, which might be relevant since pip only supports recursive (or nested, see above) dependencies since version 21.2. However, since we already have a requirements-file, we might not need to resolve the recursive dependencies. Apart from that, Dependabot allows to specify a dependency file that is regularly checked for possible upgrades. If some viable new combination is found, the bot creates a new PR, but a frequency can be specified to do so only daily/weekly/monthly. This might keep the extra workload manageable.

@glatterf42
Copy link
Member Author

As discussed earlier with @khaeru, I have added doc/requirements.in files to some of these PRs (all but this one, as it happens), in which I specified the package versions of the direct dependencies in the [docs] groups that are necessary to build the documentation. I also activated PR builds for message_ix temporarily and the (private) build can be found here: the bullet lists are looking good again :)

One other thing I noted: comparing the latest version of the message_data docs to the stable one, the bullet lists look fine on both (to my surprise) and a note on the top has gone missing. This is fixed by the fix/docs build again, but made me wonder: should we set the default version of our docs from 'stable' to 'latest'? The latest versions seem to be more reliable and accurate (cf the installation instructions for message_ix including the step to setup the mamba solver for anaconda).
If I understand this correctly, the 'stable' version is built whenever something is pushed to a tag with a version number bigger than the one currently pointed to by 'stable'. For some reason, this includes PR builds (maybe it identifies the message_ix PR's 'version' as 733), so PR builds would be a way to update it. Other than that, only new tags will update 'stable', whereas each push to 'main' updates 'latest'.
So if we publish tags/versions twice a year and introduce some changes to the docs soon after the last release (as was the case with the mamba solver), using 'stable' as our default version would mean manually telling people to use the updated 'latest' for roughly six months.

* Update doc/requirements.txt accordingly
* Install with [docs] again via RTD config
@glatterf42
Copy link
Member Author

Due to the success on the other PRs, this PR receives the same changes now. The steps for arriving at the current doc/requirements.in and doc/requirements.txt are as follows:
Based on the existing doc/requirements.txt and pyproject.toml, I picked the versions of the dependencies needed for buildings the docs and wrote them to doc/requirements.in. The command to update doc/requirements.txt is then pip-compile --allow-unsafe -o doc/requirements.txt doc/requirements.in.

@glatterf42
Copy link
Member Author

The pinned dependencies need to be updated regularly, though not necessarily frequently. Since this topic is relevant across all four repositories, I decided to add information on why and how to the ECE wiki rather than any individual repo.
In essence, we might start with manual updates in about four months, and if that seems to cumbersome in the long run, we can look into dependabot or some custom action instead.

Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

Using the requirements.in pattern, the requirements.txt now seems to have a smaller set of packages more closely focused on those actually run in the Sphinx build.

LGTM!

@khaeru khaeru merged commit 4274880 into iiasa:main Aug 9, 2023
19 checks passed
@glatterf42 glatterf42 deleted the fix/docs branch August 9, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation, tutorials, etc. enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants