Skip to content

Add docs build CI workflow#5418

Open
mithr4ndir wants to merge 2 commits intoyt-project:mainfrom
mithr4ndir:issue/4391/docs-ci-workflow
Open

Add docs build CI workflow#5418
mithr4ndir wants to merge 2 commits intoyt-project:mainfrom
mithr4ndir:issue/4391/docs-ci-workflow

Conversation

@mithr4ndir
Copy link
Copy Markdown

Picks up the stalled work from #4391. The docs site has been down for about a year, and this adds a CI workflow to validate Sphinx builds on PRs and pushes to main.

Changes

  • New workflow: .github/workflows/docs.yaml
  • Triggers on pushes to main and PRs that touch doc/
  • Uses uv sync --group=docs (consistent with existing CI)
  • Uploads built HTML as an artifact for review
  • No linkcheck (per @zingale comment, too many false positives from site blocking)

Fixes from the original PR

  • Branch references: main only (no development)
  • Dependencies: uv sync --group=docs instead of pip install -r ./requirements.txt (file does not exist)
  • Directory: doc/ not docs/
  • Action versions: pinned SHAs matching the rest of the repo
  • Skips cookbook script execution via READTHEDOCS=True to avoid data dependency issues

What this does NOT do

  • Does not deploy to GitHub Pages (separate discussion)
  • Does not run linkcheck
  • Does not change conf.py or any existing files

Test plan

  • Workflow YAML is valid
  • Sphinx HTML build succeeds
  • Built docs artifact is downloadable

Adds a GitHub Actions workflow to build Sphinx documentation on pushes
to main and PRs that touch the doc/ directory. Uses uv sync --group=docs
consistent with existing CI patterns, and uploads the built HTML as an
artifact for review.

Sets READTHEDOCS=True to skip cookbook script execution that requires
data files not available in CI.
@neutrinoceros
Copy link
Copy Markdown
Member

I initially greenlighted this but I'm going to take a step back. The author has been approaching many repos and contributing directly to infrastructure level code. This is sketchy in itself. Furthermore, the appearence of legitimate gh contributions provided in the shape of several repos owned and seemingly developed at a human pace doesn't resist scrutiny, and they all look vibe coded with only two contributors: the owner + Claude.
With the rise of agentic-AI powered security exploits, and given how insecure GitHub Action generally is, I think we should apply extreme caution to this class of PRs, especially from first-time contributors.

For these reasons, I'm rejecting this PR entirely.

@mithr4ndir
Copy link
Copy Markdown
Author

I appreciate the caution, it's a valid concern. I'm contributing to infrastructure because that's been my career for the last 20 years. I do use AI tools to help with my workflow, but I review and own everything I submit. Here's my LinkedIn if it helps: https://www.linkedin.com/in/chris-ladino-93654254/

@mithr4ndir
Copy link
Copy Markdown
Author

I genuinely want to give back to this community. I work hard and fast, so you'll definitely see more of me contributing to science and astronomy projects.

Copy link
Copy Markdown
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

okay, maybe I was a little too paranoid. Here's a review with everything I don't fully get with this PR, which also contributed to my suspicion.

Comment thread .github/workflows/docs.yaml
Comment thread .github/workflows/docs.yaml Outdated
- name: Set up uv
uses: astral-sh/setup-uv@681c641aba71e4a1c380be3ab5e12ad51f415867 # v7.1.6
with:
version: ">=0.9.11,<0.10.0"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we don't pin uv anywhere else on CI, so I'd just remove this for consistency

Suggested change
version: ">=0.9.11,<0.10.0"

Copy link
Copy Markdown
Author

@mithr4ndir mithr4ndir Apr 7, 2026

Choose a reason for hiding this comment

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

Sure, I'll remove it. I matched it from https://github.com/yt-project/yt/blob/main/.github/workflows/build-test.yaml#L33 but happy to drop it here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh, weird. I don't remember writing this and don't really get why I would now. Thanks for pointing it out. I think we should remove the pin everywhere (but that other file is out of scope here)

Comment thread .github/workflows/docs.yaml Outdated
with:
version: ">=0.9.11,<0.10.0"
python-version: "3.12"
prune-cache: false
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd rather we disable caching here for now.

Suggested change
prune-cache: false
enable-cache: false

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Makes sense, I'll apply your suggestion.

Comment on lines +26 to +27
env:
READTHEDOCS: "True"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this reads like we're trying to impressionate RTD. Is there a reason for doing this that I'm missing ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not trying to impersonate RTD. The https://github.com/yt-project/yt/blob/main/doc/Makefile#L49 checks that env var and skips sphinx-apidoc, which generates API stubs for every yt module. Those stubs cause autodoc to import the full package at build time, which can fail if compiled extensions or data aren't available. https://github.com/yt-project/yt/blob/main/doc/source/conf.py#L46 also uses it to skip autosummary and pythonscript_sphinxext (which executes inline Python scripts in the docs). It's saves time, and its avoiding build failures from operations that need a fully configured yt environment. Happy to use a different env var if you'd prefer

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for the detailed explanation, I really had no idea this was the state of our setup

- name: Build HTML docs
run: |
cd doc
uv run make html
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't look right. Where is that make python entry point supposed to come from ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

make is preinstalled on Ubuntu runners. The Makefile sets SPHINXBUILD = sphinx-build, so it needs sphinx-build on PATH. uv run handles that by prepending the venv's bin/ directory to PATH before executing the command, and child processes inherit it (https://docs.astral.sh/uv/concepts/projects/run/). Since we're setting READTHEDOCS=True (which skips sphinx-apidoc in the Makefile), we could also just call uv run sphinx-build -b html -d doc/build/doctrees doc/source doc/build/html directly if you'd prefer to skip make.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The given command can be provided by the project environment or exist outside

oh, I was not aware uv run did this. It makes total sense now

@Xarthisius
Copy link
Copy Markdown
Member

For the record, in case someone wants to revive old docs build, here are some notes:

  1. The most difficult aspect of building docs on free infra is the fact it requires a lot of extra data (https://yt-project.org/data/) and a lot of CPU/RAM to build in reasonable time.
  2. Most resource consuming part is evaluating cookbook (see https://github.com/yt-project/yt/blob/main/doc/helper_scripts/run_recipes.py)
  3. There are system level dependencies (apart from already identified pandoc). This is a Dockerfile for env I've used in the past:
FROM ubuntu:24.04

RUN apt-get update && apt-get install -y \
    python3 \
    python3-gdbm \
    python3-pip \
    python3-dev \
    python3-venv \
    build-essential \
    libssl-dev \
    libffi-dev \
    python3-setuptools \
    python3-venv \
    git \
    curl \
    wget \
    libmpich-dev \
    openjdk-18-jre-headless \
    ffmpeg \
    pandoc \
    rsync \
    && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

RUN usermod -l fido ubuntu
  1. This is the script that's been running on jekins:
rm -rf .venv
python3.11 -m venv .venv
. .venv/bin/activate
pip install -U pip setuptools

rm -rf /tmp/openmpi-sessions-fido* /tmp/tmp* $HOME/.cache/matplotlib

export PYTHONPATH=$PWD
export YT_BUILD=$PWD
export GPERFTOOLS=yes

sed -e 's/"scipy<=1.7.1".*$/"scipy",/' -i pyproject.toml
sed -i -e '/test_data_dir/ s:/does/not/exist:/mnt/yt:g' \
    -e '/xray_data_dir/ s:/does/not/exist:/mnt/yt/xray_data:g' \
    -e '/supp_data_dir/ s:/does/not/exist:/mnt/yt/supp_data:g' \
    -e '/suppress_stream_logging/ s/False/True/g' yt/config.py
sed -i -e '/^where=/d' *.cfg
sed -e '/#html_last_updated_fmt/ s/#//' -i doc/source/conf.py
sed -i -e '/^SPHINXBUILD/ s/sphinx-build/& -j 12/g' doc/Makefile
sed -e '/download_datasets =/ s/True/False/' \
    -i doc/source/quickstart/1\)_Introduction.ipynb

echo "/usr/local/src/rockstar" > rockstar.cfg
export CFLAGS="-Wno-cpp -fno-strict-aliasing -O3 -march=native -pipe"
python -m pip install -e .[test,full,doc]
# cookbook
cd doc
rm -rf source/visualizing/colormaps/_static
rm -rf source/cookbook/_static
python helper_scripts/run_recipes.py

# we need entry points
cd ..
export PYTHONPATH=$PWD:$PWD/temp/lib/python3.11/site-packages
mkdir -p $PWD/temp/lib/python3.11/site-packages
export PATH=$PWD/temp/bin:$HOME/.local/bin:$PATH
[[ -f requirements/docs.txt ]] && pip install -r requirements/docs.txt
python -m pip install .[test,full,doc] --prefix=$PWD/temp
python -m pip install nose nose-exclude
find .venv/lib/python*/site-packages/nose -name "*.py" -exec sed -i -e 's/collections.Callable/collections.abc.Callable/g' {} \;

# build docs
export PYDEVD_DISABLE_FILE_VALIDATION=1
cd doc
make html

# Allow to download .py files from the web
cat <<-EOF > build/html/.htaccess
Options +ExecCGI +FollowSymLinks +Indexes
AddHandler default-handler .py
EOF

echo "Uploading to server"
rsync -au build/html/ --delete <target_server>

@mithr4ndir
Copy link
Copy Markdown
Author

Thanks @Xarthisius, this is really useful context. My workflow intentionally sets READTHEDOCS=True to skip the cookbook evaluation and sphinx-apidoc steps, which keeps the CI build lightweight and avoids needing the test data. The goal for now is just to validate that the core Sphinx HTML build doesn't break on PRs.

A fuller build that runs cookbook recipes and evaluates notebooks could be a separate workflow down the road, potentially using a self-hosted runner with the test data pre-staged. I have a 3-node Kubernetes home lab with dedicated resources and a full monitoring stack that I'd be happy to offer for that. Let me know if there's interest.

@neutrinoceros
Copy link
Copy Markdown
Member

ok this looks much less suspicious to me now we had a couple back and forth. Sorry about my response earlier, I'm really on the fence these days.

@neutrinoceros neutrinoceros reopened this Apr 9, 2026
@neutrinoceros neutrinoceros added docs infrastructure Related to CI, versioning, websites, organizational issues, etc labels Apr 9, 2026
@mithr4ndir
Copy link
Copy Markdown
Author

No worries, you have every right to be skeptical, especially for things that you are passionate about, and with AI usage on the rise.

Copy link
Copy Markdown
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

quick comment, thanks for working on this!

Comment thread .github/workflows/docs.yaml
@mithr4ndir mithr4ndir force-pushed the issue/4391/docs-ci-workflow branch 2 times, most recently from e787e30 to e71f8df Compare April 10, 2026 14:31
@mithr4ndir mithr4ndir force-pushed the issue/4391/docs-ci-workflow branch from e71f8df to e9af7a4 Compare April 10, 2026 14:32
@chrishavlin
Copy link
Copy Markdown
Contributor

chrishavlin commented Apr 10, 2026

https://github.com/yt-project/yt/actions/runs/24248090733/job/70807840760?pr=5418 👀

(it's running! let's see how it does)

@mithr4ndir
Copy link
Copy Markdown
Author

mithr4ndir commented Apr 10, 2026

https://github.com/yt-project/yt/actions/runs/24248090733/job/70807840760?pr=5418 👀

(it's running! let's see how it does)

Very cool, it passsed :) - build took ~5.5 minutes with the actual Sphinx step at ~2.5 minutes

Copy link
Copy Markdown
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this!

I checked out the html artifact, and it was as expected given the limitations on data.

It is too bad that the READTHEDOCS env var ties the autodoc to inline script execution... would be nice to separate that out and be able to build the API docs without any code execution. But that's beyond the scope of this PR IMO, just having the docs building again at all in CI is helpful and a great start.

Also will note that it's a good thing uv sync --group=docs doesn't give us pooch, otherwise load_sample woulda been pulling in a ton of data :)

@chrishavlin chrishavlin added this to the 4.5.0 milestone Apr 10, 2026
@mithr4ndir
Copy link
Copy Markdown
Author

Thanks for the work on this!

I checked out the html artifact, and it was as expected given the limitations on data.

It is too bad that the READTHEDOCS env var ties the autodoc to inline script execution... would be nice to separate that out and be able to build the API docs without any code execution. But that's beyond the scope of this PR IMO, just having the docs building again at all in CI is helpful and a great start.

Also will note that it's a good thing uv sync --group=docs doesn't give us pooch, otherwise load_sample woulda been pulling in a ton of data :)

Its my pleasure to help!

Great point about decoupling the autodoc from the inline script execution. I did some local testing and it looks feasible. The key insight is that uv sync --group=docs installs yt in a way that makes it fully importable, so sphinx-apidoc and autosummary can generate API stubs and read docstrings without needing to execute any code.

The change would be small. In conf.py, split the on_rtd gate into two:

  • autosummary stays gated on on_rtd (so RTD behavior is unchanged)
  • pythonscript_sphinxext gets its own flag (YT_DOCS_SKIP_EXECUTION)

And in the Makefile, the sphinx-apidoc call would use the new flag instead of READTHEDOCS.

That way CI builds can generate API reference docs without running cookbook scripts or pulling test data. Happy to put together a follow up PR for this if there's interest.

@chrishavlin
Copy link
Copy Markdown
Contributor

The change would be small. In conf.py, split the on_rtd gate into two:

autosummary stays gated on on_rtd (so RTD behavior is unchanged)
pythonscript_sphinxext gets its own flag (YT_DOCS_SKIP_EXECUTION)

And in the Makefile, the sphinx-apidoc call would use the new flag instead of READTHEDOCS.

@mithr4ndir generally that sounds great to me! My only slight worry is backwards compatibility... totally possible folks are out there building yt docs manually (@zingale maybe I'm not remembering right, but don't you have a yt docs build somewhere?). And I'm forever confused by the on_rtd and READTHEDOCS variable names (I always have to check the code to remember what changes when in READTHEDOCS mode...), so I'd suggest:

Add two new environment variables

  • YT_DOCS_EXECUTE_NOTEBOOKS : inverse of your YT_DOCS_SKIP_EXECUTION: if True, include pythonscript_sphinxext extension (flipping the sense here because of my personal preference to have flags that indicate the action to take instead an action to skip)
  • YT_DOCS_RUN_API_AUTOSUMMARY : if True, include and run the autosummary

Then keep the READTHEDOCS variable: if detected, emit a deprecation warning with suggestion to use the new variables and then set the code execution and autosummary flags to be consistent with prior READTHEDOCS behavior (if READTHEDOCS is False... YT_DOCS_EXECUTE_NOTEBOOKS and YT_DOCS_RUN_API_AUTOSUMMARY should both be True).

All the above sounds like enough to warrant a separate PR... but maybe I'm overly worried about backwards compatibility here though? @Xarthisius @matthewturk @neutrinoceros want to weigh in?

@zingale
Copy link
Copy Markdown
Member

zingale commented Apr 14, 2026

I sometimes build the docs locally when debugging, but I don't maintain my own build publicly.

@mithr4ndir
Copy link
Copy Markdown
Author

Thanks @chrishavlin, that's a much cleaner design. Inverting the flag sense (action to take, not action to skip) is the right call, and the explicit names YT_DOCS_EXECUTE_NOTEBOOKS and YT_DOCS_RUN_API_AUTOSUMMARY are easier to reason about than the current READTHEDOCS overload.

I'm on board with the backward-compat approach. Keeping READTHEDOCS recognized but mirroring it onto the two new flags with a DeprecationWarning should keep existing local builds working without surprises, and per @zingale's note the practical surface area there looks small.

Happy to put together the follow-up PR. Rough plan:

  • Parse the two new env vars once at the top of conf.py and use the booleans throughout.
  • Wire pythonscript_sphinxext to YT_DOCS_EXECUTE_NOTEBOOKS, and the autosummary block to YT_DOCS_RUN_API_AUTOSUMMARY.
  • When READTHEDOCS=True is set without the new flags, mirror the prior on_rtd behavior (both new flags False) and emit a warning pointing at the replacements; default (READTHEDOCS unset or False) is both new flags True.
  • Switch the Makefile's sphinx-apidoc invocation from READTHEDOCS to YT_DOCS_RUN_API_AUTOSUMMARY.
  • Document the new flags wherever the existing READTHEDOCS guidance lives, with a short deprecation note.

If that scope sounds right, would it make sense to merge this PR first so the follow-up lands on a stable base? It's approved and CI is green, and the follow-up shouldn't depend on anything that would change in this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs infrastructure Related to CI, versioning, websites, organizational issues, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants