Update RTD config

[glatterf42]

Replaces #483 to enable testing if the docs are still built successfully by RTD.

How to review

• Read the diff and note that the CI checks all pass.
• Build the documentation and check that it works as usual (also on RTD).

PR checklist

• Continuous integration checks all ✅
• Add or expand tests; coverage checks both ✅
• Add, expand, or update documentation. N/A; CI changes only.
• Update release notes. N/A

[codecov]

Codecov Report

Merging #485 (e60910f) into main (f348a6b) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #485   +/-   ##
=====================================
  Coverage   98.5%   98.5%           
=====================================
  Files         42      42           
  Lines       4489    4489           
=====================================
  Hits        4425    4425           
  Misses        64      64           

[glatterf42]

The build on RTD is failing with a note that the pytest module is not available. Each module should presumably only appear in pyproject.toml once, so it is not an option to add pytest to [docs]. Simply switching to [tests] is the status quo, adding [tests] to [docs] creates a circular import and seems weird: why do the docs need pytest?
Technically, this happens because in conf.py, ixmp.testing is imported, based on its commit history so that 'autodoc can find code'. I'm wondering what happens when this line is removed.

[glatterf42]

The build on RTD is successful now and looks good to me: https://docs.messageix.org/projects/ixmp/en/update-rtd-config/

[khaeru]

Note what appears in this section in the docs built from main: https://docs.messageix.org/projects/ixmp/en/latest/api.html#module-ixmp.testing —on the current branch, with ixmp.testing not available to Sphinx, the documentation for this part of the API is missing.

I will try to come back to this soon and suggest a fix.

[glatterf42]

I see, I was wondering if there were special parts like this that needed the import line. So I've tried to take a closer look at this: there are ways to recursively find and document all submodules of a package, primarily through the :recursive: option of the autosummary extension. See also this long answer on how to use it. Unfortunately, it seems to me that you can only use it to detect all modules and them have them all summarized, contrary to our structure in api.rst of presenting some submodule, adding some text, and presenting the next submodule.
Within api.rst, there are two mentions of ixmp.testing. One as the :currentmodule:, which should not influence the build process. The other is as part of :autodoc:, which does issue a warning that all modules to be documented are imported as well. This leads to our problem since testing/__init__.py includes several pytest hooks that require pytest to be imported.
The cleanest solution, then, seems to be to move these pytest hooks to another location so that import pytest is no longer required in ixmp.testing. For that, it would be useful to understand: why do we have a testing and a tests directory? And what depends on the pytest hooks being in testing (also cf message_ix, where there are no pytest fixtures in testing/__init__.py)?
While this option does seem particularly clean to me, it could also imply scope creep and might take longer than adding some line to the sphinx config.

[khaeru]

For that, it would be useful to understand: why do we have a testing and a tests directory? And what depends on the pytest hooks being in testing (also cf message_ix, where there are no pytest fixtures in testing/__init__.py)?

You're on the right track here.

• ixmp/tests/ contains the actual tests.
• ixmp/testing/ contains, as the docs section implies, utilities for testing. In other words, these are fixtures, functions, hook implementations, other tools, etc. that help with writing tests that are simple and clear. Some of these are used in downstream packages, so must be importable.

Having a .testing submodule is a practice that emulates high-quality upstream packages: numpy, pandas, xarray, etc.

Note that conftest.py names ixmp.testing:

ixmp/conftest.py

Line 1 in f348a6b

pytest_plugins = ("genno.testing", "ixmp.testing")

…and so do the corresponding files in message_ix, message-ix-models, etc.

These mean that:

• All of the pytest hooks defined by those modules are loaded, and
• All of the pytest fixtures defined by those modules are available.

In addition, downstream code can import and use the utility functions as it likes.

Anyway, the upshot of all of this is: ixmp.testing and its contents should be documented for reference by users and developers of both ixmp and the downstream packages which depend on it. This is why it has its section in the documentation.

As for a fix, I think the simplest is to keep .readthedocs.yaml as it was, and add a comment:

install:
- method: pip
  path: .
  # NB we use "tests" here, not "docs", so that ixmp.testing can be imported and its
  # contents documented. Users may build the docs with only ``ixmp[docs]`` installed,
  # but that section will appear empty.
  extra_requirements: [tests]

Or, second simplest, change the dependencies in pyproject.toml:

docs = [
  "ixmp[tests]",
  "GitPython",
  "numpydoc",
  "sphinx >= 3.0",
  "sphinx_rtd_theme",
  "sphinxcontrib-bibtex",
]
report = ["genno[compat,graphviz]"]
tutorial = ["jupyter"]
tests = [
  "ixmp[report,tutorial]",
  "memory_profiler",
  "nbclient >= 0.5",
  "pretenders >= 1.4.4",
  "pytest >= 5",
  "pytest-benchmark",
  "pytest-cov",
]

[glatterf42]

Thanks for the explanation. Of these, the second option seems cleaner to me. I wasn't sure if the tests would still run when only [tests] is installed, but it seems to work (there are two errors, but they don't look related to something missing from [docs]). So I'll push a commit here and we'll see if the tests pass and if the docs can be built on RTD.

[glatterf42]

All tests fail because in the workflow file, we only install [tests], where I have now removed the dependency on [docs]. But changing the workflow file to install [docs] should fix this and this way might be preferable since the imports on the user-facing side work as expected.

[glatterf42]

The tests are passing again and RTD also builds the docs without complaint, but the resulting docs still don't show the missing section. I'm not too sure what's going on, especially since for ixmp.utils, the same does work and I can't quickly find a major difference between the two submodules with respect to building the docs, so I don't know why we should have to import ixmp.testing explicitly in conf.py, but not ixmp.utils.

[glatterf42]

When I delete everything in doc/_build locally, checkout this branch and run sphinx-build -T -E -d _build/doctrees-readthedocs -D language=en . _build/html, the docs do include the missing section. I'm wondering whether RTD uses some sort of a cached version of the doc pages since the latest commits only change pyproject.toml and the workflow file.

[glatterf42]

Now they also do on RTD. Maybe when I was checking them out yesterday that was too soon after the prior run so I was still directed to the former version?
Either way, I think this PR is ready to be merged now.

[khaeru]

Thanks, looks good to go!

NB I edited the PR description to add the other items in the PR check-list. It's less mental work for a reviewer to see them struck-out and say, "Ah yes, I agree, this is not needed for this PR" than to say "What was in that list again? Was the PR author right to skip them?" We can change the inline comment if it's not clear enough.

[glatterf42]

Sounds good, thank you :)