Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 105 additions & 0 deletions .github/workflows/actions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ run-name: SurPyval CI
# master (i.e. release merges + tags). Feature work accumulates on `develop`
# via PRs; day-to-day pushes to feature branches no longer each trigger the
# full suite. See docs/Contributing.rst for the branching / release flow.
#
# The jobs below are not all gated the same way. Lint runs on everything;
# the test suite and the documentation build run only on the release pull
# request into `master`, where the time they cost is worth paying. Each
# job carries the reasoning for its own condition.
on:
pull_request:
branches: [develop, master]
Expand Down Expand Up @@ -40,7 +45,26 @@ jobs:
- name: black
run: black --check $SRC

# The test suite runs on the release pull request into `master`, and on
# pushes to `master`, but not on pull requests into `develop`.
#
# It is roughly nine minutes across the three interpreters, against
# about one for lint, and paying that on every feature pull request
# made the edit-review loop the slowest part of working on the
# package. Development is single-maintainer and the suite is run
# locally before pushing, so the pull-request run was mostly
# confirming what was already known.
#
# What that gives up is real, and worth naming: a failure that only
# appears on one interpreter is now found when the release is
# prepared, with a release's worth of commits to search rather than
# one. That is not hypothetical -- the doctest comparison landed
# green on 3.11 and broke on 3.12 and 3.13, and it was the
# pull-request run that caught it. Run the suite locally across more
# than one interpreter when touching numerics, or open the release
# pull request early and let it sit.
surpyval_ci:
if: github.event_name != 'pull_request' || github.base_ref == 'master'
runs-on: ubuntu-latest
strategy:
fail-fast: false
Expand Down Expand Up @@ -82,6 +106,36 @@ jobs:
python -m pytest -n auto --cov=surpyval --cov-report=
--ignore=surpyval/tests/alpha --run-ml

# Execute the ``>>>`` examples in the docstrings and compare their
# printed output. This is what a user (or an agent) sees from
# ``help(Weibull.fit)``, so it is a documented promise like any
# other, and it went stale silently until it was checked. Kept as
# its own step -- separate from the suite above -- so a failure
# reads as "the docs drifted", not "a test broke", and so the
# doctest collection does not disturb coverage or xdist.
#
# ``surpyval/tests`` is excluded because the test modules have no
# user-facing examples, and ``surpyval/alpha`` because it is not
# part of the release contract (same reason the suite skips it).
# Option flags are set in pyproject.toml; conftest.py adds the
# numeric comparison that lets the examples record real output.
- name: doctests
run:
python -m pytest --doctest-modules surpyval
--ignore=surpyval/tests --ignore=surpyval/alpha

# The numeric comparison above only runs for an example whose
# output has actually drifted -- a handful on any one machine, and
# a different handful on each. That leaves a gap in it invisible
# until CI hits the one example that needed it. This second pass
# routes every example through it, so the fallback is checked
# against all of them. Fifteen seconds.
- name: doctests (numeric comparison forced)
run:
python -m pytest --doctest-modules surpyval
--ignore=surpyval/tests --ignore=surpyval/alpha
--doctest-force-numeric

- name: coverage
run: |
coverage report
Expand All @@ -93,3 +147,54 @@ jobs:
with:
name: coverage-html-report
path: htmlcov/

# Build the documentation on the release pull request only.
#
# The docs execute every ``.. jupyter-execute::`` cell during the
# build, so they are a second test suite that runs the public API for
# real -- and one that can be broken by a change that touches no
# documentation file at all. Removing Gamma's probability-plot fitting
# did exactly that: a cell looping over the fit methods started
# raising, and nothing noticed, because Read the Docs builds only
# ``master`` and tags. The failure would have appeared as a broken
# hosted build after the release rather than on the change that caused
# it.
#
# ``github.base_ref`` is set only for pull_request events, so this
# runs on the develop -> master release pull request and nowhere else.
# It is deliberately not run on pushes to master: Read the Docs
# rebuilds there anyway, and by that point the gate has nothing left
# to gate.
#
# The point is to reproduce the hosted build, so this matches
# .readthedocs.yaml rather than the test jobs above: Python 3.12, and
# the package installed with its own ``docs`` extra.
docs:
if: github.event_name == 'pull_request' && github.base_ref == 'master'
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- name: Set-up Python
uses: actions/setup-python@v5
with:
python-version: '3.12'

- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install ".[docs]"

# Not -W: there are pre-existing warnings (duplicate changelog
# labels from autosectionlabel, an rtd-theme deprecation, three
# autodoc imports). Clearing those is worth doing, and then
# failing on warning here and in .readthedocs.yaml together --
# turning it on before then would just fail every release.
- name: sphinx build
run: python -m sphinx -b html docs docs/_build/html

- name: Upload documentation artifact
uses: actions/upload-artifact@v4
with:
name: docs-html
path: docs/_build/html/
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,6 @@ __pycache__/


publish.sh

# Local multi-interpreter check environments (scripts/check_all_pythons.py)
.venvs/
3 changes: 2 additions & 1 deletion .readthedocs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,5 @@ python:
install:
- method: pip
path: .
- requirements: docs/requirements.txt
extra_requirements:
- docs
139 changes: 138 additions & 1 deletion conftest.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
"""Opt-in gating for the slow parts of the test suite.
"""Suite-wide fixtures: doctest number comparison, and opt-in gating.

The first half of this file makes the ``--doctest-modules`` run compare
the numbers in an example's output as numbers rather than as text; see
the comment above ``RTOL``. The rest is the opt-in gating below.

Two groups are skipped unless asked for, because both are expensive and
neither guards a regression that the default run would miss quickly:
Expand Down Expand Up @@ -31,8 +35,127 @@
the run dies on "unrecognized arguments".
"""

import doctest
import math
import re

import pytest

# ---------------------------------------------------------------------------
# Numeric comparison for the ``--doctest-modules`` run
# ---------------------------------------------------------------------------
# doctest compares printed output as text. That is the wrong test for a
# library whose examples end in an optimiser: the same fit lands on
# ``b = 4.1995e-05`` under one Python and ``4.2032e-05`` under the next,
# and numpy prints eight significant digits either way, so a byte-exact
# comparison fails on a difference no reader would call a difference.
#
# The alternative -- trimming every documented number to the digits that
# happen to agree everywhere -- makes the docstring show something the
# user's own session will not produce, which is the thing these examples
# exist to avoid. So the examples record the real output, in full, and
# the numbers in it are compared as numbers.
#
# The fallback only runs after the ordinary text comparison has failed,
# and only fires when the two outputs are identical apart from their
# numeric literals -- same words, same brackets, same integer-vs-float
# shape ("1" never matches "1.", which is a dtype change worth
# failing on). What it forgives is the value drifting inside a
# tolerance. What it still catches is everything that actually went
# wrong when this was first switched on: a stale value from another
# parameterisation, a different function being called, the wrong array
# shape, an exception, a missing import.
#
# RTOL is set by the loosest genuine disagreement between supported
# Pythons -- the Duane example above, at 9e-4 -- with no margin beyond
# that. ATOL exists for the one other case, a restoration factor whose
# true value is zero and which surfaces as 1e-16 with whatever sign and
# mantissa the optimiser stopped on; relative tolerance is meaningless
# there.
RTOL = 1e-3
ATOL = 1e-12

_NUMBER = re.compile(r"[-+]?(?:\d+\.\d*|\.\d+|\d+)(?:[eE][-+]?\d+)?")
_BLANKLINE = re.compile(r"(?m)^%s\s*?$" % re.escape(doctest.BLANKLINE_MARKER))


def _skeleton(text: str) -> str:
"""The text with each number replaced by its *kind*.

Integers and floats get different placeholders so that a change in
dtype -- ``array([1, 2])`` becoming ``array([1., 2.])`` -- is still
a failure rather than two numbers that happen to be equal.

Whitespace is dropped entirely. numpy pads an array's columns to its
widest element, so shortening one number moves the spaces around
every other: ``[ 6.32508961 17.37701969]`` against
``[ 6.3250866 17.377018 ]``. Those spaces carry no meaning the
numeric comparison below has not already made.
"""

def mark(match: re.Match) -> str:
token = match.group(0)
return "~f" if ("." in token or "e" in token or "E" in token) else "~i"

return "".join(_NUMBER.sub(mark, text).split())


def _numerically_equal(want: str, got: str) -> bool:
# ``<BLANKLINE>`` stands for an empty line in the expected output.
# The text comparison substitutes it before matching, so this one has
# to as well, or a model repr with a blank line in it can never reach
# the numeric comparison at all.
want = _BLANKLINE.sub("", want)

if _skeleton(want) != _skeleton(got):
return False
wants = _NUMBER.findall(want)
gots = _NUMBER.findall(got)
if not wants or len(wants) != len(gots):
return False
return all(
math.isclose(float(w), float(g), rel_tol=RTOL, abs_tol=ATOL)
for w, g in zip(wants, gots)
)


_text_check_output = doctest.OutputChecker.check_output


def _check_output(self, want, got, optionflags):
if _text_check_output(self, want, got, optionflags):
return True
return _numerically_equal(want, got)


# Patched on the base class rather than installed as a checker: pytest
# builds its own ``LiteralsOutputChecker`` subclass and calls up to this
# method, so overriding here survives both plain ``doctest`` and pytest,
# and does not depend on pytest's internals.
_patched = _check_output # type: ignore[assignment]
doctest.OutputChecker.check_output = _patched # type: ignore[method-assign]


def _forced_check_output(self, want, got, optionflags):
"""As above, but the numeric path is the *only* path.

The fallback normally runs only when an example's output has
actually drifted, which on any one machine is a handful of them. A
gap in it -- the ``<BLANKLINE>`` markers it did not strip, say --
therefore stays invisible locally and surfaces in CI, on whichever
Python happens to compute a different last digit.

Under ``--doctest-force-numeric`` every example whose output
contains a number is compared numerically instead, so the fallback
is exercised against all 229 of them rather than against today's
accidental few. Outputs with no numbers keep the text comparison;
there is nothing in them for this to compare.
"""
if not _NUMBER.search(want):
return _text_check_output(self, want, got, optionflags)
return _numerically_equal(want, got)


OPT_IN = {
"ml": (
"--run-ml",
Expand All @@ -55,13 +178,27 @@ def pytest_addoption(parser):
default=False,
help=f"run the {description} (skipped by default)",
)
parser.addoption(
"--doctest-force-numeric",
action="store_true",
default=False,
help=(
"compare every doctest example's numbers numerically, not "
"only those whose text has drifted; exercises the fallback "
"against all of them"
),
)


def pytest_configure(config):
for mark, (flag, description, _path) in OPT_IN.items():
config.addinivalue_line(
"markers", f"{mark}: {description}; opt in with {flag}"
)
if config.getoption("--doctest-force-numeric"):
doctest.OutputChecker.check_output = ( # type: ignore[method-assign]
_forced_check_output # type: ignore[assignment]
)


def pytest_collection_modifyitems(config, items):
Expand Down
56 changes: 50 additions & 6 deletions docs/Contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,55 @@ documentation build from running on every change:

Continuous integration (``.github/workflows/actions.yml``) therefore runs on
**pull requests into develop or master** and on **pushes to master**, rather
than on every push to every branch. Read the Docs is configured to build
``master`` and tags only. The net effect is that the full test suite and the
documentation build run once per pull request and once per release, instead of
once per intermediate commit.
than on every push to every branch. Not every job runs on every event:

.. list-table::
:header-rows: 1
:widths: 30 70

* - Event
- Jobs
* - Pull request into ``develop``
- lint only (about a minute)
* - Pull request into ``master`` (the release)
- lint, the test suite across three interpreters, and the
documentation build (about ten minutes)
* - Push to ``master`` / tag
- lint and the test suite; Read the Docs rebuilds the hosted
documentation

The test suite and the documentation build are both gated at the release
rather than on every pull request because of what they cost: the suite is
roughly nine minutes across the three interpreters and the documentation build
around three from cold, against about one for lint. Paying that on every
feature pull request made the edit-review loop the slowest part of working on
the package, and with a single maintainer running the suite locally before
pushing, the pull-request run was mostly confirming what was already known.

The trade-off is real and worth understanding before you rely on it. A failure
that appears on only one interpreter, or a change that breaks a documentation
example, is now found when the release pull request is opened -- with a
release's worth of commits to search through rather than one. So:

* Run the suite locally before pushing, and across more than one interpreter
when you have touched anything numerical. ``scripts/check_all_pythons.py``
does exactly that -- it runs what continuous integration would have run, on
3.11, 3.12 and 3.13:

.. code-block:: bash

python scripts/check_all_pythons.py # all three
python scripts/check_all_pythons.py 3.12 # just one
python scripts/check_all_pythons.py --skip-install # reuse as-is

It keeps its environments in ``.venvs/`` (git-ignored) and reuses them, so
only the first run pays for the installs. It uses ``uv`` when that is
available and falls back to ``venv`` and ``pip`` when it is not.

* Build the documentation locally when you change the behaviour of a public
function, since documentation cells call the real API.
* On a long-running branch, open the release pull request early and let it sit,
so the full run has somewhere to fail before the release itself.

Documentation
-------------
Expand All @@ -41,8 +86,7 @@ To build the documentation locally:

.. code-block:: bash

pip install -e .
pip install -r docs/requirements.txt
pip install -e ".[docs]"
sphinx-build -b html docs docs/_build/html

When writing documentation, prefer ``.. jupyter-execute::`` over static
Expand Down
Loading
Loading