scikit-hep / cabinetry

design and steer profile likelihood fits
https://cabinetry.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
27 stars 21 forks source link

chore: update ReadTheDocs config file to be compliant #439

Closed matthewfeickert closed 1 year ago

matthewfeickert commented 1 year ago

@alexander-held not sure if this is going to fix your issues, but this is basically the pyhf RTD config that has been working fine so far.

matthewfeickert commented 1 year ago

Also renamed as

Read the Docs supports configuring your documentation builds with a configuration file. This file is named .readthedocs.yaml and should be placed in the top level of your Git repository.

matthewfeickert commented 1 year ago

@alexander-held There's no RTD test build happening at the moment. Do you have it configured to not run on PRs?

codecov[bot] commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (2af877c) 100.00% compared to head (aa5ee0d) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #439 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 22 22 Lines 2082 2082 Branches 340 340 ========================================= Hits 2082 2082 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alexander-held commented 1 year ago

@alexander-held There's no RTD test build happening at the moment. Do you have it configured to not run on PRs?

Yes, so far it was just running on master as there was never a need for branches (local compilation has been working fine for testing). I now switched the setup temporarily to build from this branch, but still see the build failing.

These are the commands I see running:

git clone --depth 1 https://github.com/scikit-hep/cabinetry.git .
git fetch origin --force --prune --prune-tags --depth 50 refs/heads/chore/update-RTD-config:refs/remotes/origin/chore/update-RTD-config
git checkout --force origin/chore/update-RTD-config
git clean -d -f -f
cat .readthedocs.yaml
apt-get update --assume-yes --quiet
apt-get install --assume-yes --quiet -- curl jq
asdf global python 3.11.4
python -mvirtualenv $READTHEDOCS_VIRTUALENV_PATH
python -m pip install --upgrade --no-cache-dir pip setuptools
python -m pip install --upgrade --no-cache-dir pillow mock==1.0.1 alabaster>=0.7,<0.8,!=0.7.5 commonmark==0.9.1 recommonmark==0.5.0 sphinx<2 sphinx-rtd-theme<0.5 readthedocs-sphinx-ext<2.3 jinja2<3.1.0
python -m pip install --upgrade --upgrade-strategy only-if-needed --no-cache-dir .[docs,contrib]
cat docs/conf.py
python -m sphinx -T -E -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html

The thing I do not understand here is what triggers this line

python -m pip install --upgrade --no-cache-dir pillow mock==1.0.1 alabaster>=0.7,<0.8,!=0.7.5 commonmark==0.9.1 recommonmark==0.5.0 sphinx<2 sphinx-rtd-theme<0.5 readthedocs-sphinx-ext<2.3 jinja2<3.1.0

This installs sphinx-rtd-theme<0.5 which does not subsequently get updated, as the dependency listed in cabinetry has no lower bound on this: https://github.com/scikit-hep/cabinetry/blob/2af877c58052c335572fcea4e604d49f32804e0a/setup.py#L27-L31

I imagine this could be solved by adding a lower bound, I'll push a commit to this branch here to try it out.

It is very unclear to me why this one step happens with the upper bounds set though (which is what ultimately causes the problem). Maybe @henryiii might have an idea?

alexander-held commented 1 year ago

I can confirm that the build passes again with the changes from aa5ee0d. It feels to me that there should be a better solution that addresses what happens in the build in that extra step. Unless anyone has other ideas, I think this is good enough to go ahead with though.

matthewfeickert commented 1 year ago

I agree that's strange, but I also agree that this is probably not worth the effort to debug further.

henryiii commented 1 year ago

RtD pre-installs old versions of things, like the old rtd theme. Setting a minimum forces the upgrade.