From c1953c6838c6ebebc9002e083c36e012de13d701 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 07:08:07 -0400 Subject: [PATCH 1/7] Build OBR targets only from hash-verified EFO workbooks The OBR targets came from whatever obr.uk served, falling back to committed workbooks matched by URL substring, with the vintage label taken from sources.yaml rather than the bytes, year columns fixed by letter, and missing rows logged and dropped. A manifest per EFO vintage (targets/source_manifests/) now names the two workbooks with their SHA-256, size and own title, the unit and financial years every parsed table must head, and the targets they must yield. Builds read only the verified committed bytes; each table's years come from its header row; a missing target or year, or an undeclared one, fails the build. `python -m policyengine_uk_data.targets.obr_sources ingest` adopts a new vintage from browser-downloaded workbooks and prints each target's change. On the March 2026 bytes all 29 OBR targets are unchanged. Co-Authored-By: Claude Opus 5.5 --- changelog.d/obr-source-manifest.changed.md | 1 + policyengine_uk_data/targets/obr_sources.py | 496 ++++++++++++++++++ .../source_manifests/obr_march_2026.yaml | 69 +++ policyengine_uk_data/targets/sources.yaml | 7 +- policyengine_uk_data/targets/sources/obr.py | 321 +++--------- .../tests/test_dwp_housing_benefit_targets.py | 3 + .../tests/test_esa_jsa_targets.py | 3 + .../tests/test_obr_efo_fallback.py | 250 --------- policyengine_uk_data/tests/test_obr_nics.py | 31 +- .../tests/test_obr_receipts_sheet.py | 15 +- .../tests/test_obr_sources.py | 385 ++++++++++++++ .../tests/test_obr_universal_credit_target.py | 3 + 12 files changed, 1045 insertions(+), 539 deletions(-) create mode 100644 changelog.d/obr-source-manifest.changed.md create mode 100644 policyengine_uk_data/targets/obr_sources.py create mode 100644 policyengine_uk_data/targets/source_manifests/obr_march_2026.yaml delete mode 100644 policyengine_uk_data/tests/test_obr_efo_fallback.py create mode 100644 policyengine_uk_data/tests/test_obr_sources.py diff --git a/changelog.d/obr-source-manifest.changed.md b/changelog.d/obr-source-manifest.changed.md new file mode 100644 index 000000000..b524adfd8 --- /dev/null +++ b/changelog.d/obr-source-manifest.changed.md @@ -0,0 +1 @@ +Build the OBR targets only from the committed EFO workbooks named, with their SHA-256 and title, in a vintage manifest (`targets/source_manifests/`). Read each table's financial years from its header row rather than fixed column letters, and fail the build when a manifest target or year is missing instead of logging and dropping it. Add `python -m policyengine_uk_data.targets.obr_sources ingest` for adopting a new EFO vintage, which prints every target's change. diff --git a/policyengine_uk_data/targets/obr_sources.py b/policyengine_uk_data/targets/obr_sources.py new file mode 100644 index 000000000..4620e580d --- /dev/null +++ b/policyengine_uk_data/targets/obr_sources.py @@ -0,0 +1,496 @@ +"""Hash-bound OBR Economic and fiscal outlook workbooks. + +Each EFO vintage the OBR targets are built from has a manifest in +``targets/source_manifests/``. It names the two "detailed forecast tables" +workbooks (receipts and expenditure) with their SHA-256, size, title and +provenance. It also gives the unit and the financial years every parsed +table must carry, and the targets the vintage must yield. ``sources.yaml`` +names the manifest in use. + +Builds read only the committed bytes, verified against the manifest, and +never download. A target's ``forecast_vintage`` therefore always names the +publication its bytes came from. Year columns are read from each table's +header row, so a vintage whose tables start a year later is parsed by +year rather than by column letter. A missing row, year or target fails the +build instead of being logged and dropped. + +Adopting a new vintage (for example on Budget day): + + python -m policyengine_uk_data.targets.obr_sources ingest \\ + --vintage october_2026 --label "October 2026" \\ + --receipts receipts.xlsx --expenditure expenditure.xlsx \\ + --first-year 2025 --last-year 2031 + +obr.uk refuses scripted downloads (HTTP 403 to GitHub Actions runners, as +11e41f9d records, and to a developer machine on 2026-10-15), so download +the two workbooks in a browser and pass their paths. ``ingest`` copies them under ``storage/obr_efo//``, +checks that each workbook's title names the vintage, parses them with the +build's own parsers, and requires the targets the current manifest +requires. It then writes the manifest and prints each target's change from +the current vintage. Point ``sources.yaml`` at the new manifest to adopt it. +""" + +from __future__ import annotations + +import argparse +import hashlib +import io +import re +import shutil +import sys +from dataclasses import dataclass +from datetime import date +from functools import lru_cache +from pathlib import Path +from typing import Mapping, Optional, Sequence + +import openpyxl +import yaml + +MANIFEST_DIR = Path(__file__).parent / "source_manifests" +KINDS = ("receipts", "expenditure") +UNIT_SCALES = {"£ billion": 1e9} +_FINANCIAL_YEAR = re.compile(r"^\s*(\d{4})\s*[-–]\s*(\d{2})\s*$") +_HEADER_ROWS = 10 +_UNIT_ROWS = 4 + + +class SourceError(Exception): + """A source file, table or target set does not match its manifest. + + Not a ValueError, so a parser's "row not found" handling cannot swallow + it: a table without the declared years or unit stops the build. + """ + + +@dataclass(frozen=True) +class SourceFile: + kind: str + path: str # relative to the storage folder + sha256: str + size_bytes: int + title: str # the workbook's own title, cell B2 of its Contents sheet + url: str + retrieved: str + + +@dataclass(frozen=True) +class ObrManifest: + vintage: str + label: str + first_year: int # financial years by the calendar year they start in + last_year: int + unit: str + files: Mapping[str, SourceFile] + required: tuple # target names + alternatives: tuple # each a tuple of sets of names; one set must be present + path: Optional[Path] = None + + @property + def years(self) -> list: + return list(range(self.first_year, self.last_year + 1)) + + @property + def scale(self) -> float: + return UNIT_SCALES[self.unit] + + +def parse_manifest(raw: Mapping, path: Optional[Path] = None) -> ObrManifest: + where = str(path or "manifest") + if (raw or {}).get("schema_version") != 1: + raise SourceError(f"{where}: schema_version must be 1") + unit = raw.get("unit") + if unit not in UNIT_SCALES: + raise SourceError(f"{where}: unit must be one of {sorted(UNIT_SCALES)}") + first, last = int(raw["first_year"]), int(raw["last_year"]) + if not first <= last: + raise SourceError(f"{where}: first_year is after last_year") + files = {} + for kind in KINDS: + entry = (raw.get("files") or {}).get(kind) + if not entry: + raise SourceError(f"{where}: no {kind} workbook") + sha = str(entry["sha256"]) + if not re.fullmatch(r"[0-9a-f]{64}", sha): + raise SourceError(f"{where}: {kind} sha256 must be 64 hex characters") + files[kind] = SourceFile( + kind=kind, + path=str(entry["path"]), + sha256=sha, + size_bytes=int(entry["size_bytes"]), + title=str(entry["title"]), + url=str(entry["url"]), + retrieved=str(entry["retrieved"]), + ) + required, alternatives = [], [] + for item in (raw.get("targets") or {}).get("required") or []: + if isinstance(item, str): + required.append(item) + elif isinstance(item, Mapping) and item.get("any_of"): + alternatives.append(tuple(frozenset(group) for group in item["any_of"])) + else: + raise SourceError(f"{where}: unreadable required target {item!r}") + if len(set(required)) != len(required): + raise SourceError(f"{where}: a required target is listed twice") + if not required: + raise SourceError(f"{where}: no required targets") + return ObrManifest( + vintage=str(raw["vintage"]), + label=str(raw["label"]), + first_year=first, + last_year=last, + unit=unit, + files=files, + required=tuple(required), + alternatives=tuple(alternatives), + path=path, + ) + + +def load_manifest(path) -> ObrManifest: + path = Path(path) + if not path.is_absolute(): + path = MANIFEST_DIR / path + with open(path) as f: + return parse_manifest(yaml.safe_load(f), path) + + +@lru_cache(maxsize=1) +def active_manifest() -> ObrManifest: + """The manifest ``sources.yaml`` names for the OBR targets.""" + from policyengine_uk_data.targets.sources._common import load_config + + return load_manifest(load_config()["obr"]["manifest"]) + + +def _storage() -> Path: + from policyengine_uk_data.storage import STORAGE_FOLDER + + return Path(STORAGE_FOLDER) + + +def sha256_bytes(data: bytes) -> str: + return hashlib.sha256(data).hexdigest() + + +def verified_bytes(entry: SourceFile, storage: Optional[Path] = None) -> bytes: + """The file's bytes, if and only if they are the ones the manifest names.""" + path = (storage or _storage()) / entry.path + if not path.is_file(): + raise SourceError(f"OBR {entry.kind} workbook {path} is missing") + data = path.read_bytes() + if len(data) != entry.size_bytes or sha256_bytes(data) != entry.sha256: + raise SourceError( + f"OBR {entry.kind} workbook {path} is not the file its manifest " + f"names: {len(data)} bytes, sha256 {sha256_bytes(data)}, against " + f"{entry.size_bytes} bytes, sha256 {entry.sha256}" + ) + return data + + +def workbook_title(wb: openpyxl.Workbook) -> str: + """The title an EFO workbook gives itself (Contents sheet, cell B2).""" + if "Contents" not in wb.sheetnames: + raise SourceError("the workbook has no Contents sheet") + return str(wb["Contents"]["B2"].value or "").strip() + + +def load_workbook( + kind: str, + manifest: Optional[ObrManifest] = None, + storage: Optional[Path] = None, +) -> openpyxl.Workbook: + """Open a manifest's verified workbook, checking it carries its own title.""" + manifest = manifest or active_manifest() + entry = manifest.files[kind] + wb = openpyxl.load_workbook( + io.BytesIO(verified_bytes(entry, storage)), data_only=False + ) + if workbook_title(wb) != entry.title: + raise SourceError( + f"OBR {kind} workbook is titled {workbook_title(wb)!r}, not {entry.title!r}" + ) + return wb + + +def year_columns(ws, first_year: int, last_year: int) -> dict: + """Map the column letters of a table's financial-year headers to years. + + The header row is the first of the top rows with cells reading like + "2025-26"; each maps to the calendar year it starts in. The years must + be exactly ``first_year`` to ``last_year``, each once, so a table that + starts a year later, or misses a year, fails rather than being read + with the wrong labels. + """ + for row in ws.iter_rows(min_row=1, max_row=_HEADER_ROWS): + found = {} + for cell in row: + match = ( + _FINANCIAL_YEAR.match(cell.value) + if isinstance(cell.value, str) + else None + ) + if not match: + continue + start, end = int(match[1]), int(match[2]) + if (start + 1) % 100 != end: + raise SourceError( + f"{ws.title}!{cell.coordinate}: {cell.value!r} is not one " + "financial year" + ) + if start in found.values(): + raise SourceError( + f"{ws.title}: {cell.value!r} heads two columns of row {cell.row}" + ) + found[cell.column_letter] = start + if found: + years = sorted(found.values()) + if years != list(range(first_year, last_year + 1)): + raise SourceError( + f"{ws.title}: header row {row[0].row} covers " + f"{', '.join(str(y) for y in years)}, but the manifest " + f"declares {first_year} to {last_year}" + ) + return found + raise SourceError( + f"{ws.title}: no row of financial-year headers in rows 1-{_HEADER_ROWS}" + ) + + +def unit_scale(ws, unit: str) -> float: + """The multiplier for a table's values, from its unit label.""" + for row in ws.iter_rows(min_row=1, max_row=_UNIT_ROWS): + for cell in row: + if isinstance(cell.value, str) and cell.value.strip().startswith(unit): + return UNIT_SCALES[unit] + raise SourceError(f"{ws.title}: no {unit!r} unit label in rows 1-{_UNIT_ROWS}") + + +def read_row(ws, row: int, manifest: ObrManifest) -> dict: + """A table row's numeric values by year, in pounds.""" + scale = unit_scale(ws, manifest.unit) + out = {} + for column, year in year_columns( + ws, manifest.first_year, manifest.last_year + ).items(): + value = ws[f"{column}{row}"].value + if isinstance(value, (int, float)) and not isinstance(value, bool): + out[year] = float(value) * scale + return out + + +def coverage_problems(targets: Sequence, manifest: ObrManifest) -> list: + """Everything wrong with the targets built from a vintage's workbooks. + + Targets built from the workbooks are those carrying the vintage. Each + required target must be present with every year of the manifest; one + set of each ``any_of`` group must be present; and no workbook target + may be missing from the manifest, so the manifest stays the complete + list. + """ + built = {t.name: t for t in targets if t.forecast_vintage == manifest.vintage} + problems = [] + for name in manifest.required: + if name not in built: + problems.append(f"{name} was not built") + for groups in manifest.alternatives: + if not any(group <= set(built) for group in groups): + options = " or ".join("+".join(sorted(g)) for g in groups) + problems.append(f"none of {options} was built") + declared = set(manifest.required).union( + *(group for groups in manifest.alternatives for group in groups) + ) + for name, target in sorted(built.items()): + if name not in declared: + problems.append(f"{name} was built but is not in the manifest") + missing = [y for y in manifest.years if y not in target.values] + if missing: + problems.append( + f"{name} has no value for {', '.join(str(y) for y in missing)}" + ) + return problems + + +def check_coverage(targets: Sequence, manifest: ObrManifest) -> None: + problems = coverage_problems(targets, manifest) + if problems: + raise SourceError( + f"OBR {manifest.label} targets are incomplete: " + "; ".join(problems) + ) + + +def target_diff(old: Sequence, new: Sequence) -> list: + """Each workbook target's value by year in two target sets.""" + before = {t.name: t for t in old} + after = {t.name: t for t in new} + rows = [] + for name in sorted(set(before) | set(after)): + a, b = before.get(name), after.get(name) + years = sorted(set(a.values if a else {}) | set(b.values if b else {})) + for year in years: + x = a.values.get(year) if a else None + y = b.values.get(year) if b else None + rows.append({"target": name, "year": year, "old": x, "new": y}) + return rows + + +def _manifest_yaml(manifest: ObrManifest, alternatives_raw) -> str: + raw = { + "schema_version": 1, + "vintage": manifest.vintage, + "label": manifest.label, + "first_year": manifest.first_year, + "last_year": manifest.last_year, + "unit": manifest.unit, + "files": { + kind: { + "path": f.path, + "sha256": f.sha256, + "size_bytes": f.size_bytes, + "title": f.title, + "url": f.url, + "retrieved": f.retrieved, + } + for kind, f in manifest.files.items() + }, + "targets": { + "required": list(manifest.required) + + [{"any_of": groups} for groups in alternatives_raw] + }, + } + header = ( + f"# OBR Economic and fiscal outlook, {manifest.label}: detailed forecast\n" + "# tables. Written by `python -m policyengine_uk_data.targets.obr_sources\n" + "# ingest`; see that module for what each field means.\n" + ) + return header + yaml.safe_dump(raw, sort_keys=False, allow_unicode=True) + + +def ingest( + *, + vintage: str, + label: str, + sources: Mapping[str, Path], + urls: Mapping[str, str], + first_year: int, + last_year: int, + current: Optional[ObrManifest] = None, + storage: Optional[Path] = None, + manifest_dir: Optional[Path] = None, + retrieved: Optional[str] = None, +): + """Adopt a vintage's two workbooks. Returns (manifest, manifest path, diff).""" + from policyengine_uk_data.targets.sources import obr + + current = current or active_manifest() + storage = storage or _storage() + manifest_dir = manifest_dir or MANIFEST_DIR + retrieved = retrieved or f"ingested {date.today().isoformat()}" + if not re.fullmatch(r"[a-z]+_\d{4}", vintage): + raise SourceError("vintage must look like october_2026") + files = {} + for kind in KINDS: + data = Path(sources[kind]).read_bytes() + try: + wb = openpyxl.load_workbook(io.BytesIO(data), data_only=False) + except Exception as error: + raise SourceError( + f"{sources[kind]} is not an xlsx workbook ({type(error).__name__}: " + f"{error}); obr.uk serves an HTML page to scripted downloads, so " + "download the workbook in a browser" + ) from error + title = workbook_title(wb) + if not title.startswith( + f"{label} Economic and fiscal outlook" + ) or not title.endswith(kind): + raise SourceError( + f"{sources[kind]} is titled {title!r}, which is not the {label} " + f"EFO {kind} tables" + ) + relative = f"obr_efo/{vintage}/{kind}.xlsx" + target = storage / relative + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(sources[kind], target) + files[kind] = SourceFile( + kind=kind, + path=relative, + sha256=sha256_bytes(data), + size_bytes=len(data), + title=title, + url=urls.get(kind, ""), + retrieved=retrieved, + ) + alternatives_raw = [ + [sorted(group) for group in groups] for groups in current.alternatives + ] + manifest = ObrManifest( + vintage=vintage, + label=label, + first_year=first_year, + last_year=last_year, + unit=current.unit, + files=files, + required=current.required, + alternatives=current.alternatives, + ) + try: + # Raises unless the new workbooks yield every target and year required. + new = obr.workbook_targets(manifest, storage) + except Exception: + shutil.rmtree(storage / "obr_efo" / vintage, ignore_errors=True) + raise + old = obr.workbook_targets(current) + path = Path(manifest_dir) / f"obr_{vintage}.yaml" + path.write_text(_manifest_yaml(manifest, alternatives_raw)) + return load_manifest(path), path, target_diff(old, new) + + +def _gbp(value) -> str: + return "" if value is None else f"{value / 1e9:,.3f}" + + +def main(argv=None) -> int: + parser = argparse.ArgumentParser( + prog="python -m policyengine_uk_data.targets.obr_sources" + ) + sub = parser.add_subparsers(dest="command", required=True) + i = sub.add_parser("ingest", help="adopt a new EFO vintage's workbooks") + i.add_argument("--vintage", required=True, help="e.g. october_2026") + i.add_argument("--label", required=True, help='e.g. "October 2026"') + i.add_argument("--receipts", required=True, type=Path) + i.add_argument("--expenditure", required=True, type=Path) + i.add_argument("--receipts-url", default="") + i.add_argument("--expenditure-url", default="") + i.add_argument("--first-year", required=True, type=int) + i.add_argument("--last-year", required=True, type=int) + i.add_argument("--retrieved", help="how and when the files were obtained") + v = sub.add_parser("verify", help="verify the active manifest's workbooks") + v.add_argument("--manifest", help="manifest file (default: the active one)") + args = parser.parse_args(argv) + if args.command == "verify": + from policyengine_uk_data.targets.sources import obr + + manifest = load_manifest(args.manifest) if args.manifest else active_manifest() + targets = obr.workbook_targets(manifest) + check_coverage(targets, manifest) + print(f"{manifest.label}: both workbooks verified, {len(targets)} targets") + return 0 + manifest, path, diff = ingest( + vintage=args.vintage, + label=args.label, + sources={"receipts": args.receipts, "expenditure": args.expenditure}, + urls={"receipts": args.receipts_url, "expenditure": args.expenditure_url}, + first_year=args.first_year, + last_year=args.last_year, + retrieved=args.retrieved, + ) + print(f"wrote {path}") + print("target,year,current £bn,new £bn") + for row in diff: + print(f"{row['target']},{row['year']},{_gbp(row['old'])},{_gbp(row['new'])}") + print(f"To adopt it, set obr.manifest in sources.yaml to {path.name}.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/policyengine_uk_data/targets/source_manifests/obr_march_2026.yaml b/policyengine_uk_data/targets/source_manifests/obr_march_2026.yaml new file mode 100644 index 000000000..a0716cc29 --- /dev/null +++ b/policyengine_uk_data/targets/source_manifests/obr_march_2026.yaml @@ -0,0 +1,69 @@ +# OBR Economic and fiscal outlook, March 2026: detailed forecast tables. +# The bytes, not this file's labels, decide the vintage: builds verify each +# workbook's SHA-256, size and own title (Contents!B2) before parsing it. +# See targets/obr_sources.py for every field. +schema_version: 1 +vintage: march_2026 +label: March 2026 +# Financial years every parsed table must head, by the calendar year they +# start in: 2024-25 to 2030-31. +first_year: 2024 +last_year: 2030 +unit: £ billion +files: + receipts: + path: obr_efo/efo_receipts.xlsx + sha256: f65b6cb7f96931d4abd85940794eb60f6f65afca8bbf16c74a4548bf92766a28 + size_bytes: 187899 + title: "March 2026 Economic and fiscal outlook – detailed forecast tables: receipts" + url: https://obr.uk/download/march-2026-economic-and-fiscal-outlook-detailed-forecast-tables-receipts/ + retrieved: >- + Committed on 2026-07-21 in 11e41f9d, as the download fallback. The + workbook's own properties record it as modified on 2026-03-02. + obr.uk refused scripted downloads on 2026-10-15 (HTTP 403), so the + bytes were not re-fetched for this manifest. + expenditure: + path: obr_efo/efo_expenditure.xlsx + sha256: 700bfd700c872794e5fe8cf164086cd38a63850db26427b9d4b2e363d177d9a1 + size_bytes: 236965 + title: "March 2026 Economic and fiscal outlook – detailed forecast tables: expenditure" + url: https://obr.uk/download/march-2026-economic-and-fiscal-outlook-detailed-forecast-tables-expenditure/ + retrieved: >- + Committed on 2026-07-21 in 11e41f9d, as the download fallback. The + workbook's own properties record it as modified on 2026-03-02. + obr.uk refused scripted downloads on 2026-10-15 (HTTP 403), so the + bytes were not re-fetched for this manifest. +# Every target the workbooks must yield, each with every year above. A +# target the parsers build that is not listed here also fails, so changing +# the target set means changing this list. +targets: + required: + - obr/income_tax + - obr/ni + - obr/vat + - obr/fuel_duties + - obr/capital_gains_tax + - obr/sdlt + - obr/ni_employee + - obr/ni_employer + - obr/council_tax + - obr/council_tax_england + - obr/council_tax_scotland + - obr/council_tax_wales + - obr/domestic_rates + - obr/pip + - obr/esa + - obr/attendance_allowance + - obr/carers_allowance + - obr/statutory_maternity_pay + - obr/winter_fuel_allowance + - obr/child_benefit + - obr/state_pension + - obr/jobseekers_allowance + - obr/universal_credit + - obr/tv_licence_fee + # Self-employed NICs: one combined Class 2 and 4 line (March 2026), or + # the two classes on separate rows (earlier vintages). + - any_of: + - [obr/ni_self_employed] + - [obr/ni_class_2, obr/ni_class_4] diff --git a/policyengine_uk_data/targets/sources.yaml b/policyengine_uk_data/targets/sources.yaml index 973e39567..a5a3b7733 100644 --- a/policyengine_uk_data/targets/sources.yaml +++ b/policyengine_uk_data/targets/sources.yaml @@ -2,9 +2,10 @@ # Update these when new vintages are published. obr: - efo_receipts: "https://obr.uk/download/march-2026-economic-and-fiscal-outlook-detailed-forecast-tables-receipts/" - efo_expenditure: "https://obr.uk/download/march-2026-economic-and-fiscal-outlook-detailed-forecast-tables-expenditure/" - vintage: "march_2026" + # The EFO vintage the OBR targets come from: a manifest in + # targets/source_manifests/ naming its hash-verified workbooks. Adopt a new + # vintage with `python -m policyengine_uk_data.targets.obr_sources ingest`. + manifest: obr_march_2026.yaml hmrc: spi_collated: "https://assets.publishing.service.gov.uk/media/69f1f12d2fae53a03709682f/Collated_Tables_3_1_to_3_11_2324.ods" diff --git a/policyengine_uk_data/targets/sources/obr.py b/policyengine_uk_data/targets/sources/obr.py index 1b7d3661c..60f2be1d1 100644 --- a/policyengine_uk_data/targets/sources/obr.py +++ b/policyengine_uk_data/targets/sources/obr.py @@ -1,150 +1,28 @@ """OBR Economic and Fiscal Outlook targets. -Downloads and parses the OBR's detailed supplementary tables (receipts -and expenditure xlsx) to extract tax receipt forecasts, benefit -expenditure, and benefit caseloads. - -Sources: -- Receipts: https://obr.uk/download/march-2026-economic-and-fiscal-outlook-detailed-forecast-tables-receipts/ -- Expenditure: https://obr.uk/download/march-2026-economic-and-fiscal-outlook-detailed-forecast-tables-expenditure/ +Parses the OBR's detailed forecast tables (receipts and expenditure +workbooks) to extract tax receipt forecasts, benefit expenditure, and +benefit caseloads. The workbooks are the committed, hash-verified files the +active manifest names (``targets/obr_sources.py``); nothing is downloaded. +Each table's year columns are read from its header row, and a missing +target or year fails the build. """ -import io import logging -import time -from functools import lru_cache import openpyxl -import requests -from policyengine_uk_data.targets.schema import GREAT_BRITAIN, Target, Unit -from policyengine_uk_data.targets.sources._common import ( - HEADERS, - load_config, +from policyengine_uk_data.targets.obr_sources import ( + ObrManifest, + active_manifest, + check_coverage, + load_workbook, + read_row, ) +from policyengine_uk_data.targets.schema import GREAT_BRITAIN, Target, Unit logger = logging.getLogger(__name__) -# Financial year columns in OBR tables: C=2024-25, D=2025-26, ..., I=2030-31 -# PolicyEngine convention: FY 2025-26 → calendar year 2025 (first year) -_FY_COL_TO_YEAR = { - "C": 2024, - "D": 2025, - "E": 2026, - "F": 2027, - "G": 2028, - "H": 2029, - "I": 2030, -} - - -# OBR occasionally rate-limits CI runners (HTTP 429) or returns transient 5xx -# errors. Retry with exponential backoff so a single throttled response does not -# drop the whole OBR target set and red an unrelated build. -_DOWNLOAD_MAX_ATTEMPTS = 4 -_DOWNLOAD_RETRY_STATUSES = {429, 500, 502, 503, 504} - - -# obr.uk serves 403 Forbidden to GitHub Actions runner IPs (observed on the -# 2026-07-21 push builds: every attempt 403'd while the same URLs work from -# residential connections). A 403 is not retryable, and losing the workbook -# silently drops 28 OBR targets and calibrates a degraded dataset. The EFO -# workbooks for the vintage pinned in sources.yaml are therefore committed -# under storage/obr_efo/ — following the committed-not-fetched precedent of -# capital_gains_size_distribution_hmrc.csv and ons_demographics.py — and -# used as a fallback whenever the download fails. Update them together with -# the sources.yaml URLs when a new EFO vintage is adopted. -_EFO_FALLBACKS = { - "receipts": "efo_receipts.xlsx", - "expenditure": "efo_expenditure.xlsx", -} - - -def _fallback_workbook(url: str) -> openpyxl.Workbook | None: - from policyengine_uk_data.storage import STORAGE_FOLDER - - for slug, filename in _EFO_FALLBACKS.items(): - if slug in url: - path = STORAGE_FOLDER / "obr_efo" / filename - if path.exists(): - return openpyxl.load_workbook(path, data_only=False) - return None - - -@lru_cache(maxsize=2) -def _download_workbook(url: str) -> openpyxl.Workbook: - """Download an xlsx from OBR and return an openpyxl workbook. - - Retries transient HTTP errors (429/5xx) and connection failures with - exponential backoff, honouring a numeric Retry-After header when - present. Falls back to the committed workbook in storage/obr_efo/ when - the download ultimately fails: obr.uk 403s CI runner IPs, and can answer - 200 with an HTML page that is not a workbook. Neither is retried. - """ - last_error: Exception | None = None - for attempt in range(_DOWNLOAD_MAX_ATTEMPTS): - wait = 2**attempt - try: - r = requests.get(url, headers=HEADERS, allow_redirects=True, timeout=60) - except requests.RequestException as e: - last_error = e # connection/timeout — retryable - else: - if r.status_code < 400: - # obr.uk can answer 200 with an HTML "No Access" page instead - # of the workbook (observed 2026-10-04). A body openpyxl cannot - # read will not become a workbook on retry, so it is permanent, - # like a 403. An openpyxl break on every workbook still - # surfaces, as the fallback goes through the same load_workbook. - try: - return openpyxl.load_workbook( - io.BytesIO(r.content), data_only=False - ) - except Exception as e: - last_error = ValueError( - f"{r.status_code} for url: {url}, but the body " - f"({r.headers.get('Content-Type', 'no Content-Type')}) " - "could not be read as an xlsx workbook " - f"({type(e).__name__}: {e})" - ) - last_error.__cause__ = e - break - last_error = requests.HTTPError( - f"{r.status_code} for url: {url}", response=r - ) - if r.status_code not in _DOWNLOAD_RETRY_STATUSES: - break # 403 and other permanent errors: don't burn retries - retry_after = r.headers.get("Retry-After", "") - if retry_after.isdigit(): - wait = int(retry_after) - if attempt < _DOWNLOAD_MAX_ATTEMPTS - 1: - logger.warning( - "OBR download %s failed (%s); retrying in %ss", - url, - last_error, - wait, - ) - time.sleep(wait) - fallback = _fallback_workbook(url) - if fallback is not None: - logger.warning( - "OBR download %s failed (%s); using committed workbook fallback", - url, - last_error, - ) - return fallback - raise last_error - - -def _read_row_values(ws, row_num: int, col_letters: list[str]) -> dict[int, float]: - """Read numeric values from a row, mapped to calendar years.""" - result = {} - for col in col_letters: - cell = ws[f"{col}{row_num}"] - val = cell.value - if val is not None and isinstance(val, (int, float)): - result[_FY_COL_TO_YEAR[col]] = float(val) * 1e9 - return result - def _find_receipts_sheet(wb): """Return the current-receipts worksheet, located by title. @@ -193,7 +71,9 @@ def _find_row(ws, label: str, col: str = "B", max_row: int = 80) -> int: raise ValueError(f"Row '{label}' not found in sheet") -def _parse_receipts(wb: openpyxl.Workbook) -> list[Target]: +def _parse_receipts( + wb: openpyxl.Workbook, manifest: ObrManifest | None = None +) -> list[Target]: """Parse tax receipts from the OBR EFO. Income tax uses Table 3.4 (accrued basis) for consistency with @@ -201,32 +81,8 @@ def _parse_receipts(wb: openpyxl.Workbook) -> list[Target]: current-receipts table (cash basis) since they only appear there; that table is located by title because EFO vintages renumber the sheets. """ - config = load_config() - vintage = config["obr"]["vintage"] - ref = config["obr"]["efo_receipts"] - cols_34 = list(_FY_COL_TO_YEAR.keys()) - - # Table 3.9 columns are shifted right by one vs 3.4 - cols_39 = ["D", "E", "F", "G", "H", "I", "J"] - fy_39 = { - "D": 2024, - "E": 2025, - "F": 2026, - "G": 2027, - "H": 2028, - "I": 2029, - "J": 2030, - } - - def read_39(ws, row_num: int) -> dict[int, float]: - result = {} - for col in cols_39: - cell = ws[f"{col}{row_num}"] - val = cell.value - if val is not None and isinstance(val, (int, float)): - result[fy_39[col]] = float(val) * 1e9 - return result - + manifest = manifest or active_manifest() + vintage, ref = manifest.vintage, manifest.files["receipts"].url targets = [] # Income tax from Table 3.4 (accrued basis) @@ -235,7 +91,7 @@ def read_39(ws, row_num: int) -> dict[int, float]: row_num = _find_row( ws34, "Income tax (gross of tax credits)", col="B", max_row=30 ) - values = _read_row_values(ws34, row_num, cols_34) + values = read_row(ws34, row_num, manifest) if values: targets.append( Target( @@ -268,7 +124,7 @@ def read_39(ws, row_num: int) -> dict[int, float]: for name, (label, variable) in cash_rows.items(): try: row_num = _find_row(ws39, label, col="B", max_row=80) - values = read_39(ws39, row_num) + values = read_row(ws39, row_num, manifest) if values: recovered.append(label) targets.append( @@ -299,33 +155,14 @@ def read_39(ws, row_num: int) -> dict[int, float]: return targets -def _parse_council_tax(wb: openpyxl.Workbook) -> list[Target]: +def _parse_council_tax( + wb: openpyxl.Workbook, manifest: ObrManifest | None = None +) -> list[Target]: """Parse Table 4.1 (council tax receipts) from expenditure xlsx.""" - config = load_config() - vintage = config["obr"]["vintage"] - ref = config["obr"]["efo_expenditure"] + manifest = manifest or active_manifest() + vintage, ref = manifest.vintage, manifest.files["expenditure"].url ws = wb["4.1"] - cols = ["C", "D", "E", "F", "G", "H", "I"] - fy = { - "C": 2024, - "D": 2025, - "E": 2026, - "F": 2027, - "G": 2028, - "H": 2029, - "I": 2030, - } - - def read_41(row_num: int) -> dict[int, float]: - result = {} - for col in cols: - cell = ws[f"{col}{row_num}"] - val = cell.value - if val is not None and isinstance(val, (int, float)): - result[fy[col]] = float(val) * 1e9 - return result - ct_rows = { "council_tax": ("Total net council tax receipts", "council_tax"), "council_tax_england": ( @@ -344,7 +181,7 @@ def read_41(row_num: int) -> dict[int, float]: for name, (label, variable) in ct_rows.items(): try: row_num = _find_row(ws, label, col="B", max_row=30) - values = read_41(row_num) + values = read_row(ws, row_num, manifest) if values: targets.append( Target( @@ -363,7 +200,9 @@ def read_41(row_num: int) -> dict[int, float]: return targets -def _parse_nics(wb: openpyxl.Workbook) -> list[Target]: +def _parse_nics( + wb: openpyxl.Workbook, manifest: ObrManifest | None = None +) -> list[Target]: """Parse Table 3.4 (income tax and NICs detail) for all NIC classes. Covers Class 1 employee and employer (PAYE), plus Classes 2, 3 and 4 @@ -375,11 +214,9 @@ def _parse_nics(wb: openpyxl.Workbook) -> list[Target]: Where a row label is not found the parser logs a warning and skips that variable, so partial matches still produce useful targets. """ - config = load_config() - vintage = config["obr"]["vintage"] - ref = config["obr"]["efo_receipts"] + manifest = manifest or active_manifest() + vintage, ref = manifest.vintage, manifest.files["receipts"].url ws = wb["3.4"] - cols = list(_FY_COL_TO_YEAR.keys()) # Map of target name → (list of candidate labels to search, variable # name). Candidates let us accept minor wording variation in OBR EFOs @@ -455,7 +292,7 @@ def _parse_nics(wb: openpyxl.Workbook) -> list[Target]: ) continue - values = _read_row_values(ws, row_num, cols) + values = read_row(ws, row_num, manifest) if not values: continue @@ -499,32 +336,16 @@ def _universal_credit_rows(ws) -> tuple[int, int]: return inside[0], outside[0] -def _parse_welfare(wb: openpyxl.Workbook) -> list[Target]: +def _parse_welfare( + wb: openpyxl.Workbook, manifest: ObrManifest | None = None +) -> list[Target]: """Parse Table 4.9 (welfare spending) from expenditure xlsx.""" - config = load_config() - vintage = config["obr"]["vintage"] - ref = config["obr"]["efo_expenditure"] + manifest = manifest or active_manifest() + vintage, ref = manifest.vintage, manifest.files["expenditure"].url ws = wb["4.9"] - cols = ["C", "D", "E", "F", "G", "H", "I"] - fy = { - "C": 2024, - "D": 2025, - "E": 2026, - "F": 2027, - "G": 2028, - "H": 2029, - "I": 2030, - } - def read_49(row_num: int) -> dict[int, float]: - result = {} - for col in cols: - cell = ws[f"{col}{row_num}"] - val = cell.value - if val is not None and isinstance(val, (int, float)): - result[fy[col]] = float(val) * 1e9 - return result + return read_row(ws, row_num, manifest) # "Housing benefit (not on JSA)" is not targeted: it is DWP-funded GB # spending only, and dwp_housing_benefit.py targets total GB Housing @@ -617,35 +438,20 @@ def read_49(row_num: int) -> dict[int, float]: return targets -def _parse_tv_licence(wb: openpyxl.Workbook) -> list[Target]: +def _parse_tv_licence( + wb: openpyxl.Workbook, manifest: ObrManifest | None = None +) -> list[Target]: """Parse Table 4.19 (BBC) from expenditure xlsx.""" - config = load_config() - vintage = config["obr"]["vintage"] - ref = config["obr"]["efo_expenditure"] + manifest = manifest or active_manifest() + vintage, ref = manifest.vintage, manifest.files["expenditure"].url try: ws = wb["4.19"] - cols = ["C", "D", "E", "F", "G", "H", "I"] - fy = { - "C": 2024, - "D": 2025, - "E": 2026, - "F": 2027, - "G": 2028, - "H": 2029, - "I": 2030, - } - # Find "Licence fee receipts" or "BBC licence fee" for row_num in range(1, 30): val = ws[f"B{row_num}"].value if val and "licence fee" in str(val).lower(): - values = {} - for col in cols: - cell = ws[f"{col}{row_num}"] - v = cell.value - if v is not None and isinstance(v, (int, float)): - values[fy[col]] = float(v) * 1e9 + values = read_row(ws, row_num, manifest) if values: return [ Target( @@ -658,7 +464,7 @@ def _parse_tv_licence(wb: openpyxl.Workbook) -> list[Target]: forecast_vintage=vintage, ) ] - except Exception: + except (KeyError, ValueError): logger.warning("OBR: TV licence table not found") return [] @@ -683,24 +489,29 @@ def _parse_tv_licence(wb: openpyxl.Workbook) -> list[Target]: } -def get_targets() -> list[Target]: - config = load_config() - targets = [] +def workbook_targets(manifest: ObrManifest | None = None, storage=None) -> list[Target]: + """The targets a vintage's two verified workbooks yield. - try: - receipts_wb = _download_workbook(config["obr"]["efo_receipts"]) - targets.extend(_parse_receipts(receipts_wb)) - targets.extend(_parse_nics(receipts_wb)) - except Exception as e: - logger.error("Failed to download/parse OBR receipts: %s", e) + Fails if either workbook is not the file its manifest names, or if any + target or year the manifest requires is missing. + """ + manifest = manifest or active_manifest() + receipts = load_workbook("receipts", manifest, storage) + expenditure = load_workbook("expenditure", manifest, storage) + targets = [ + *_parse_receipts(receipts, manifest), + *_parse_nics(receipts, manifest), + *_parse_council_tax(expenditure, manifest), + *_parse_welfare(expenditure, manifest), + *_parse_tv_licence(expenditure, manifest), + ] + check_coverage(targets, manifest) + return targets - try: - expenditure_wb = _download_workbook(config["obr"]["efo_expenditure"]) - targets.extend(_parse_council_tax(expenditure_wb)) - targets.extend(_parse_welfare(expenditure_wb)) - targets.extend(_parse_tv_licence(expenditure_wb)) - except Exception as e: - logger.error("Failed to download/parse OBR expenditure: %s", e) + +def get_targets() -> list[Target]: + # Not caught: a calibration without the OBR targets is never correct. + targets = workbook_targets() # Static targets that don't come from the xlsx targets.append( diff --git a/policyengine_uk_data/tests/test_dwp_housing_benefit_targets.py b/policyengine_uk_data/tests/test_dwp_housing_benefit_targets.py index 53cd1c46e..f60612178 100644 --- a/policyengine_uk_data/tests/test_dwp_housing_benefit_targets.py +++ b/policyengine_uk_data/tests/test_dwp_housing_benefit_targets.py @@ -228,6 +228,9 @@ def test_obr_housing_benefit_row_is_not_parsed(): wb = openpyxl.Workbook() ws = wb.active ws.title = "4.9" + ws["A1"] = "£ billion" + for col, year in zip("CDEFGHI", range(2024, 2031)): + ws[f"{col}1"] = f"{year}-{(year + 1) % 100:02d}" ws["B2"] = "Housing benefit (not on JSA)1" ws["B3"] = "Attendance allowance" for row in (2, 3): diff --git a/policyengine_uk_data/tests/test_esa_jsa_targets.py b/policyengine_uk_data/tests/test_esa_jsa_targets.py index b3e7585ac..894e0d817 100644 --- a/policyengine_uk_data/tests/test_esa_jsa_targets.py +++ b/policyengine_uk_data/tests/test_esa_jsa_targets.py @@ -26,6 +26,9 @@ def test_obr_jobseekers_allowance_maps_to_total_jsa(): wb = openpyxl.Workbook() ws = wb.active ws.title = "4.9" + ws["A1"] = "£ billion" + for col, year in zip("CDEFGHI", range(2024, 2031)): + ws[f"{col}1"] = f"{year}-{(year + 1) % 100:02d}" ws["B2"] = "Jobseeker's allowance" ws["C2"] = 0.2 ws["D2"] = 0.3 diff --git a/policyengine_uk_data/tests/test_obr_efo_fallback.py b/policyengine_uk_data/tests/test_obr_efo_fallback.py deleted file mode 100644 index 5884642cc..000000000 --- a/policyengine_uk_data/tests/test_obr_efo_fallback.py +++ /dev/null @@ -1,250 +0,0 @@ -"""Tests for the committed EFO workbook fallback. - -obr.uk serves 403 Forbidden to GitHub Actions runner IPs, and losing the -workbooks silently drops 28 OBR targets and calibrates a degraded dataset -(observed on the 2026-07-21 push builds). obr.uk can also answer 200 with -an HTML "No Access" page, which once escaped the fallback as a BadZipFile -and dropped the nine receipts and NICs targets (observed 2026-10-04). These -tests pin: the fallback workbooks are committed and parseable, every kind of -failed download uses them instead of raising, and permanent failures do not -burn the retry budget. -""" - -import io -import logging -import zipfile -from contextlib import contextmanager -from types import SimpleNamespace -from unittest.mock import patch - -import pytest -import requests - -from policyengine_uk_data.storage import STORAGE_FOLDER -from policyengine_uk_data.targets.sources import obr -from policyengine_uk_data.targets.sources._common import load_config - - -@pytest.fixture(autouse=True) -def _clear_workbook_cache(): - obr._download_workbook.cache_clear() - yield - obr._download_workbook.cache_clear() - - -def _response(status_code, content=b"", content_type=None): - headers = {"Content-Type": content_type} if content_type else {} - return SimpleNamespace(status_code=status_code, headers=headers, content=content) - - -def _zip(members: dict[str, str]) -> bytes: - buffer = io.BytesIO() - with zipfile.ZipFile(buffer, "w") as archive: - for name, data in members.items(): - archive.writestr(name, data) - return buffer.getvalue() - - -def _connection_error(): - raise requests.ConnectionError("no route to obr.uk") - - -_NO_ACCESS_PAGE = ( - b"" - b"No Access - Office for Budget Responsibility" - b"

No Access

" -) - -# Every way the download can fail, with the number of requests it should -# cost: transient failures use the full retry budget, permanent ones one. -_FAILED_DOWNLOADS = { - "200-html-page": ( - lambda: _response(200, _NO_ACCESS_PAGE, "text/html; charset=UTF-8"), - 1, - ), - "200-empty-body": (lambda: _response(200), 1), - "200-truncated-zip": (lambda: _response(200, b"PK\x03\x04garbage"), 1), - "200-zip-without-xlsx-manifest": ( - lambda: _response(200, _zip({"readme.txt": "not a workbook"})), - 1, - ), - "200-zip-without-workbook-part": ( - lambda: _response( - 200, - _zip( - { - "[Content_Types].xml": '' - } - ), - ), - 1, - ), - "200-zip-with-malformed-xml": ( - lambda: _response(200, _zip({"[Content_Types].xml": " None: - """Write rows of (label, fy_values) into column B / C-I of ``ws``.""" + """Write rows of (label, fy_values) into column B / C-I of ``ws``, under + a unit label and 2024-25 to 2030-31 year headers in row 1.""" + ws["A1"] = "£ billion" + for col_idx, year in enumerate(range(2024, 2031), start=3): + ws.cell(row=1, column=col_idx, value=f"{year}-{(year + 1) % 100:02d}") for row_idx, (label, values) in enumerate(rows, start=2): ws.cell(row=row_idx, column=2, value=label) for col_idx, value in enumerate(values, start=3): @@ -93,8 +87,7 @@ def test_parse_nics_combined_self_employed_line(): target alongside Class 1 employee/employer.""" from policyengine_uk_data.targets.sources import obr as obr_module - with patch.object(obr_module, "load_config", return_value=_FAKE_CONFIG): - targets = obr_module._parse_nics(_build_combined_efo_workbook()) + targets = obr_module._parse_nics(_build_combined_efo_workbook()) names = {t.name for t in targets} assert names == { @@ -114,8 +107,7 @@ def test_parse_nics_falls_back_to_separate_classes_for_old_efo(): the legacy candidates pick them up individually.""" from policyengine_uk_data.targets.sources import obr as obr_module - with patch.object(obr_module, "load_config", return_value=_FAKE_CONFIG): - targets = obr_module._parse_nics(_build_separate_efo_workbook()) + targets = obr_module._parse_nics(_build_separate_efo_workbook()) names = {t.name for t in targets} assert names == { @@ -134,8 +126,7 @@ def test_parse_nics_intentionally_skips_class_3_in_combined_efo(): "Other NIC" and the parser must not emit it (#378).""" from policyengine_uk_data.targets.sources import obr as obr_module - with patch.object(obr_module, "load_config", return_value=_FAKE_CONFIG): - targets = obr_module._parse_nics(_build_combined_efo_workbook()) + targets = obr_module._parse_nics(_build_combined_efo_workbook()) assert "obr/ni_class_3" not in {t.name for t in targets} assert "ni_class_3" not in {t.variable for t in targets} @@ -146,8 +137,7 @@ def test_parse_nics_intentionally_skips_class_3_in_separate_efo(): a target for it — the underlying PE-UK variable has no signal (#378).""" from policyengine_uk_data.targets.sources import obr as obr_module - with patch.object(obr_module, "load_config", return_value=_FAKE_CONFIG): - targets = obr_module._parse_nics(_build_separate_efo_workbook()) + targets = obr_module._parse_nics(_build_separate_efo_workbook()) assert "obr/ni_class_3" not in {t.name for t in targets} assert "ni_class_3" not in {t.variable for t in targets} @@ -171,8 +161,7 @@ def test_parse_nics_tolerates_alt_label_wording(): ], ) - with patch.object(obr_module, "load_config", return_value=_FAKE_CONFIG): - targets = obr_module._parse_nics(wb) + targets = obr_module._parse_nics(wb) assert {t.variable for t in targets} == { "ni_employee", diff --git a/policyengine_uk_data/tests/test_obr_receipts_sheet.py b/policyengine_uk_data/tests/test_obr_receipts_sheet.py index d8b2928db..196420960 100644 --- a/policyengine_uk_data/tests/test_obr_receipts_sheet.py +++ b/policyengine_uk_data/tests/test_obr_receipts_sheet.py @@ -91,6 +91,9 @@ def _receipts_wb(*, populate: bool) -> openpyxl.Workbook: wb.create_sheet("3.4")["B2"] = "3.4 Income tax (accrued basis)" ws = wb.create_sheet("3.8") ws["B2"] = "3.8 Current receipts (on a cash basis)" + ws["D3"] = "£ billion" + for col, year in zip("DEFGHIJ", range(2024, 2031)): + ws[f"{col}5"] = f"{year}-{(year + 1) % 100:02d}" labels = [ "National insurance contributions", "Value added tax", @@ -107,26 +110,18 @@ def _receipts_wb(*, populate: bool) -> openpyxl.Workbook: return wb -def test_raises_when_rows_are_found_but_yield_no_values(monkeypatch): +def test_raises_when_rows_are_found_but_yield_no_values(): """A column shift finds every label and still produces nothing. The guard must key on targets produced, not on exceptions caught, or this reproduces the silent drop by a different route. """ - monkeypatch.setattr( - "policyengine_uk_data.targets.sources.obr.load_config", - lambda: {"obr": {"vintage": "test", "efo_receipts": "https://example.invalid"}}, - ) with pytest.raises(ValueError, match="produced values"): _parse_receipts(_receipts_wb(populate=False)) -def test_parses_rows_when_values_are_present(monkeypatch): +def test_parses_rows_when_values_are_present(): """The same sheet with values yields one target per cash-basis row.""" - monkeypatch.setattr( - "policyengine_uk_data.targets.sources.obr.load_config", - lambda: {"obr": {"vintage": "test", "efo_receipts": "https://example.invalid"}}, - ) targets = _parse_receipts(_receipts_wb(populate=True)) names = {target.name for target in targets} assert "obr/capital_gains_tax" in names diff --git a/policyengine_uk_data/tests/test_obr_sources.py b/policyengine_uk_data/tests/test_obr_sources.py new file mode 100644 index 000000000..80ae29675 --- /dev/null +++ b/policyengine_uk_data/tests/test_obr_sources.py @@ -0,0 +1,385 @@ +"""Hash-bound OBR workbooks, header-read years and required target coverage. + +The OBR targets come only from the workbook bytes the active manifest names. +These tests pin that: +- other bytes, or a workbook with another title, are refused; +- year columns follow the header row, not the column letter; +- a missing target or year fails; +- ingesting a vintage reproduces the build's own targets. +""" + +import io +import shutil + +import openpyxl +import pytest +from hypothesis import HealthCheck, given, settings +from hypothesis import strategies as st + +from policyengine_uk_data.storage import STORAGE_FOLDER +from policyengine_uk_data.targets import obr_sources +from policyengine_uk_data.targets.obr_sources import ( + SourceError, + active_manifest, + check_coverage, + coverage_problems, + ingest, + load_workbook, + parse_manifest, + read_row, + verified_bytes, + year_columns, +) +from policyengine_uk_data.targets.sources import obr + +MANIFEST = active_manifest() + + +def _workbook_targets(receipts, expenditure, manifest=MANIFEST): + return [ + *obr._parse_receipts(receipts, manifest), + *obr._parse_nics(receipts, manifest), + *obr._parse_council_tax(expenditure, manifest), + *obr._parse_welfare(expenditure, manifest), + *obr._parse_tv_licence(expenditure, manifest), + ] + + +def _copy_storage(tmp_path): + for entry in MANIFEST.files.values(): + target = tmp_path / entry.path + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(STORAGE_FOLDER / entry.path, target) + return tmp_path + + +def test_the_build_reads_the_manifest_files_and_names_their_vintage(): + assert MANIFEST.vintage == "march_2026" + for kind, entry in MANIFEST.files.items(): + assert verified_bytes(entry) + assert obr_sources.workbook_title(load_workbook(kind)) == entry.title + targets = obr.get_targets() + built = [t for t in targets if t.forecast_vintage == MANIFEST.vintage] + assert len(built) == 25 + assert {t.reference_url for t in built} == {f.url for f in MANIFEST.files.values()} + check_coverage(targets, MANIFEST) + + +@settings( + max_examples=25, + deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture], +) +@given(data=st.data()) +def test_any_change_to_the_bytes_is_refused(tmp_path, data): + storage = _copy_storage(tmp_path) + entry = MANIFEST.files[data.draw(st.sampled_from(["receipts", "expenditure"]))] + path = storage / entry.path + original = (STORAGE_FOLDER / entry.path).read_bytes() + kind = data.draw(st.sampled_from(["flip", "truncate", "append"])) + if kind == "flip": + i = data.draw(st.integers(0, len(original) - 1)) + changed = original[:i] + bytes([original[i] ^ 0x01]) + original[i + 1 :] + elif kind == "truncate": + changed = original[: data.draw(st.integers(0, len(original) - 1))] + else: + changed = original + data.draw(st.binary(min_size=1, max_size=16)) + path.write_bytes(changed) + with pytest.raises(SourceError, match="not the file its manifest names"): + verified_bytes(entry, storage) + + +def test_a_missing_workbook_fails_the_targets(tmp_path): + with pytest.raises(SourceError, match="missing"): + obr.workbook_targets(MANIFEST, tmp_path) + + +def test_a_workbook_titled_for_another_vintage_is_refused(tmp_path): + """Bytes that match a manifest but carry another vintage's title fail.""" + entry = MANIFEST.files["receipts"] + wb = openpyxl.load_workbook(STORAGE_FOLDER / entry.path) + wb["Contents"]["B2"] = entry.title.replace("March 2026", "November 2025") + buffer = io.BytesIO() + wb.save(buffer) + data = buffer.getvalue() + (tmp_path / "x.xlsx").write_bytes(data) + relabelled = obr_sources.SourceFile( + kind="receipts", + path="x.xlsx", + sha256=obr_sources.sha256_bytes(data), + size_bytes=len(data), + title=entry.title, + url=entry.url, + retrieved="test", + ) + manifest = obr_sources.ObrManifest( + **{**MANIFEST.__dict__, "files": {**MANIFEST.files, "receipts": relabelled}} + ) + with pytest.raises(SourceError, match="November 2025"): + load_workbook("receipts", manifest, tmp_path) + + +# The March 2026 tables' year columns as the loader hard-coded them before it +# read the headers: an independent reader for a differential check. +_FIXED = { + "3.4": dict(zip("CDEFGHI", range(2024, 2031))), + "3.8": dict(zip("DEFGHIJ", range(2024, 2031))), + "4.1": dict(zip("CDEFGHI", range(2024, 2031))), + "4.9": dict(zip("CDEFGHI", range(2024, 2031))), + "4.19": dict(zip("CDEFGHI", range(2024, 2031))), +} +_ROWS = { + "obr/income_tax": ("receipts", "3.4", "Income tax (gross of tax credits)"), + "obr/ni_employer": ("receipts", "3.4", "Class 1 Employer NICs"), + "obr/vat": ("receipts", "3.8", "Value added tax"), + "obr/sdlt": ("receipts", "3.8", "Stamp duty land tax"), + "obr/council_tax_wales": ("expenditure", "4.1", "Wales council tax receipts"), + "obr/child_benefit": ("expenditure", "4.9", "Child benefit"), + "obr/state_pension": ("expenditure", "4.9", "State pension"), +} + + +def test_header_read_years_match_the_fixed_columns_on_march_2026(): + books = {kind: load_workbook(kind) for kind in ("receipts", "expenditure")} + targets = { + t.name: t for t in _workbook_targets(books["receipts"], books["expenditure"]) + } + for sheet, fixed in _FIXED.items(): + ws = books["receipts" if sheet.startswith("3") else "expenditure"][sheet] + assert year_columns(ws, 2024, 2030) == fixed, sheet + for name, (kind, sheet, label) in _ROWS.items(): + ws = books[kind][sheet] + row = obr._find_row(ws, label, max_row=80) + expected = { + year: ws[f"{c}{row}"].value * 1e9 for c, year in _FIXED[sheet].items() + } + assert targets[name].values == pytest.approx(expected), name + + +def _sheet(header_row, first_column, years, values_by_year, unit="£ billion"): + wb = openpyxl.Workbook() + ws = wb.active + ws["A1"] = unit + for offset, year in enumerate(years): + column = first_column + offset + ws.cell(row=header_row, column=column, value=f"{year}-{(year + 1) % 100:02d}") + ws.cell(row=12, column=column, value=values_by_year[year]) + return ws + + +@settings(max_examples=200, deadline=None) +@given( + header_row=st.integers(2, 10), + first_column=st.integers(2, 6), + first_year=st.integers(2020, 2030), + length=st.integers(3, 8), + data=st.data(), +) +def test_values_follow_their_header_whatever_the_column_order( + header_row, first_column, first_year, length, data +): + years = list(range(first_year, first_year + length)) + order = data.draw(st.permutations(years)) + values = {year: float(year - 2000) for year in years} + ws = _sheet(header_row, first_column, order, values) + manifest = obr_sources.ObrManifest( + **{**MANIFEST.__dict__, "first_year": years[0], "last_year": years[-1]} + ) + assert read_row(ws, 12, manifest) == {y: v * 1e9 for y, v in values.items()} + # Declaring the vintage a year later or earlier than the table fails. + for shift in (-1, 1): + with pytest.raises(SourceError, match="the manifest declares"): + year_columns(ws, years[0] + shift, years[-1] + shift) + + +def test_a_table_without_headers_or_unit_fails(): + ws = openpyxl.Workbook().active + ws["C12"] = 1.0 + with pytest.raises(SourceError, match="financial-year headers"): + year_columns(ws, 2024, 2030) + ws = _sheet( + 5, 3, range(2024, 2031), {y: 1.0 for y in range(2024, 2031)}, unit="£ million" + ) + with pytest.raises(SourceError, match="unit label"): + read_row(ws, 12, MANIFEST) + + +def test_a_repeated_or_malformed_year_header_fails(): + ws = _sheet(5, 3, [2024, 2025, 2025], {2024: 1.0, 2025: 1.0}) + with pytest.raises(SourceError, match="heads two columns"): + year_columns(ws, 2024, 2025) + ws = openpyxl.Workbook().active + ws["C5"] = "2024-26" + with pytest.raises(SourceError, match="not one financial year"): + year_columns(ws, 2024, 2024) + + +@pytest.fixture(scope="module") +def march_targets(): + return obr.workbook_targets() + + +@settings( + max_examples=60, + deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture], +) +@given(data=st.data()) +def test_losing_any_year_of_any_target_fails_coverage(march_targets, data): + targets = [t for t in march_targets if t.forecast_vintage == MANIFEST.vintage] + victim = data.draw(st.sampled_from(targets)) + year = data.draw(st.sampled_from(sorted(victim.values))) + damaged = [ + t.model_copy( + update={"values": {y: v for y, v in t.values.items() if y != year}} + ) + if t is victim + else t + for t in targets + ] + problems = coverage_problems(damaged, MANIFEST) + assert problems == [f"{victim.name} has no value for {year}"] + + +@settings( + max_examples=40, + deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture], +) +@given(data=st.data()) +def test_losing_any_required_target_fails_coverage(march_targets, data): + name = data.draw(st.sampled_from(MANIFEST.required)) + problems = coverage_problems([t for t in march_targets if t.name != name], MANIFEST) + assert problems == [f"{name} was not built"] + + +def test_a_layout_change_in_the_workbook_fails_the_build(): + """A renamed row, a formula in a value cell, or a lost self-employed line.""" + receipts, expenditure = load_workbook("receipts"), load_workbook("expenditure") + ws = receipts[obr._find_receipts_sheet(receipts).title] + ws[f"B{obr._find_row(ws, 'Value added tax', max_row=80)}"] = "VAT (renamed)" + ws49 = expenditure["4.9"] + ws49[f"F{obr._find_row(ws49, 'Child benefit', max_row=55)}"] = "=1+1" + ws34 = receipts["3.4"] + ws34[ + f"B{obr._find_row(ws34, 'Class 4 and Class 2 Self employed NICs', max_row=40)}" + ] = "x" + problems = coverage_problems(_workbook_targets(receipts, expenditure), MANIFEST) + assert "obr/vat was not built" in problems + assert "obr/child_benefit has no value for 2027" in problems + assert ( + "none of obr/ni_self_employed or obr/ni_class_2+obr/ni_class_4 was built" + in problems + ) + + +def test_a_target_missing_from_the_manifest_fails(march_targets): + manifest = obr_sources.ObrManifest( + **{ + **MANIFEST.__dict__, + "required": tuple( + n for n in MANIFEST.required if n != "obr/tv_licence_fee" + ), + } + ) + assert coverage_problems(march_targets, manifest) == [ + "obr/tv_licence_fee was built but is not in the manifest" + ] + + +def _sources(tmp_path): + src = tmp_path / "downloads" + src.mkdir() + out = {} + for kind, entry in MANIFEST.files.items(): + out[kind] = src / f"{kind}.xlsx" + shutil.copyfile(STORAGE_FOLDER / entry.path, out[kind]) + return out + + +def _ingest(tmp_path, **overrides): + (tmp_path / "manifests").mkdir(exist_ok=True) + arguments = dict( + vintage="copy_2026", + label="March 2026", + sources=overrides.pop("sources", None) or _sources(tmp_path), + urls={k: f.url for k, f in MANIFEST.files.items()}, + first_year=2024, + last_year=2030, + current=MANIFEST, + storage=tmp_path / "storage", + manifest_dir=tmp_path / "manifests", + retrieved="test", + ) + arguments.update(overrides) + return ingest(**arguments) + + +def test_ingesting_the_same_workbooks_reproduces_the_targets(tmp_path, march_targets): + manifest, path, diff = _ingest(tmp_path) + assert path.name == "obr_copy_2026.yaml" + assert manifest.vintage == "copy_2026" + assert {k: f.sha256 for k, f in manifest.files.items()} == { + k: f.sha256 for k, f in MANIFEST.files.items() + } + assert manifest.files["receipts"].path == "obr_efo/copy_2026/receipts.xlsx" + assert diff and all(row["old"] == row["new"] for row in diff) + again = obr.workbook_targets(manifest, tmp_path / "storage") + assert {t.name: t.values for t in again} == { + t.name: t.values for t in march_targets + } + assert {t.forecast_vintage for t in again} == {"copy_2026"} + + +def test_ingest_refuses_a_workbook_that_does_not_name_the_vintage(tmp_path): + with pytest.raises(SourceError, match="not the October 2026 EFO receipts"): + _ingest(tmp_path, label="October 2026") + + +def test_ingest_refuses_years_the_tables_do_not_head_and_keeps_nothing(tmp_path): + with pytest.raises(SourceError, match="the manifest declares 2025 to 2031"): + _ingest(tmp_path, first_year=2025, last_year=2031) + assert not (tmp_path / "storage" / "obr_efo" / "copy_2026").exists() + assert not list((tmp_path / "manifests").iterdir()) + + +def test_ingest_refuses_the_page_obr_serves_to_scripts(tmp_path): + sources = _sources(tmp_path) + sources["receipts"].write_bytes( + b"No Access" + ) + with pytest.raises(SourceError, match="download the workbook in a browser"): + _ingest(tmp_path, sources=sources) + + +def test_verify_command_passes_on_the_active_manifest(capsys): + assert obr_sources.main(["verify"]) == 0 + assert "both workbooks verified, 25 targets" in capsys.readouterr().out + + +@pytest.mark.parametrize( + "change, match", + [ + ({"schema_version": 2}, "schema_version"), + ({"unit": "£ million"}, "unit"), + ({"first_year": 2031}, "first_year"), + ({"targets": {"required": ["obr/vat", "obr/vat"]}}, "twice"), + ({"targets": {"required": []}}, "no required"), + ], +) +def test_malformed_manifests_are_refused(change, match): + import yaml + + raw = yaml.safe_load(MANIFEST.path.read_text()) + raw.update(change) + with pytest.raises(SourceError, match=match): + parse_manifest(raw) + + +def test_a_bad_hash_in_the_manifest_is_refused(): + import yaml + + raw = yaml.safe_load(MANIFEST.path.read_text()) + raw["files"]["receipts"]["sha256"] = "abc" + with pytest.raises(SourceError, match="64 hex"): + parse_manifest(raw) diff --git a/policyengine_uk_data/tests/test_obr_universal_credit_target.py b/policyengine_uk_data/tests/test_obr_universal_credit_target.py index 8fcdb51e0..216dd551d 100644 --- a/policyengine_uk_data/tests/test_obr_universal_credit_target.py +++ b/policyengine_uk_data/tests/test_obr_universal_credit_target.py @@ -75,6 +75,9 @@ def _table(rows: list[tuple]) -> openpyxl.Workbook: wb = openpyxl.Workbook() ws = wb.active ws.title = "4.9" + ws["C3"] = "£ billion" + for column, year in YEAR_COLUMNS.items(): + ws[f"{column}5"] = f"{year}-{(year + 1) % 100:02d}" for i, (label, value) in enumerate(rows, start=6): ws[f"B{i}"] = label if value is not None: From 98ad844665fde3a12835a98d7cc895dab2b41a7d Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 07:57:40 -0400 Subject: [PATCH 2/7] Bind EFO vintages to their workbook titles; refuse rows with their own unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sol review of #592: - The manifest's vintage, its label and both workbook titles must agree (march_2026, "March 2026", "March 2026 Economic and fiscal outlook ... receipts"), at load and at ingest, so verified March bytes cannot be labelled October. - A row whose label states its own unit ("Standard colour licence fee (£)") is refused under a "£ billion unless otherwise stated" table, and the TV licence target reads the "BBC licence fee receipts" row only. - Ingest checks both workbooks and parses them in a staging folder before copying anything, and never replaces an ingested vintage. - Coverage checks every workbook target, including one without the vintage. - The NICs docstring no longer claims Class 3 or that partial results pass. All 29 OBR targets are still identical to main's on the March bytes. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/targets/obr_sources.py | 141 ++++++++++++------ policyengine_uk_data/targets/sources/obr.py | 44 +++--- .../tests/test_obr_sources.py | 87 +++++++++-- 3 files changed, 197 insertions(+), 75 deletions(-) diff --git a/policyengine_uk_data/targets/obr_sources.py b/policyengine_uk_data/targets/obr_sources.py index 4620e580d..03afbbd98 100644 --- a/policyengine_uk_data/targets/obr_sources.py +++ b/policyengine_uk_data/targets/obr_sources.py @@ -36,7 +36,6 @@ import hashlib import io import re -import shutil import sys from dataclasses import dataclass from datetime import date @@ -134,6 +133,8 @@ def parse_manifest(raw: Mapping, path: Optional[Path] = None) -> ObrManifest: raise SourceError(f"{where}: a required target is listed twice") if not required: raise SourceError(f"{where}: no required targets") + vintage, label = str(raw["vintage"]), str(raw["label"]) + check_names(vintage, label, {kind: f.title for kind, f in files.items()}, where) return ObrManifest( vintage=str(raw["vintage"]), label=str(raw["label"]), @@ -147,6 +148,28 @@ def parse_manifest(raw: Mapping, path: Optional[Path] = None) -> ObrManifest: ) +def check_names(vintage: str, label: str, titles: Mapping[str, str], where: str): + """The vintage, its label and each workbook's own title must agree. + + ``march_2026`` must be labelled "March 2026", and each workbook titled + "March 2026 Economic and fiscal outlook ..." ending with its kind. The + title is read from the verified bytes, so the bytes decide the vintage. + """ + if not re.fullmatch(r"[a-z]+_\d{4}", vintage): + raise SourceError(f"{where}: vintage must look like october_2026") + if label.lower().replace(" ", "_") != vintage: + raise SourceError(f"{where}: label {label!r} does not name vintage {vintage!r}") + for kind, title in titles.items(): + lowered = title.strip().lower() + if not lowered.startswith(f"{label.lower()} economic and fiscal outlook") or ( + not lowered.endswith(kind) + ): + raise SourceError( + f"{where}: the {kind} workbook is titled {title!r}, which is not " + f"the {label} EFO {kind} tables" + ) + + def load_manifest(path) -> ObrManifest: path = Path(path) if not path.is_absolute(): @@ -266,8 +289,27 @@ def unit_scale(ws, unit: str) -> float: raise SourceError(f"{ws.title}: no {unit!r} unit label in rows 1-{_UNIT_ROWS}") +# A row label giving its own unit, as table 4.19's "Standard colour licence +# fee (£)" or "Number of UK households (m)" do under a "£ billion unless +# otherwise stated" table. +_ROW_UNIT = re.compile( + r"\(\s*(?:£|£m|£bn|£ ?million|£ ?billion|m|bn|million|thousands?|per cent|%|p|pence)\s*\)" + r"|\bper cent\b|%" +) + + def read_row(ws, row: int, manifest: ObrManifest) -> dict: - """A table row's numeric values by year, in pounds.""" + """A table row's numeric values by year, in pounds. + + Refuses a row whose label states a unit of its own: the table's unit is + applied to every row read. + """ + label = str(ws[f"B{row}"].value or "") + if _ROW_UNIT.search(label): + raise SourceError( + f"{ws.title}!B{row}: {label!r} states its own unit, not the table's " + f"{manifest.unit}" + ) scale = unit_scale(ws, manifest.unit) out = {} for column, year in year_columns( @@ -282,14 +324,18 @@ def read_row(ws, row: int, manifest: ObrManifest) -> dict: def coverage_problems(targets: Sequence, manifest: ObrManifest) -> list: """Everything wrong with the targets built from a vintage's workbooks. - Targets built from the workbooks are those carrying the vintage. Each - required target must be present with every year of the manifest; one - set of each ``any_of`` group must be present; and no workbook target - may be missing from the manifest, so the manifest stays the complete - list. + ``targets`` are all the targets the workbooks yielded. Each must carry + the manifest's vintage; each required target must be present with every + year of the manifest; one set of each ``any_of`` group must be present; + and no workbook target may be missing from the manifest, so the manifest + stays the complete list. """ - built = {t.name: t for t in targets if t.forecast_vintage == manifest.vintage} - problems = [] + built = {t.name: t for t in targets} + problems = [ + f"{t.name} carries vintage {t.forecast_vintage!r}, not {manifest.vintage!r}" + for t in targets + if t.forecast_vintage != manifest.vintage + ] for name in manifest.required: if name not in built: problems.append(f"{name} was not built") @@ -379,50 +425,51 @@ def ingest( manifest_dir: Optional[Path] = None, retrieved: Optional[str] = None, ): - """Adopt a vintage's two workbooks. Returns (manifest, manifest path, diff).""" + """Adopt a vintage's two workbooks. Returns (manifest, manifest path, diff). + + Everything is checked in a staging folder first: both workbooks read + and carry the vintage's titles, and the build's parsers yield every + required target and year from them. Only then are the files copied + into storage and the manifest written. A vintage already in storage or + in the manifests folder is never replaced. + """ + import tempfile + from policyengine_uk_data.targets.sources import obr current = current or active_manifest() - storage = storage or _storage() - manifest_dir = manifest_dir or MANIFEST_DIR + storage = Path(storage or _storage()) + manifest_dir = Path(manifest_dir or MANIFEST_DIR) retrieved = retrieved or f"ingested {date.today().isoformat()}" - if not re.fullmatch(r"[a-z]+_\d{4}", vintage): - raise SourceError("vintage must look like october_2026") - files = {} + destination = storage / "obr_efo" / vintage + path = manifest_dir / f"obr_{vintage}.yaml" + if destination.exists() or path.exists(): + raise SourceError( + f"{vintage} is already ingested ({destination} or {path} exists); " + "remove it deliberately before ingesting again" + ) + files, titles, data = {}, {}, {} for kind in KINDS: - data = Path(sources[kind]).read_bytes() + data[kind] = Path(sources[kind]).read_bytes() try: - wb = openpyxl.load_workbook(io.BytesIO(data), data_only=False) + wb = openpyxl.load_workbook(io.BytesIO(data[kind]), data_only=False) except Exception as error: raise SourceError( f"{sources[kind]} is not an xlsx workbook ({type(error).__name__}: " f"{error}); obr.uk serves an HTML page to scripted downloads, so " "download the workbook in a browser" ) from error - title = workbook_title(wb) - if not title.startswith( - f"{label} Economic and fiscal outlook" - ) or not title.endswith(kind): - raise SourceError( - f"{sources[kind]} is titled {title!r}, which is not the {label} " - f"EFO {kind} tables" - ) - relative = f"obr_efo/{vintage}/{kind}.xlsx" - target = storage / relative - target.parent.mkdir(parents=True, exist_ok=True) - shutil.copyfile(sources[kind], target) + titles[kind] = workbook_title(wb) files[kind] = SourceFile( kind=kind, - path=relative, - sha256=sha256_bytes(data), - size_bytes=len(data), - title=title, + path=f"obr_efo/{vintage}/{kind}.xlsx", + sha256=sha256_bytes(data[kind]), + size_bytes=len(data[kind]), + title=titles[kind], url=urls.get(kind, ""), retrieved=retrieved, ) - alternatives_raw = [ - [sorted(group) for group in groups] for groups in current.alternatives - ] + check_names(vintage, label, titles, "ingest") manifest = ObrManifest( vintage=vintage, label=label, @@ -433,15 +480,23 @@ def ingest( required=current.required, alternatives=current.alternatives, ) - try: + with tempfile.TemporaryDirectory() as staging: + staging = Path(staging) + for kind in KINDS: + staged = staging / files[kind].path + staged.parent.mkdir(parents=True, exist_ok=True) + staged.write_bytes(data[kind]) # Raises unless the new workbooks yield every target and year required. - new = obr.workbook_targets(manifest, storage) - except Exception: - shutil.rmtree(storage / "obr_efo" / vintage, ignore_errors=True) - raise + new = obr.workbook_targets(manifest, staging) old = obr.workbook_targets(current) - path = Path(manifest_dir) / f"obr_{vintage}.yaml" - path.write_text(_manifest_yaml(manifest, alternatives_raw)) + text = _manifest_yaml( + manifest, + [[sorted(group) for group in groups] for groups in current.alternatives], + ) + destination.mkdir(parents=True) + for kind in KINDS: + (storage / files[kind].path).write_bytes(data[kind]) + path.write_text(text) return load_manifest(path), path, target_diff(old, new) diff --git a/policyengine_uk_data/targets/sources/obr.py b/policyengine_uk_data/targets/sources/obr.py index 60f2be1d1..a8ac80541 100644 --- a/policyengine_uk_data/targets/sources/obr.py +++ b/policyengine_uk_data/targets/sources/obr.py @@ -205,14 +205,13 @@ def _parse_nics( ) -> list[Target]: """Parse Table 3.4 (income tax and NICs detail) for all NIC classes. - Covers Class 1 employee and employer (PAYE), plus Classes 2, 3 and 4 - (self-employed flat-rate, voluntary, and self-employed profit-based). - Omitting Classes 2/3/4 biased calibration by ~£2-5B/yr of NIC receipts - — the omission pushed self-employment income downward to compensate, - distorting reforms that touch SE income. + Covers Class 1 employee and employer (PAYE) and the self-employed + Classes 2 and 4, as one combined line or as separate rows. Class 3 + (voluntary) is left out: no dataset populates ni_class_3 (#378). Where a row label is not found the parser logs a warning and skips that - variable, so partial matches still produce useful targets. + variable; the manifest's required targets then fail the build unless one + of the self-employed alternatives was found. """ manifest = manifest or active_manifest() vintage, ref = manifest.vintage, manifest.files["receipts"].url @@ -447,23 +446,22 @@ def _parse_tv_licence( try: ws = wb["4.19"] - # Find "Licence fee receipts" or "BBC licence fee" - for row_num in range(1, 30): - val = ws[f"B{row_num}"].value - if val and "licence fee" in str(val).lower(): - values = read_row(ws, row_num, manifest) - if values: - return [ - Target( - name="obr/tv_licence_fee", - variable="tv_licence", - source="obr", - unit=Unit.GBP, - values=values, - reference_url=ref, - forecast_vintage=vintage, - ) - ] + # The receipts row only: neighbouring rows such as "Standard colour + # licence fee (£)" share the words but not the unit. + row_num = _find_row(ws, "BBC licence fee receipts", max_row=30) + values = read_row(ws, row_num, manifest) + if values: + return [ + Target( + name="obr/tv_licence_fee", + variable="tv_licence", + source="obr", + unit=Unit.GBP, + values=values, + reference_url=ref, + forecast_vintage=vintage, + ) + ] except (KeyError, ValueError): logger.warning("OBR: TV licence table not found") diff --git a/policyengine_uk_data/tests/test_obr_sources.py b/policyengine_uk_data/tests/test_obr_sources.py index 80ae29675..d541befc2 100644 --- a/policyengine_uk_data/tests/test_obr_sources.py +++ b/policyengine_uk_data/tests/test_obr_sources.py @@ -62,7 +62,7 @@ def test_the_build_reads_the_manifest_files_and_names_their_vintage(): built = [t for t in targets if t.forecast_vintage == MANIFEST.vintage] assert len(built) == 25 assert {t.reference_url for t in built} == {f.url for f in MANIFEST.files.values()} - check_coverage(targets, MANIFEST) + check_coverage(built, MANIFEST) @settings( @@ -289,7 +289,7 @@ def test_a_target_missing_from_the_manifest_fails(march_targets): def _sources(tmp_path): src = tmp_path / "downloads" - src.mkdir() + src.mkdir(exist_ok=True) out = {} for kind, entry in MANIFEST.files.items(): out[kind] = src / f"{kind}.xlsx" @@ -300,7 +300,7 @@ def _sources(tmp_path): def _ingest(tmp_path, **overrides): (tmp_path / "manifests").mkdir(exist_ok=True) arguments = dict( - vintage="copy_2026", + vintage="march_2026", label="March 2026", sources=overrides.pop("sources", None) or _sources(tmp_path), urls={k: f.url for k, f in MANIFEST.files.items()}, @@ -317,29 +317,32 @@ def _ingest(tmp_path, **overrides): def test_ingesting_the_same_workbooks_reproduces_the_targets(tmp_path, march_targets): manifest, path, diff = _ingest(tmp_path) - assert path.name == "obr_copy_2026.yaml" - assert manifest.vintage == "copy_2026" + assert path.name == "obr_march_2026.yaml" + assert manifest.vintage == "march_2026" assert {k: f.sha256 for k, f in manifest.files.items()} == { k: f.sha256 for k, f in MANIFEST.files.items() } - assert manifest.files["receipts"].path == "obr_efo/copy_2026/receipts.xlsx" + assert manifest.files["receipts"].path == "obr_efo/march_2026/receipts.xlsx" assert diff and all(row["old"] == row["new"] for row in diff) again = obr.workbook_targets(manifest, tmp_path / "storage") assert {t.name: t.values for t in again} == { t.name: t.values for t in march_targets } - assert {t.forecast_vintage for t in again} == {"copy_2026"} + assert {t.forecast_vintage for t in again} == {"march_2026"} def test_ingest_refuses_a_workbook_that_does_not_name_the_vintage(tmp_path): + """Sol review of #592, P1: March bytes cannot be adopted as October.""" with pytest.raises(SourceError, match="not the October 2026 EFO receipts"): - _ingest(tmp_path, label="October 2026") + _ingest(tmp_path, vintage="october_2026", label="October 2026") + with pytest.raises(SourceError, match="does not name vintage"): + _ingest(tmp_path, vintage="october_2026", label="March 2026") def test_ingest_refuses_years_the_tables_do_not_head_and_keeps_nothing(tmp_path): with pytest.raises(SourceError, match="the manifest declares 2025 to 2031"): _ingest(tmp_path, first_year=2025, last_year=2031) - assert not (tmp_path / "storage" / "obr_efo" / "copy_2026").exists() + assert not (tmp_path / "storage" / "obr_efo" / "march_2026").exists() assert not list((tmp_path / "manifests").iterdir()) @@ -383,3 +386,69 @@ def test_a_bad_hash_in_the_manifest_is_refused(): raw["files"]["receipts"]["sha256"] = "abc" with pytest.raises(SourceError, match="64 hex"): parse_manifest(raw) + + +def test_a_manifest_cannot_relabel_verified_bytes(): + """Sol review of #592, P1: the vintage, label and titles must agree.""" + import yaml + + raw = yaml.safe_load(MANIFEST.path.read_text()) + raw.update(vintage="october_2026", label="October 2026") + with pytest.raises(SourceError, match="not the October 2026 EFO"): + parse_manifest(raw) + raw = yaml.safe_load(MANIFEST.path.read_text()) + raw.update(label="October 2026") + with pytest.raises(SourceError, match="does not name vintage"): + parse_manifest(raw) + + +def test_a_row_stating_its_own_unit_is_refused(): + """Sol review of #592, P1: "£ billion unless otherwise stated" tables.""" + expenditure = load_workbook("expenditure") + ws = expenditure["4.19"] + row = obr._find_row(ws, "Standard colour licence fee (£)", max_row=30) + with pytest.raises(SourceError, match="states its own unit"): + read_row(ws, row, MANIFEST) + # A renamed receipts row no longer falls through to the fee row. + ws[f"B{obr._find_row(ws, 'BBC licence fee receipts', max_row=30)}"] = ( + "BBC television licensing receipts" + ) + receipts = load_workbook("receipts") + problems = coverage_problems(_workbook_targets(receipts, expenditure), MANIFEST) + assert problems == ["obr/tv_licence_fee was not built"] + + +def test_ingest_never_replaces_an_ingested_vintage(tmp_path): + """Sol review of #592, P2: a retry cannot delete an adopted vintage.""" + _ingest(tmp_path) + kept = ( + tmp_path / "storage" / "obr_efo" / "march_2026" / "receipts.xlsx" + ).read_bytes() + with pytest.raises(SourceError, match="already ingested"): + _ingest(tmp_path, first_year=2025, last_year=2031) + assert ( + tmp_path / "storage" / "obr_efo" / "march_2026" / "receipts.xlsx" + ).read_bytes() == kept + assert (tmp_path / "manifests" / "obr_march_2026.yaml").exists() + + +def test_ingest_checks_both_workbooks_before_copying_either(tmp_path): + sources = _sources(tmp_path) + sources["expenditure"].write_bytes(b"not a workbook") + with pytest.raises(SourceError, match="not an xlsx workbook"): + _ingest(tmp_path, sources=sources) + assert not (tmp_path / "storage").exists() + + +def test_a_workbook_target_without_the_vintage_fails_coverage(march_targets): + """Sol review of #592, P2: an unversioned target cannot skip the checks.""" + extra = march_targets[0].model_copy( + update={ + "name": "obr/unversioned", + "forecast_vintage": None, + "values": {2024: 1.0}, + } + ) + problems = coverage_problems(list(march_targets) + [extra], MANIFEST) + assert "obr/unversioned carries vintage None, not 'march_2026'" in problems + assert "obr/unversioned was built but is not in the manifest" in problems From 2fb5cfa1cbcff806193d7eac9b50b6ff70ff75ad Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 08:19:54 -0400 Subject: [PATCH 3/7] Record each browser download's time, and rehearse ingest with --dry-run obr.uk refuses scripted downloads, so on Budget day a person downloads the two EFO workbooks in a browser. The manifest now records each workbook's download time with its SHA-256 and size: the file's modification time, which a browser sets on saving, unless --downloaded-at is given. `ingest --dry-run` checks everything and prints titles, hashes, sizes, download times and every target's change, writing nothing; on the active vintage it says whether the bytes match the committed workbooks, so a March 2026 download rehearses the Budget-day path. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/targets/obr_sources.py | 103 +++++++++++++++--- .../tests/test_obr_sources.py | 75 +++++++++++++ 2 files changed, 165 insertions(+), 13 deletions(-) diff --git a/policyengine_uk_data/targets/obr_sources.py b/policyengine_uk_data/targets/obr_sources.py index 03afbbd98..10fc4bc16 100644 --- a/policyengine_uk_data/targets/obr_sources.py +++ b/policyengine_uk_data/targets/obr_sources.py @@ -19,15 +19,23 @@ python -m policyengine_uk_data.targets.obr_sources ingest \\ --vintage october_2026 --label "October 2026" \\ --receipts receipts.xlsx --expenditure expenditure.xlsx \\ - --first-year 2025 --last-year 2031 + --first-year 2025 --last-year 2031 --dry-run obr.uk refuses scripted downloads (HTTP 403 to GitHub Actions runners, as -11e41f9d records, and to a developer machine on 2026-10-15), so download -the two workbooks in a browser and pass their paths. ``ingest`` copies them under ``storage/obr_efo//``, -checks that each workbook's title names the vintage, parses them with the -build's own parsers, and requires the targets the current manifest -requires. It then writes the manifest and prints each target's change from -the current vintage. Point ``sources.yaml`` at the new manifest to adopt it. +11e41f9d records, and to a developer machine on 2026-10-15), so a person +downloads the two "detailed forecast tables" workbooks in a browser and +passes their paths. ``--dry-run`` checks everything and prints each +workbook's title, SHA-256, size and download time and every target's old +and new value, writing nothing. It also says whether the bytes match the +committed workbooks when the vintage is the active one, which makes a +March 2026 download a rehearsal. Without ``--dry-run``, ``ingest`` parses +the workbooks in a staging folder and then copies them under +``storage/obr_efo//``. It requires each workbook's title to name +the vintage, and requires the targets the current manifest requires. It +writes the manifest with each file's SHA-256, size and download time (the +file's modification time unless ``--downloaded-at`` is given) and never +replaces an ingested vintage. Point ``sources.yaml`` at the new manifest to +adopt it. """ from __future__ import annotations @@ -38,7 +46,7 @@ import re import sys from dataclasses import dataclass -from datetime import date +from datetime import date, datetime, timezone from functools import lru_cache from pathlib import Path from typing import Mapping, Optional, Sequence @@ -71,6 +79,8 @@ class SourceFile: title: str # the workbook's own title, cell B2 of its Contents sheet url: str retrieved: str + # When the file was downloaded (ISO 8601 with a UTC offset), if known. + downloaded_at: Optional[str] = None @dataclass(frozen=True) @@ -120,6 +130,7 @@ def parse_manifest(raw: Mapping, path: Optional[Path] = None) -> ObrManifest: title=str(entry["title"]), url=str(entry["url"]), retrieved=str(entry["retrieved"]), + downloaded_at=_timestamp(entry.get("downloaded_at"), f"{where}: {kind}"), ) required, alternatives = [], [] for item in (raw.get("targets") or {}).get("required") or []: @@ -148,6 +159,22 @@ def parse_manifest(raw: Mapping, path: Optional[Path] = None) -> ObrManifest: ) +def _timestamp(value, where: str) -> Optional[str]: + """An ISO 8601 time with a UTC offset, or None.""" + if value is None: + return None + text = value.isoformat() if isinstance(value, datetime) else str(value) + try: + parsed = datetime.fromisoformat(text) + except ValueError as error: + raise SourceError( + f"{where}: downloaded_at {text!r} is not an ISO 8601 time" + ) from error + if parsed.tzinfo is None: + raise SourceError(f"{where}: downloaded_at {text!r} needs a UTC offset") + return parsed.isoformat() + + def check_names(vintage: str, label: str, titles: Mapping[str, str], where: str): """The vintage, its label and each workbook's own title must agree. @@ -396,6 +423,7 @@ def _manifest_yaml(manifest: ObrManifest, alternatives_raw) -> str: "title": f.title, "url": f.url, "retrieved": f.retrieved, + **({"downloaded_at": f.downloaded_at} if f.downloaded_at else {}), } for kind, f in manifest.files.items() }, @@ -424,9 +452,17 @@ def ingest( storage: Optional[Path] = None, manifest_dir: Optional[Path] = None, retrieved: Optional[str] = None, + downloaded_at: Optional[Mapping[str, str]] = None, + dry_run: bool = False, ): """Adopt a vintage's two workbooks. Returns (manifest, manifest path, diff). + Each workbook's SHA-256, size and download time go into the manifest. + The download time is ``downloaded_at[kind]`` if given, otherwise the + file's modification time, which a browser sets when it saves a download. + With ``dry_run`` everything is checked and the diff returned, but + nothing is written; the manifest returned is not on disk. + Everything is checked in a staging folder first: both workbooks read and carry the vintage's titles, and the build's parsers yield every required target and year from them. Only then are the files copied @@ -440,10 +476,13 @@ def ingest( current = current or active_manifest() storage = Path(storage or _storage()) manifest_dir = Path(manifest_dir or MANIFEST_DIR) - retrieved = retrieved or f"ingested {date.today().isoformat()}" + retrieved = retrieved or ( + f"downloaded in a browser and ingested on {date.today().isoformat()}" + ) + downloaded_at = dict(downloaded_at or {}) destination = storage / "obr_efo" / vintage path = manifest_dir / f"obr_{vintage}.yaml" - if destination.exists() or path.exists(): + if not dry_run and (destination.exists() or path.exists()): raise SourceError( f"{vintage} is already ingested ({destination} or {path} exists); " "remove it deliberately before ingesting again" @@ -468,6 +507,13 @@ def ingest( title=titles[kind], url=urls.get(kind, ""), retrieved=retrieved, + downloaded_at=_timestamp( + downloaded_at.get(kind) + or datetime.fromtimestamp( + Path(sources[kind]).stat().st_mtime, tz=timezone.utc + ), + f"ingest: {kind}", + ), ) check_names(vintage, label, titles, "ingest") manifest = ObrManifest( @@ -493,6 +539,8 @@ def ingest( manifest, [[sorted(group) for group in groups] for groups in current.alternatives], ) + if dry_run: + return manifest, path, target_diff(old, new) destination.mkdir(parents=True) for kind in KINDS: (storage / files[kind].path).write_bytes(data[kind]) @@ -518,7 +566,17 @@ def main(argv=None) -> int: i.add_argument("--expenditure-url", default="") i.add_argument("--first-year", required=True, type=int) i.add_argument("--last-year", required=True, type=int) - i.add_argument("--retrieved", help="how and when the files were obtained") + i.add_argument("--retrieved", help="how the files were obtained") + i.add_argument( + "--downloaded-at", + help="when the workbooks were downloaded (ISO 8601 with offset); " + "default: each file's modification time", + ) + i.add_argument( + "--dry-run", + action="store_true", + help="check everything and print the change, but write nothing", + ) v = sub.add_parser("verify", help="verify the active manifest's workbooks") v.add_argument("--manifest", help="manifest file (default: the active one)") args = parser.parse_args(argv) @@ -538,12 +596,31 @@ def main(argv=None) -> int: first_year=args.first_year, last_year=args.last_year, retrieved=args.retrieved, + downloaded_at={kind: args.downloaded_at for kind in KINDS} + if args.downloaded_at + else None, + dry_run=args.dry_run, ) - print(f"wrote {path}") + for kind, f in manifest.files.items(): + print( + f"{kind}: {f.title}; sha256 {f.sha256}, {f.size_bytes:,} bytes, " + f"downloaded {f.downloaded_at}" + ) + print(f"{'would write' if args.dry_run else 'wrote'} {path}") print("target,year,current £bn,new £bn") for row in diff: print(f"{row['target']},{row['year']},{_gbp(row['old'])},{_gbp(row['new'])}") - print(f"To adopt it, set obr.manifest in sources.yaml to {path.name}.") + if not args.dry_run: + print(f"To adopt it, set obr.manifest in sources.yaml to {path.name}.") + else: + current = active_manifest() + if manifest.vintage == current.vintage: + for kind, f in manifest.files.items(): + same = f.sha256 == current.files[kind].sha256 + print( + f"{kind}: {'the same bytes as' if same else 'different bytes from'} " + f"the committed {current.label} workbook" + ) return 0 diff --git a/policyengine_uk_data/tests/test_obr_sources.py b/policyengine_uk_data/tests/test_obr_sources.py index d541befc2..5a9d90562 100644 --- a/policyengine_uk_data/tests/test_obr_sources.py +++ b/policyengine_uk_data/tests/test_obr_sources.py @@ -452,3 +452,78 @@ def test_a_workbook_target_without_the_vintage_fails_coverage(march_targets): problems = coverage_problems(list(march_targets) + [extra], MANIFEST) assert "obr/unversioned carries vintage None, not 'march_2026'" in problems assert "obr/unversioned was built but is not in the manifest" in problems + + +def test_ingest_records_each_download_time(tmp_path): + """A browser download is hash-bound and timed: the file's modification + time by default, or the time given.""" + import os + + sources = _sources(tmp_path) + from datetime import datetime, timezone + + when = datetime(2026, 10, 18, 12, tzinfo=timezone.utc).timestamp() + for path in sources.values(): + os.utime(path, (when, when)) + manifest, path, _ = _ingest(tmp_path, sources=sources) + assert {f.downloaded_at for f in manifest.files.values()} == { + "2026-10-18T12:00:00+00:00" + } + assert "downloaded_at: '2026-10-18T12:00:00+00:00'" in path.read_text() + (tmp_path / "again").mkdir() + manifest, _, _ = _ingest( + tmp_path / "again", + downloaded_at={ + "receipts": "2026-10-28T13:05:00+00:00", + "expenditure": "2026-10-28T13:06:00+00:00", + }, + ) + assert manifest.files["receipts"].downloaded_at == "2026-10-28T13:05:00+00:00" + with pytest.raises(SourceError, match="UTC offset"): + _ingest( + tmp_path / "again", + vintage="march_2026", + downloaded_at={ + "receipts": "2026-10-28T13:05:00", + "expenditure": "2026-10-28T13:06:00+00:00", + }, + dry_run=True, + ) + + +def test_a_dry_run_checks_everything_and_writes_nothing(tmp_path): + manifest, path, diff = _ingest(tmp_path, dry_run=True) + assert manifest.vintage == "march_2026" + assert diff and all(row["old"] == row["new"] for row in diff) + assert not path.exists() + assert not (tmp_path / "storage" / "obr_efo").exists() + # A dry run still refuses workbooks that do not carry the vintage. + with pytest.raises(SourceError, match="not the October 2026 EFO receipts"): + _ingest(tmp_path, vintage="october_2026", label="October 2026", dry_run=True) + + +def test_the_rehearsal_command_runs_on_the_committed_march_workbooks(tmp_path, capsys): + """The Budget-day command, end to end, on downloads of the active vintage.""" + sources = _sources(tmp_path) + code = obr_sources.main( + [ + "ingest", + "--vintage", + "march_2026", + "--label", + "March 2026", + "--receipts", + str(sources["receipts"]), + "--expenditure", + str(sources["expenditure"]), + "--first-year", + "2024", + "--last-year", + "2030", + "--dry-run", + ] + ) + out = capsys.readouterr().out + assert code == 0 + assert "the same bytes as the committed March 2026 workbook" in out + assert "would write" in out From f1faff2fbc54eccb2814c3eb3459d21e110fee7e Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 11:09:17 -0400 Subject: [PATCH 4/7] Harden OBR ingest for Budget day (Sol re-review of #592) - Refuse a row whose label gives its own unit in any spelling, or that sits under a unit sub-heading, rather than listing exact forms. - Accept plausible EFO title formats (reordered, a closing note, trailing punctuation, Unicode spaces and dashes) while refusing any title that names another vintage, another report or another kind. - Write the workbooks and the manifest under temporary names and rename them into place; a partial ingest is named on the next try. - Fail coverage when a target is built twice. - Require the download pages on a real ingest. None of the 26 rows the build reads from the March 2026 workbooks (25 targets) is refused. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/targets/obr_sources.py | 137 ++++++++++++++---- .../tests/test_obr_sources.py | 123 ++++++++++++++++ 2 files changed, 231 insertions(+), 29 deletions(-) diff --git a/policyengine_uk_data/targets/obr_sources.py b/policyengine_uk_data/targets/obr_sources.py index 10fc4bc16..6898341fd 100644 --- a/policyengine_uk_data/targets/obr_sources.py +++ b/policyengine_uk_data/targets/obr_sources.py @@ -28,14 +28,17 @@ workbook's title, SHA-256, size and download time and every target's old and new value, writing nothing. It also says whether the bytes match the committed workbooks when the vintage is the active one, which makes a -March 2026 download a rehearsal. Without ``--dry-run``, ``ingest`` parses -the workbooks in a staging folder and then copies them under -``storage/obr_efo//``. It requires each workbook's title to name -the vintage, and requires the targets the current manifest requires. It -writes the manifest with each file's SHA-256, size and download time (the -file's modification time unless ``--downloaded-at`` is given) and never -replaces an ingested vintage. Point ``sources.yaml`` at the new manifest to -adopt it. +March 2026 download a rehearsal. Without ``--dry-run``, ``ingest`` also +needs ``--receipts-url`` and ``--expenditure-url``, the pages the workbooks +came from, which every target cites. It parses the workbooks in a staging +folder and then moves them under ``storage/obr_efo//``. It requires +each workbook's title to name the vintage and no other, and requires the +targets the current manifest requires. It writes the manifest with each +file's SHA-256, size and download time (the file's modification time unless +``--downloaded-at`` is given). Each part is written under a temporary name +and renamed into place, so an interrupted ingest leaves a ``.partial`` file +that the next attempt names. It never replaces an ingested vintage. Point +``sources.yaml`` at the new manifest to adopt it. """ from __future__ import annotations @@ -43,8 +46,10 @@ import argparse import hashlib import io +import os import re import sys +import unicodedata from dataclasses import dataclass from datetime import date, datetime, timezone from functools import lru_cache @@ -175,21 +180,43 @@ def _timestamp(value, where: str) -> Optional[str]: return parsed.isoformat() +_MONTHS = ( + "january|february|march|april|may|june|july|august|september|october|" + "november|december" +) + + +def _normal(text: str) -> str: + """Unicode-normalised lower case, one plain space between words, dashes as -.""" + text = unicodedata.normalize("NFKC", text) + text = re.sub(r"[\u2010-\u2015\u2212]", "-", text) + return re.sub(r"\s+", " ", text).strip().lower() + + def check_names(vintage: str, label: str, titles: Mapping[str, str], where: str): """The vintage, its label and each workbook's own title must agree. - ``march_2026`` must be labelled "March 2026", and each workbook titled - "March 2026 Economic and fiscal outlook ..." ending with its kind. The - title is read from the verified bytes, so the bytes decide the vintage. + ``march_2026`` must be labelled "March 2026". Each workbook's title must + name that vintage and no other, call itself the Economic and fiscal + outlook, and end with its kind, ignoring a closing note in brackets and + punctuation: "March 2026 Economic and fiscal outlook - detailed forecast + tables: receipts" or "Economic and fiscal outlook, March 2026: detailed + forecast tables - receipts (revised)". The title is read from the verified + bytes, so the bytes decide the vintage. """ if not re.fullmatch(r"[a-z]+_\d{4}", vintage): raise SourceError(f"{where}: vintage must look like october_2026") if label.lower().replace(" ", "_") != vintage: raise SourceError(f"{where}: label {label!r} does not name vintage {vintage!r}") + want = _normal(label) for kind, title in titles.items(): - lowered = title.strip().lower() - if not lowered.startswith(f"{label.lower()} economic and fiscal outlook") or ( - not lowered.endswith(kind) + normal = _normal(title) + named = set(re.findall(rf"\b(?:{_MONTHS}) \d{{4}}\b", normal)) + tail = re.sub(r"\s*\([^()]*\)\s*$", "", normal).rstrip(" .,:;-") + if ( + named != {want} + or "economic and fiscal outlook" not in normal + or not re.search(rf"\b{kind}$", tail) ): raise SourceError( f"{where}: the {kind} workbook is titled {title!r}, which is not " @@ -318,10 +345,22 @@ def unit_scale(ws, unit: str) -> float: # A row label giving its own unit, as table 4.19's "Standard colour licence # fee (£)" or "Number of UK households (m)" do under a "£ billion unless -# otherwise stated" table. +# otherwise stated" table. Broad on purpose (Sol re-review of #592): any £, % +# or "per cent" in the label, or a unit word in brackets or after a final +# comma, refuses the row, in any spelling. +_UNIT_WORD = ( + r"(?:bn|billions?|millions?|thousands?|0{3}s?|m|p|pence|per\s*cent|" + r"percentage points?|ppts?)" +) _ROW_UNIT = re.compile( - r"\(\s*(?:£|£m|£bn|£ ?million|£ ?billion|m|bn|million|thousands?|per cent|%|p|pence)\s*\)" - r"|\bper cent\b|%" + rf"£|%|\bper\s*cent\b|\([^()]*\b{_UNIT_WORD}\b[^()]*\)|,\s*{_UNIT_WORD}\s*$", + re.IGNORECASE, +) +# A sub-heading that restates the unit for the rows under it, as "£ million" +# or "Memo: per cent". +_UNIT_HEADING = re.compile( + rf"^\s*(?:memo\s*:?\s*)?(?:£\s*)?{_UNIT_WORD}(?:\s+unless otherwise stated)?\s*$", + re.IGNORECASE, ) @@ -332,11 +371,19 @@ def read_row(ws, row: int, manifest: ObrManifest) -> dict: applied to every row read. """ label = str(ws[f"B{row}"].value or "") - if _ROW_UNIT.search(label): + if _ROW_UNIT.search(_normal(label)): raise SourceError( f"{ws.title}!B{row}: {label!r} states its own unit, not the table's " f"{manifest.unit}" ) + for above in range(_UNIT_ROWS + 1, row): + for column in "AB": + heading = ws[f"{column}{above}"].value + if isinstance(heading, str) and _UNIT_HEADING.match(_normal(heading)): + raise SourceError( + f"{ws.title}!{column}{above}: the sub-heading {heading!r} above " + f"row {row} gives its own unit, not the table's {manifest.unit}" + ) scale = unit_scale(ws, manifest.unit) out = {} for column, year in year_columns( @@ -358,7 +405,12 @@ def coverage_problems(targets: Sequence, manifest: ObrManifest) -> list: stays the complete list. """ built = {t.name: t for t in targets} + names = [t.name for t in targets] problems = [ + f"{name} was built more than once" + for name in sorted({n for n in names if names.count(n) > 1}) + ] + problems += [ f"{t.name} carries vintage {t.forecast_vintage!r}, not {manifest.vintage!r}" for t in targets if t.forecast_vintage != manifest.vintage @@ -465,9 +517,10 @@ def ingest( Everything is checked in a staging folder first: both workbooks read and carry the vintage's titles, and the build's parsers yield every - required target and year from them. Only then are the files copied - into storage and the manifest written. A vintage already in storage or - in the manifests folder is never replaced. + required target and year from them. Only then are the files moved + into storage and the manifest written, each under a temporary name that + is renamed into place. A vintage already in storage or in the manifests + folder is never replaced, and a partial ingest is named on the next try. """ import tempfile @@ -482,11 +535,31 @@ def ingest( downloaded_at = dict(downloaded_at or {}) destination = storage / "obr_efo" / vintage path = manifest_dir / f"obr_{vintage}.yaml" - if not dry_run and (destination.exists() or path.exists()): - raise SourceError( - f"{vintage} is already ingested ({destination} or {path} exists); " - "remove it deliberately before ingesting again" - ) + partial_dir = destination.parent / f".{vintage}.partial" + partial_manifest = path.with_name(f".{path.name}.partial") + if not dry_run: + if path.exists(): + raise SourceError( + f"{vintage} is already ingested ({path} exists); remove it " + "deliberately before ingesting again" + ) + leftovers = [ + p for p in (destination, partial_dir, partial_manifest) if p.exists() + ] + if leftovers: + raise SourceError( + f"an earlier ingest of {vintage} stopped part way: " + f"{', '.join(str(p) for p in leftovers)} exist but {path.name} does " + "not. Remove them deliberately, then ingest again" + ) + missing = [ + kind for kind in KINDS if not str(urls.get(kind, "")).startswith("https://") + ] + if missing: + raise SourceError( + f"ingest: give the https page each workbook was downloaded from " + f"(--{missing[0]}-url); every target cites it" + ) files, titles, data = {}, {}, {} for kind in KINDS: data[kind] = Path(sources[kind]).read_bytes() @@ -541,10 +614,16 @@ def ingest( ) if dry_run: return manifest, path, target_diff(old, new) - destination.mkdir(parents=True) + # Write each part under a temporary name and rename it into place, so an + # interrupted ingest leaves a partial file that the next attempt names, + # never a half-written workbook or manifest under the real name. + partial_dir.mkdir(parents=True) for kind in KINDS: - (storage / files[kind].path).write_bytes(data[kind]) - path.write_text(text) + (partial_dir / f"{kind}.xlsx").write_bytes(data[kind]) + os.replace(partial_dir, destination) + manifest_dir.mkdir(parents=True, exist_ok=True) + partial_manifest.write_text(text) + os.replace(partial_manifest, path) return load_manifest(path), path, target_diff(old, new) diff --git a/policyengine_uk_data/tests/test_obr_sources.py b/policyengine_uk_data/tests/test_obr_sources.py index 5a9d90562..5393acc47 100644 --- a/policyengine_uk_data/tests/test_obr_sources.py +++ b/policyengine_uk_data/tests/test_obr_sources.py @@ -22,6 +22,7 @@ SourceError, active_manifest, check_coverage, + check_names, coverage_problems, ingest, load_workbook, @@ -527,3 +528,125 @@ def test_the_rehearsal_command_runs_on_the_committed_march_workbooks(tmp_path, c assert code == 0 assert "the same bytes as the committed March 2026 workbook" in out assert "would write" in out + + +# Sol re-review of #592: P3 hardening. + + +@pytest.mark.parametrize( + "label", + [ + "BBC licence fee receipts (£ millions)", + "BBC licence fee receipts (£ bn)", + "BBC licence fee receipts (£ m)", + "BBC licence fee receipts (£ thousand)", + "BBC licence fee receipts (£000)", + "BBC licence fee receipts (000s)", + "BBC licence fee receipts (£m, cash)", + "BBC licence fee receipts (millions)", + "BBC licence fee receipts (bn)", + "BBC licence fee receipts, £ million", + "BBC licence fee receipts, million", + "BBC licence fee receipts (per cent)", + "BBC licence fee receipts (% of GDP)", + "BBC licence fee receipts\u00a0(£\u00a0million)", + ], +) +def test_a_row_stating_its_own_unit_in_any_spelling_is_refused(label): + ws = load_workbook("expenditure")["4.19"] + row = obr._find_row(ws, "BBC licence fee receipts", max_row=30) + ws[f"B{row}"] = label + with pytest.raises(SourceError, match="states its own unit"): + read_row(ws, row, MANIFEST) + + +@pytest.mark.parametrize("heading", ["£ million", "Memo: £ million", "per cent", "£m"]) +def test_a_row_under_a_unit_sub_heading_is_refused(heading): + ws = load_workbook("expenditure")["4.19"] + row = obr._find_row(ws, "BBC licence fee receipts", max_row=30) + assert read_row(ws, row, MANIFEST) # the real row reads + ws[f"A{row - 1}"] = heading + with pytest.raises(SourceError, match="sub-heading"): + read_row(ws, row, MANIFEST) + + +@pytest.mark.parametrize( + "title", + [ + "October 2026 Economic and fiscal outlook \u2013 detailed forecast tables: receipts", + "Economic and fiscal outlook \u2013 October 2026: detailed forecast tables \u2013 receipts", + "October 2026 Economic and fiscal outlook - detailed forecast tables: receipts (revised)", + "October 2026 Economic and fiscal outlook \u2013 detailed forecast tables: receipts.", + "October\u00a02026 Economic and fiscal outlook \u2014 detailed forecast tables: Receipts", + ], +) +def test_plausible_title_formats_are_accepted(title): + check_names("october_2026", "October 2026", {"receipts": title}, "test") + + +@pytest.mark.parametrize( + "title", + [ + "March 2026 Economic and fiscal outlook \u2013 detailed forecast tables: receipts", + "October 2026 Economic and fiscal outlook (March 2026 tables): receipts", + "October 2026 Fiscal risks and sustainability \u2013 tables: receipts", + "October 2026 Economic and fiscal outlook \u2013 detailed forecast tables: expenditure", + "October 2026 Economic and fiscal outlook \u2013 receipts: supplementary tables", + ], +) +def test_a_title_for_another_vintage_report_or_kind_is_refused(title): + with pytest.raises(SourceError, match="not the October 2026 EFO receipts"): + check_names("october_2026", "October 2026", {"receipts": title}, "test") + + +def test_a_target_built_twice_fails_coverage(march_targets): + targets = [t for t in march_targets if t.forecast_vintage == MANIFEST.vintage] + problems = coverage_problems(targets + [targets[0]], MANIFEST) + assert problems == [f"{targets[0].name} was built more than once"] + + +def test_a_real_ingest_needs_the_download_pages(tmp_path): + with pytest.raises(SourceError, match="https page"): + _ingest(tmp_path, urls={"receipts": "", "expenditure": ""}) + assert not (tmp_path / "storage").exists() + # A dry run, the Budget-day rehearsal, does not need them. + _ingest(tmp_path, urls={}, dry_run=True) + + +def test_an_interrupted_workbook_copy_is_named_on_retry(tmp_path, monkeypatch): + real = obr_sources.Path.write_bytes + + def fail_on_expenditure(self, data): + # The copy into storage, after staging and every check have passed. + if self.name == "expenditure.xlsx" and self.parent.name.endswith(".partial"): + raise OSError("disk full") + return real(self, data) + + monkeypatch.setattr(obr_sources.Path, "write_bytes", fail_on_expenditure) + with pytest.raises(OSError, match="disk full"): + _ingest(tmp_path) + monkeypatch.undo() + # Nothing is under the vintage's real name. + assert not (tmp_path / "storage" / "obr_efo" / "march_2026").exists() + assert not (tmp_path / "manifests" / "obr_march_2026.yaml").exists() + with pytest.raises(SourceError, match="stopped part way: .*march_2026.partial"): + _ingest(tmp_path) + + +def test_an_interrupted_manifest_write_is_named_on_retry(tmp_path, monkeypatch): + real = obr_sources.os.replace + + def fail_on_manifest(source, destination): + if str(destination).endswith(".yaml"): + raise OSError("interrupted") + return real(source, destination) + + monkeypatch.setattr(obr_sources.os, "replace", fail_on_manifest) + with pytest.raises(OSError, match="interrupted"): + _ingest(tmp_path) + monkeypatch.undo() + assert not (tmp_path / "manifests" / "obr_march_2026.yaml").exists() + with pytest.raises(SourceError, match="stopped part way") as refused: + _ingest(tmp_path) + message = str(refused.value) + assert "march_2026" in message and ".obr_march_2026.yaml.partial" in message From 3991f6975b9c88ec5d15e95594ec4ca5e382a5b3 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 16:53:38 -0400 Subject: [PATCH 5/7] Report dry-run refusals, record download-time basis, tighten OBR checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sol delta review of #592 at f1faff2f (four P3 findings), plus one unit fix: - A workbook title must also say "detailed forecast tables", so "Economic and fiscal outlook – October 2026: supplementary fiscal tables – receipts" is refused. - A dry run reports what the real ingest would refuse (an ingested vintage, an interrupted ingest's leftovers, a download page missing or off obr.uk) as "would refuse: ..." lines, returns them in IngestResult.refusals and exits 1, still writing nothing. A real ingest raises exactly those refusals. A dry run of the active vintage on the bytes its manifest names is a rehearsal, not a refusal. - Each manifest file records downloaded_at_basis: mtime or given, or unrecorded when there is no downloaded_at. The March 2026 workbooks entered git in 11e41f9d with no download time, so theirs is unrecorded. - A download page must be https on obr.uk or a subdomain, with nothing in the address but the host and port 443. - A sub-heading restating the table's own unit ("£ billion", "£bn") is no longer refused. - The manifest text is parsed back before anything is written. - The CLI rehearsal tests run on a copy of the committed manifest and workbooks made the active one, so no test can touch the committed files. Co-Authored-By: Claude Opus 5.5 --- changelog.d/obr-source-manifest.changed.md | 2 +- policyengine_uk_data/targets/obr_sources.py | 230 +++++++++----- .../source_manifests/obr_march_2026.yaml | 6 + .../tests/test_obr_sources.py | 293 +++++++++++++++++- 4 files changed, 443 insertions(+), 88 deletions(-) diff --git a/changelog.d/obr-source-manifest.changed.md b/changelog.d/obr-source-manifest.changed.md index b524adfd8..8a609f7c1 100644 --- a/changelog.d/obr-source-manifest.changed.md +++ b/changelog.d/obr-source-manifest.changed.md @@ -1 +1 @@ -Build the OBR targets only from the committed EFO workbooks named, with their SHA-256 and title, in a vintage manifest (`targets/source_manifests/`). Read each table's financial years from its header row rather than fixed column letters, and fail the build when a manifest target or year is missing instead of logging and dropping it. Add `python -m policyengine_uk_data.targets.obr_sources ingest` for adopting a new EFO vintage, which prints every target's change. +Build the OBR targets only from the committed EFO workbooks named, with their SHA-256 and title, in a vintage manifest (`targets/source_manifests/`). Read each table's financial years from its header row rather than fixed column letters, and fail the build when a manifest target or year is missing instead of logging and dropping it. Add `python -m policyengine_uk_data.targets.obr_sources ingest` for adopting a new EFO vintage, which prints every target's change; its `--dry-run` reports, and exits non-zero on, an ingested vintage, an interrupted ingest's leftovers or a download page off obr.uk, which the real ingest refuses. diff --git a/policyengine_uk_data/targets/obr_sources.py b/policyengine_uk_data/targets/obr_sources.py index 6898341fd..93b813909 100644 --- a/policyengine_uk_data/targets/obr_sources.py +++ b/policyengine_uk_data/targets/obr_sources.py @@ -24,21 +24,26 @@ obr.uk refuses scripted downloads (HTTP 403 to GitHub Actions runners, as 11e41f9d records, and to a developer machine on 2026-10-15), so a person downloads the two "detailed forecast tables" workbooks in a browser and -passes their paths. ``--dry-run`` checks everything and prints each -workbook's title, SHA-256, size and download time and every target's old -and new value, writing nothing. It also says whether the bytes match the -committed workbooks when the vintage is the active one, which makes a -March 2026 download a rehearsal. Without ``--dry-run``, ``ingest`` also -needs ``--receipts-url`` and ``--expenditure-url``, the pages the workbooks -came from, which every target cites. It parses the workbooks in a staging -folder and then moves them under ``storage/obr_efo//``. It requires +passes their paths, with ``--receipts-url`` and ``--expenditure-url``, the +obr.uk pages they came from, which every target cites. ``ingest`` requires each workbook's title to name the vintage and no other, and requires the -targets the current manifest requires. It writes the manifest with each -file's SHA-256, size and download time (the file's modification time unless -``--downloaded-at`` is given). Each part is written under a temporary name -and renamed into place, so an interrupted ingest leaves a ``.partial`` file -that the next attempt names. It never replaces an ingested vintage. Point +targets the current manifest requires. It parses the workbooks in a staging +folder and then moves them under ``storage/obr_efo//``. It writes +the manifest with each file's SHA-256, size and download time, and the +basis of that time: ``mtime``, the file's modification time, or ``given`` +with ``--downloaded-at``, which a copied or unzipped file needs. Each part is written under a temporary name and +renamed into place, so an interrupted ingest leaves a ``.partial`` file that +the next attempt names. It never replaces an ingested vintage. Point ``sources.yaml`` at the new manifest to adopt it. + +``--dry-run`` checks everything and prints each workbook's title, SHA-256, +size and download time and every target's old and new value, writing +nothing. The three things a real ingest refuses before parsing (an ingested +vintage, an earlier ingest's leftovers, a download page missing or off +obr.uk) it prints as "would refuse: ..." and exits non-zero; any other +problem raises, as in a real ingest. On the active vintage it says whether +the bytes are the committed workbooks; the same bytes are a rehearsal of +the Budget-day path, not a refusal. """ from __future__ import annotations @@ -54,7 +59,8 @@ from datetime import date, datetime, timezone from functools import lru_cache from pathlib import Path -from typing import Mapping, Optional, Sequence +from typing import Mapping, NamedTuple, Optional, Sequence +from urllib.parse import urlsplit import openpyxl import yaml @@ -62,6 +68,9 @@ MANIFEST_DIR = Path(__file__).parent / "source_manifests" KINDS = ("receipts", "expenditure") UNIT_SCALES = {"£ billion": 1e9} +# How a download time is known: the file's modification time at ingest, or a +# time given with --downloaded-at. +DOWNLOAD_TIME_BASES = ("mtime", "given") _FINANCIAL_YEAR = re.compile(r"^\s*(\d{4})\s*[-–]\s*(\d{2})\s*$") _HEADER_ROWS = 10 _UNIT_ROWS = 4 @@ -84,8 +93,10 @@ class SourceFile: title: str # the workbook's own title, cell B2 of its Contents sheet url: str retrieved: str - # When the file was downloaded (ISO 8601 with a UTC offset), if known. + # When the file was downloaded (ISO 8601 with a UTC offset), if known, and + # how: one of DOWNLOAD_TIME_BASES, or "unrecorded" when there is no time. downloaded_at: Optional[str] = None + downloaded_at_basis: str = "unrecorded" @dataclass(frozen=True) @@ -127,6 +138,15 @@ def parse_manifest(raw: Mapping, path: Optional[Path] = None) -> ObrManifest: sha = str(entry["sha256"]) if not re.fullmatch(r"[0-9a-f]{64}", sha): raise SourceError(f"{where}: {kind} sha256 must be 64 hex characters") + downloaded_at = _timestamp(entry.get("downloaded_at"), f"{where}: {kind}") + bases = DOWNLOAD_TIME_BASES if downloaded_at else ("unrecorded",) + basis = entry.get("downloaded_at_basis") + if basis not in bases: + raise SourceError( + f"{where}: {kind} downloaded_at_basis is {basis!r}; with " + f"{'a' if downloaded_at else 'no'} downloaded_at it must be " + + " or ".join(bases) + ) files[kind] = SourceFile( kind=kind, path=str(entry["path"]), @@ -135,7 +155,8 @@ def parse_manifest(raw: Mapping, path: Optional[Path] = None) -> ObrManifest: title=str(entry["title"]), url=str(entry["url"]), retrieved=str(entry["retrieved"]), - downloaded_at=_timestamp(entry.get("downloaded_at"), f"{where}: {kind}"), + downloaded_at=downloaded_at, + downloaded_at_basis=basis, ) required, alternatives = [], [] for item in (raw.get("targets") or {}).get("required") or []: @@ -198,11 +219,11 @@ def check_names(vintage: str, label: str, titles: Mapping[str, str], where: str) ``march_2026`` must be labelled "March 2026". Each workbook's title must name that vintage and no other, call itself the Economic and fiscal - outlook, and end with its kind, ignoring a closing note in brackets and - punctuation: "March 2026 Economic and fiscal outlook - detailed forecast - tables: receipts" or "Economic and fiscal outlook, March 2026: detailed - forecast tables - receipts (revised)". The title is read from the verified - bytes, so the bytes decide the vintage. + outlook's detailed forecast tables, and end with its kind, ignoring a + closing note in brackets and punctuation: "March 2026 Economic and fiscal + outlook - detailed forecast tables: receipts" or "Economic and fiscal + outlook, March 2026: detailed forecast tables - receipts (revised)". The + title is read from the verified bytes, so the bytes decide the vintage. """ if not re.fullmatch(r"[a-z]+_\d{4}", vintage): raise SourceError(f"{where}: vintage must look like october_2026") @@ -216,6 +237,7 @@ def check_names(vintage: str, label: str, titles: Mapping[str, str], where: str) if ( named != {want} or "economic and fiscal outlook" not in normal + or "detailed forecast tables" not in normal or not re.search(rf"\b{kind}$", tail) ): raise SourceError( @@ -357,11 +379,16 @@ def unit_scale(ws, unit: str) -> float: re.IGNORECASE, ) # A sub-heading that restates the unit for the rows under it, as "£ million" -# or "Memo: per cent". +# or "Memo: per cent". One giving the table's own unit, as "£ billion" or "£bn" +# under a "£ billion unless otherwise stated" table, changes nothing and is +# not refused. _UNIT_HEADING = re.compile( - rf"^\s*(?:memo\s*:?\s*)?(?:£\s*)?{_UNIT_WORD}(?:\s+unless otherwise stated)?\s*$", + rf"^\s*(?:memo\s*:?\s*)?(?P(?:£\s*)?{_UNIT_WORD})" + r"(?:\s+unless otherwise stated)?\s*$", re.IGNORECASE, ) +# How each unit in UNIT_SCALES may be spelt in such a sub-heading. +_TABLE_UNIT_SPELLINGS = {"£ billion": re.compile(r"£\s*(?:bn|billions?)")} def read_row(ws, row: int, manifest: ObrManifest) -> dict: @@ -376,10 +403,12 @@ def read_row(ws, row: int, manifest: ObrManifest) -> dict: f"{ws.title}!B{row}: {label!r} states its own unit, not the table's " f"{manifest.unit}" ) + own_unit = _TABLE_UNIT_SPELLINGS[manifest.unit] for above in range(_UNIT_ROWS + 1, row): for column in "AB": heading = ws[f"{column}{above}"].value - if isinstance(heading, str) and _UNIT_HEADING.match(_normal(heading)): + match = isinstance(heading, str) and _UNIT_HEADING.match(_normal(heading)) + if match and not own_unit.fullmatch(match["unit"]): raise SourceError( f"{ws.title}!{column}{above}: the sub-heading {heading!r} above " f"row {row} gives its own unit, not the table's {manifest.unit}" @@ -476,6 +505,7 @@ def _manifest_yaml(manifest: ObrManifest, alternatives_raw) -> str: "url": f.url, "retrieved": f.retrieved, **({"downloaded_at": f.downloaded_at} if f.downloaded_at else {}), + "downloaded_at_basis": f.downloaded_at_basis, } for kind, f in manifest.files.items() }, @@ -492,6 +522,33 @@ def _manifest_yaml(manifest: ObrManifest, alternatives_raw) -> str: return header + yaml.safe_dump(raw, sort_keys=False, allow_unicode=True) +class IngestResult(NamedTuple): + manifest: ObrManifest + path: Path # where the manifest is, or would be, written + diff: list # each target's current and new value by year + # What a real ingest would refuse. Only a dry run returns any; a real + # ingest raises them instead. + refusals: list + + +def is_obr_page(url) -> bool: + """Whether a download page is https on obr.uk or one of its subdomains. + + The address must be the host alone, or with port 443: no user name, no + other port, and no backslash, which ``urlsplit`` leaves in the address. + """ + try: + parts = urlsplit(str(url)) + host = parts.hostname or "" + except ValueError: + return False + return ( + parts.scheme == "https" + and parts.netloc.lower() in (host, f"{host}:443") + and bool(re.fullmatch(r"(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)*obr\.uk", host)) + ) + + def ingest( *, vintage: str, @@ -506,14 +563,21 @@ def ingest( retrieved: Optional[str] = None, downloaded_at: Optional[Mapping[str, str]] = None, dry_run: bool = False, -): - """Adopt a vintage's two workbooks. Returns (manifest, manifest path, diff). +) -> IngestResult: + """Adopt a vintage's two workbooks. Each workbook's SHA-256, size and download time go into the manifest. - The download time is ``downloaded_at[kind]`` if given, otherwise the - file's modification time, which a browser sets when it saves a download. - With ``dry_run`` everything is checked and the diff returned, but - nothing is written; the manifest returned is not on disk. + The download time is ``downloaded_at[kind]`` if given (basis ``given``), + otherwise the file's modification time (basis ``mtime``). Copying a file + without keeping its times, or unzipping it, changes that time, so give + the time for such a file. + + A real ingest refuses an ingested vintage, an earlier ingest's leftovers + and a download page that is not https on obr.uk. A dry run returns those + as ``refusals`` instead; it checks everything else as a real ingest does, + raising on any other problem, returns the diff and writes nothing. The + manifest it returns is not on disk. A dry run of the active vintage on the bytes its + manifest names is a rehearsal, so that manifest is not a refusal. Everything is checked in a staging folder first: both workbooks read and carry the vintage's titles, and the build's parsers yield every @@ -537,32 +601,41 @@ def ingest( path = manifest_dir / f"obr_{vintage}.yaml" partial_dir = destination.parent / f".{vintage}.partial" partial_manifest = path.with_name(f".{path.name}.partial") - if not dry_run: - if path.exists(): - raise SourceError( + data = {kind: Path(sources[kind]).read_bytes() for kind in KINDS} + rehearsal = ( + dry_run + and current.path is not None + and path.resolve() == Path(current.path).resolve() + and all(sha256_bytes(data[k]) == current.files[k].sha256 for k in KINDS) + ) + refusals = [] + if path.exists(): + if not rehearsal: + refusals.append( f"{vintage} is already ingested ({path} exists); remove it " "deliberately before ingesting again" ) + else: leftovers = [ p for p in (destination, partial_dir, partial_manifest) if p.exists() ] if leftovers: - raise SourceError( + refusals.append( f"an earlier ingest of {vintage} stopped part way: " f"{', '.join(str(p) for p in leftovers)} exist but {path.name} does " "not. Remove them deliberately, then ingest again" ) - missing = [ - kind for kind in KINDS if not str(urls.get(kind, "")).startswith("https://") - ] - if missing: - raise SourceError( - f"ingest: give the https page each workbook was downloaded from " - f"(--{missing[0]}-url); every target cites it" - ) - files, titles, data = {}, {}, {} + missing = [kind for kind in KINDS if not is_obr_page(urls.get(kind, ""))] + if missing: + refusals.append( + "give the obr.uk https page each workbook was downloaded from (" + + ", ".join(f"--{kind}-url" for kind in missing) + + "); every target cites it" + ) + if refusals and not dry_run: + raise SourceError("ingest: " + "; ".join(refusals)) + files, titles = {}, {} for kind in KINDS: - data[kind] = Path(sources[kind]).read_bytes() try: wb = openpyxl.load_workbook(io.BytesIO(data[kind]), data_only=False) except Exception as error: @@ -572,6 +645,10 @@ def ingest( "download the workbook in a browser" ) from error titles[kind] = workbook_title(wb) + given = downloaded_at.get(kind) + mtime = datetime.fromtimestamp( + Path(sources[kind]).stat().st_mtime, tz=timezone.utc + ) files[kind] = SourceFile( kind=kind, path=f"obr_efo/{vintage}/{kind}.xlsx", @@ -580,13 +657,8 @@ def ingest( title=titles[kind], url=urls.get(kind, ""), retrieved=retrieved, - downloaded_at=_timestamp( - downloaded_at.get(kind) - or datetime.fromtimestamp( - Path(sources[kind]).stat().st_mtime, tz=timezone.utc - ), - f"ingest: {kind}", - ), + downloaded_at=_timestamp(given or mtime, f"ingest: {kind}"), + downloaded_at_basis="given" if given else "mtime", ) check_names(vintage, label, titles, "ingest") manifest = ObrManifest( @@ -607,13 +679,14 @@ def ingest( staged.write_bytes(data[kind]) # Raises unless the new workbooks yield every target and year required. new = obr.workbook_targets(manifest, staging) - old = obr.workbook_targets(current) + diff = target_diff(obr.workbook_targets(current), new) text = _manifest_yaml( manifest, [[sorted(group) for group in groups] for groups in current.alternatives], ) + parse_manifest(yaml.safe_load(text), path) # reads back before it is written if dry_run: - return manifest, path, target_diff(old, new) + return IngestResult(manifest, path, diff, refusals) # Write each part under a temporary name and rename it into place, so an # interrupted ingest leaves a partial file that the next attempt names, # never a half-written workbook or manifest under the real name. @@ -624,7 +697,7 @@ def ingest( manifest_dir.mkdir(parents=True, exist_ok=True) partial_manifest.write_text(text) os.replace(partial_manifest, path) - return load_manifest(path), path, target_diff(old, new) + return IngestResult(load_manifest(path), path, diff, []) def _gbp(value) -> str: @@ -641,20 +714,26 @@ def main(argv=None) -> int: i.add_argument("--label", required=True, help='e.g. "October 2026"') i.add_argument("--receipts", required=True, type=Path) i.add_argument("--expenditure", required=True, type=Path) - i.add_argument("--receipts-url", default="") - i.add_argument("--expenditure-url", default="") + i.add_argument( + "--receipts-url", default="", help="the obr.uk page it was downloaded from" + ) + i.add_argument( + "--expenditure-url", default="", help="the obr.uk page it was downloaded from" + ) i.add_argument("--first-year", required=True, type=int) i.add_argument("--last-year", required=True, type=int) i.add_argument("--retrieved", help="how the files were obtained") i.add_argument( "--downloaded-at", - help="when the workbooks were downloaded (ISO 8601 with offset); " - "default: each file's modification time", + help="when the workbooks were downloaded (ISO 8601 with offset), " + "recorded with basis 'given'; default: each file's modification time, " + "with basis 'mtime'", ) i.add_argument( "--dry-run", action="store_true", - help="check everything and print the change, but write nothing", + help="check everything and print the change, but write nothing; " + "exits 1 if the real ingest would refuse", ) v = sub.add_parser("verify", help="verify the active manifest's workbooks") v.add_argument("--manifest", help="manifest file (default: the active one)") @@ -667,7 +746,7 @@ def main(argv=None) -> int: check_coverage(targets, manifest) print(f"{manifest.label}: both workbooks verified, {len(targets)} targets") return 0 - manifest, path, diff = ingest( + result = ingest( vintage=args.vintage, label=args.label, sources={"receipts": args.receipts, "expenditure": args.expenditure}, @@ -680,26 +759,35 @@ def main(argv=None) -> int: else None, dry_run=args.dry_run, ) + manifest, path = result.manifest, result.path for kind, f in manifest.files.items(): print( f"{kind}: {f.title}; sha256 {f.sha256}, {f.size_bytes:,} bytes, " - f"downloaded {f.downloaded_at}" + f"downloaded {f.downloaded_at} (basis: {f.downloaded_at_basis})" ) - print(f"{'would write' if args.dry_run else 'wrote'} {path}") print("target,year,current £bn,new £bn") - for row in diff: + for row in result.diff: print(f"{row['target']},{row['year']},{_gbp(row['old'])},{_gbp(row['new'])}") if not args.dry_run: + print(f"wrote {path}") print(f"To adopt it, set obr.manifest in sources.yaml to {path.name}.") + return 0 + current = active_manifest() + if manifest.vintage == current.vintage: + for kind, f in manifest.files.items(): + same = f.sha256 == current.files[kind].sha256 + print( + f"{kind}: {'the same bytes as' if same else 'different bytes from'} " + f"the committed {current.label} workbook" + ) + for refusal in result.refusals: + print(f"would refuse: {refusal}") + if result.refusals: + return 1 + if path.exists(): + print(f"{path} already names these bytes: a rehearsal, nothing to write") else: - current = active_manifest() - if manifest.vintage == current.vintage: - for kind, f in manifest.files.items(): - same = f.sha256 == current.files[kind].sha256 - print( - f"{kind}: {'the same bytes as' if same else 'different bytes from'} " - f"the committed {current.label} workbook" - ) + print(f"would write {path}") return 0 diff --git a/policyengine_uk_data/targets/source_manifests/obr_march_2026.yaml b/policyengine_uk_data/targets/source_manifests/obr_march_2026.yaml index a0716cc29..6d63f13cc 100644 --- a/policyengine_uk_data/targets/source_manifests/obr_march_2026.yaml +++ b/policyengine_uk_data/targets/source_manifests/obr_march_2026.yaml @@ -10,6 +10,10 @@ label: March 2026 first_year: 2024 last_year: 2030 unit: £ billion +# Neither workbook has a downloaded_at. Both entered git in 11e41f9d, so +# they were downloaded no later than 2026-07-21T21:06:50+01:00, but nothing +# recorded when (git keeps no file times): their download time is neither an +# mtime nor a given one. files: receipts: path: obr_efo/efo_receipts.xlsx @@ -22,6 +26,7 @@ files: workbook's own properties record it as modified on 2026-03-02. obr.uk refused scripted downloads on 2026-10-15 (HTTP 403), so the bytes were not re-fetched for this manifest. + downloaded_at_basis: unrecorded expenditure: path: obr_efo/efo_expenditure.xlsx sha256: 700bfd700c872794e5fe8cf164086cd38a63850db26427b9d4b2e363d177d9a1 @@ -33,6 +38,7 @@ files: workbook's own properties record it as modified on 2026-03-02. obr.uk refused scripted downloads on 2026-10-15 (HTTP 403), so the bytes were not re-fetched for this manifest. + downloaded_at_basis: unrecorded # Every target the workbooks must yield, each with every year above. A # target the parsers build that is not listed here also fails, so changing # the target set means changing this list. diff --git a/policyengine_uk_data/tests/test_obr_sources.py b/policyengine_uk_data/tests/test_obr_sources.py index 5393acc47..9bd63c1d0 100644 --- a/policyengine_uk_data/tests/test_obr_sources.py +++ b/policyengine_uk_data/tests/test_obr_sources.py @@ -10,9 +10,12 @@ import io import shutil +from dataclasses import replace +from datetime import datetime, timedelta, timezone import openpyxl import pytest +import yaml from hypothesis import HealthCheck, given, settings from hypothesis import strategies as st @@ -299,7 +302,7 @@ def _sources(tmp_path): def _ingest(tmp_path, **overrides): - (tmp_path / "manifests").mkdir(exist_ok=True) + (tmp_path / "manifests").mkdir(parents=True, exist_ok=True) arguments = dict( vintage="march_2026", label="March 2026", @@ -317,8 +320,9 @@ def _ingest(tmp_path, **overrides): def test_ingesting_the_same_workbooks_reproduces_the_targets(tmp_path, march_targets): - manifest, path, diff = _ingest(tmp_path) + manifest, path, diff, refusals = _ingest(tmp_path) assert path.name == "obr_march_2026.yaml" + assert refusals == [] assert manifest.vintage == "march_2026" assert {k: f.sha256 for k, f in manifest.files.items()} == { k: f.sha256 for k, f in MANIFEST.files.items() @@ -466,13 +470,15 @@ def test_ingest_records_each_download_time(tmp_path): when = datetime(2026, 10, 18, 12, tzinfo=timezone.utc).timestamp() for path in sources.values(): os.utime(path, (when, when)) - manifest, path, _ = _ingest(tmp_path, sources=sources) - assert {f.downloaded_at for f in manifest.files.values()} == { - "2026-10-18T12:00:00+00:00" - } - assert "downloaded_at: '2026-10-18T12:00:00+00:00'" in path.read_text() + manifest, path, _, _ = _ingest(tmp_path, sources=sources) + assert { + (f.downloaded_at, f.downloaded_at_basis) for f in manifest.files.values() + } == {("2026-10-18T12:00:00+00:00", "mtime")} + text = path.read_text() + assert "downloaded_at: '2026-10-18T12:00:00+00:00'" in text + assert text.count("downloaded_at_basis: mtime") == 2 (tmp_path / "again").mkdir() - manifest, _, _ = _ingest( + manifest, path, _, _ = _ingest( tmp_path / "again", downloaded_at={ "receipts": "2026-10-28T13:05:00+00:00", @@ -480,6 +486,8 @@ def test_ingest_records_each_download_time(tmp_path): }, ) assert manifest.files["receipts"].downloaded_at == "2026-10-28T13:05:00+00:00" + assert {f.downloaded_at_basis for f in manifest.files.values()} == {"given"} + assert path.read_text().count("downloaded_at_basis: given") == 2 with pytest.raises(SourceError, match="UTC offset"): _ingest( tmp_path / "again", @@ -493,8 +501,9 @@ def test_ingest_records_each_download_time(tmp_path): def test_a_dry_run_checks_everything_and_writes_nothing(tmp_path): - manifest, path, diff = _ingest(tmp_path, dry_run=True) + manifest, path, diff, refusals = _ingest(tmp_path, dry_run=True) assert manifest.vintage == "march_2026" + assert refusals == [] assert diff and all(row["old"] == row["new"] for row in diff) assert not path.exists() assert not (tmp_path / "storage" / "obr_efo").exists() @@ -503,10 +512,25 @@ def test_a_dry_run_checks_everything_and_writes_nothing(tmp_path): _ingest(tmp_path, vintage="october_2026", label="October 2026", dry_run=True) -def test_the_rehearsal_command_runs_on_the_committed_march_workbooks(tmp_path, capsys): - """The Budget-day command, end to end, on downloads of the active vintage.""" +@pytest.fixture +def committed(tmp_path, monkeypatch): + """The active manifest and its workbooks, copied under tmp_path and made + the active ones, so nothing a test runs can touch the committed files.""" + storage = _copy_storage(tmp_path / "storage") + manifests = tmp_path / "manifests" + manifests.mkdir() + shutil.copyfile(MANIFEST.path, manifests / MANIFEST.path.name) + active = obr_sources.load_manifest(manifests / MANIFEST.path.name) + monkeypatch.setattr(obr_sources, "MANIFEST_DIR", manifests) + monkeypatch.setattr(obr_sources, "_storage", lambda: storage) + monkeypatch.setattr(obr_sources, "active_manifest", lambda: active) + return active + + +def _rehearse(tmp_path, *urls): + """The Budget-day dry run on copies of the committed March 2026 workbooks.""" sources = _sources(tmp_path) - code = obr_sources.main( + return obr_sources.main( [ "ingest", "--vintage", @@ -517,6 +541,7 @@ def test_the_rehearsal_command_runs_on_the_committed_march_workbooks(tmp_path, c str(sources["receipts"]), "--expenditure", str(sources["expenditure"]), + *urls, "--first-year", "2024", "--last-year", @@ -524,10 +549,39 @@ def test_the_rehearsal_command_runs_on_the_committed_march_workbooks(tmp_path, c "--dry-run", ] ) + + +def test_the_rehearsal_command_runs_on_the_committed_march_workbooks( + tmp_path, committed, capsys +): + """The Budget-day command, end to end, on downloads of the active vintage. + + The committed manifest names these very bytes, so it is a rehearsal, not + an ingested vintage the real ingest would refuse. + """ + urls = MANIFEST.files["receipts"].url, MANIFEST.files["expenditure"].url + code = _rehearse(tmp_path, "--receipts-url", urls[0], "--expenditure-url", urls[1]) out = capsys.readouterr().out assert code == 0 - assert "the same bytes as the committed March 2026 workbook" in out - assert "would write" in out + assert out.count("the same bytes as the committed March 2026 workbook") == 2 + assert out.count("(basis: mtime)") == 2 + assert "already names these bytes: a rehearsal, nothing to write" in out + assert "would refuse" not in out and "would write" not in out + + +def test_a_dry_run_the_real_ingest_would_refuse_exits_non_zero( + tmp_path, committed, capsys +): + """Sol delta review of #592: a rehearsal without the download pages fails.""" + code = _rehearse(tmp_path) + out = capsys.readouterr().out + assert code == 1 + assert ( + "would refuse: give the obr.uk https page each workbook was downloaded " + "from (--receipts-url, --expenditure-url); every target cites it" + ) in out + assert out.count("the same bytes as the committed March 2026 workbook") == 2 + assert "would write" not in out and "nothing to write" not in out # Sol re-review of #592: P3 hardening. @@ -592,6 +646,7 @@ def test_plausible_title_formats_are_accepted(title): "October 2026 Fiscal risks and sustainability \u2013 tables: receipts", "October 2026 Economic and fiscal outlook \u2013 detailed forecast tables: expenditure", "October 2026 Economic and fiscal outlook \u2013 receipts: supplementary tables", + "Economic and fiscal outlook \u2013 October 2026: supplementary fiscal tables \u2013 receipts", ], ) def test_a_title_for_another_vintage_report_or_kind_is_refused(title): @@ -609,8 +664,14 @@ def test_a_real_ingest_needs_the_download_pages(tmp_path): with pytest.raises(SourceError, match="https page"): _ingest(tmp_path, urls={"receipts": "", "expenditure": ""}) assert not (tmp_path / "storage").exists() - # A dry run, the Budget-day rehearsal, does not need them. - _ingest(tmp_path, urls={}, dry_run=True) + # A dry run reports it instead. + refusals = _ingest(tmp_path, urls={}, dry_run=True).refusals + assert refusals == [ + ( + "give the obr.uk https page each workbook was downloaded from " + "(--receipts-url, --expenditure-url); every target cites it" + ) + ] def test_an_interrupted_workbook_copy_is_named_on_retry(tmp_path, monkeypatch): @@ -650,3 +711,203 @@ def fail_on_manifest(source, destination): _ingest(tmp_path) message = str(refused.value) assert "march_2026" in message and ".obr_march_2026.yaml.partial" in message + + +# Sol delta review of #592 at f1faff2f. + + +def test_a_dry_run_reports_what_the_real_ingest_would_refuse(tmp_path): + """Leftovers, a page off obr.uk and an ingested vintage, without writing.""" + partial = tmp_path / "storage" / "obr_efo" / ".march_2026.partial" + partial.mkdir(parents=True) + urls = {k: f.url for k, f in MANIFEST.files.items()} + urls["receipts"] = "https://example.com/receipts.xlsx" + _, path, diff, refusals = _ingest(tmp_path, urls=urls, dry_run=True) + assert len(refusals) == 2 + assert "stopped part way" in refusals[0] and ".march_2026.partial" in refusals[0] + assert ( + "https page each workbook was downloaded from (--receipts-url)" in (refusals[1]) + ) + # Everything else is still checked, and nothing is written. + assert diff and all(row["old"] == row["new"] for row in diff) + assert not path.exists() + assert [p.name for p in partial.parent.iterdir()] == [partial.name] + # The real ingest refuses exactly what the dry run reported. + with pytest.raises(SourceError) as refused: + _ingest(tmp_path, urls=urls) + assert str(refused.value) == "ingest: " + "; ".join(refusals) + # An ingested vintage that is not the active manifest is refused too. + partial.rmdir() + _ingest(tmp_path) + refusals = _ingest(tmp_path, dry_run=True).refusals + assert len(refusals) == 1 and "march_2026 is already ingested" in refusals[0] + + +def test_a_dry_run_of_the_active_vintage_is_a_rehearsal_only_on_its_own_bytes( + tmp_path, committed +): + """The active manifest names these bytes: a rehearsal, not a refusal.""" + active = {"current": committed, "dry_run": True} + _, path, _, refusals = _ingest(tmp_path, **active) + assert path == committed.path and refusals == [] + # Different bytes under the active vintage's name are refused, as a real + # ingest refuses any ingested vintage. + sources = _sources(tmp_path) + openpyxl.load_workbook(sources["receipts"]).save(sources["receipts"]) + assert obr_sources.sha256_bytes(sources["receipts"].read_bytes()) != ( + MANIFEST.files["receipts"].sha256 + ) + refusals = _ingest(tmp_path, sources=sources, **active).refusals + assert len(refusals) == 1 and "march_2026 is already ingested" in refusals[0] + + +@pytest.mark.parametrize( + "url, ok", + [ + (MANIFEST.files["receipts"].url, True), + ("https://www.obr.uk/download/receipts/", True), + ("https://OBR.UK/download/receipts/", True), + ("https://obr.uk:443/download/receipts/", True), + ("http://obr.uk/download/receipts/", False), + ("https://example.com/receipts.xlsx", False), + ("https://obr.uk.example.com/receipts/", False), + ("https://notobr.uk/receipts/", False), + ("https://example.com/obr.uk/receipts/", False), + ("https://example.com/?from=https://obr.uk/", False), + ("https://obr.uk@example.com/receipts/", False), + ("https://example.com\\.obr.uk/receipts/", False), + ("https://example.com\\@obr.uk/receipts/", False), + ("https://user:password@obr.uk/receipts/", False), + ("https://obr.uk:8443/receipts/", False), + ("https://obr.uk:notaport/receipts/", False), + ("https://-.obr.uk/receipts/", False), + ("obr.uk/download/receipts/", False), + ("https://[::1/receipts/", False), + ("", False), + ], +) +def test_a_download_page_must_be_https_on_obr_uk(url, ok): + assert obr_sources.is_obr_page(url) is ok + + +@settings(max_examples=200, deadline=None) +@given( + labels=st.lists( + st.from_regex(r"[a-z0-9](?:[a-z0-9-]{0,7}[a-z0-9])?", fullmatch=True), + max_size=3, + ), + suffix=st.sampled_from(["obr.uk", "uk", "xobr.uk", "obr.uk.example.com", "gov.uk"]), + scheme=st.sampled_from(["https", "http"]), +) +def test_only_https_pages_on_obr_uk_or_its_subdomains_are_accepted( + labels, suffix, scheme +): + host = ".".join([*labels, suffix]) + expected = scheme == "https" and (host == "obr.uk" or host.endswith(".obr.uk")) + assert obr_sources.is_obr_page(f"{scheme}://{host}/download/x/") is expected + + +def test_ingest_refuses_a_download_page_off_obr_uk(tmp_path): + urls = {k: f.url for k, f in MANIFEST.files.items()} + urls["expenditure"] = "https://example.com/expenditure.xlsx" + with pytest.raises(SourceError, match=r"https page .*\(--expenditure-url\)"): + _ingest(tmp_path, urls=urls) + assert not (tmp_path / "storage").exists() + + +@pytest.mark.parametrize( + "heading", + [ + "£ billion", + "£bn", + "£ billions", + "£ billion unless otherwise stated", + "Memo: £ billion", + ], +) +def test_a_sub_heading_restating_the_tables_own_unit_is_not_refused(heading): + assert set(obr_sources._TABLE_UNIT_SPELLINGS) == set(obr_sources.UNIT_SCALES) + ws = load_workbook("expenditure")["4.19"] + row = obr._find_row(ws, "BBC licence fee receipts", max_row=30) + values = read_row(ws, row, MANIFEST) + ws[f"A{row - 1}"] = heading + assert read_row(ws, row, MANIFEST) == values + + +_TIMES = st.datetimes( + min_value=datetime(2000, 1, 1), + max_value=datetime(2100, 1, 1), + timezones=st.sampled_from( + [timezone.utc, timezone(timedelta(hours=1)), timezone(-timedelta(hours=9.5))] + ), +) + + +@settings(max_examples=50, deadline=None) +@given(basis=st.sampled_from(["mtime", "given", "unrecorded"]), when=_TIMES) +def test_a_manifest_reads_back_as_written(basis, when): + """Each download time and its basis survive the manifest YAML.""" + files = { + kind: replace( + f, + downloaded_at=None if basis == "unrecorded" else when.isoformat(), + downloaded_at_basis=basis, + ) + for kind, f in MANIFEST.files.items() + } + manifest = obr_sources.ObrManifest(**{**MANIFEST.__dict__, "files": files}) + alternatives = [[sorted(g) for g in groups] for groups in MANIFEST.alternatives] + back = parse_manifest( + yaml.safe_load(obr_sources._manifest_yaml(manifest, alternatives)) + ) + assert back.files == files + assert (back.required, back.alternatives) == ( + MANIFEST.required, + MANIFEST.alternatives, + ) + + +def test_a_dry_run_returns_the_manifest_a_real_ingest_writes(tmp_path): + sources = _sources(tmp_path) + dry = _ingest(tmp_path / "dry", sources=sources, dry_run=True) + real = _ingest(tmp_path / "real", sources=sources) + assert dry.manifest.files == real.manifest.files + assert {f.downloaded_at_basis for f in real.manifest.files.values()} == {"mtime"} + + +def test_the_march_download_times_are_unrecorded(): + """The committed workbooks came from git (11e41f9d), with no download time.""" + assert { + (f.downloaded_at, f.downloaded_at_basis) for f in MANIFEST.files.values() + } == {(None, "unrecorded")} + + +@pytest.mark.parametrize( + "fields, match", + [ + ({"downloaded_at": "2026-10-28T13:05:00+00:00"}, "with a downloaded_at"), + ( + { + "downloaded_at": "2026-10-28T13:05:00+00:00", + "downloaded_at_basis": "unrecorded", + }, + "must be mtime or given", + ), + ({"downloaded_at_basis": "mtime"}, "with no downloaded_at it must be"), + ({"downloaded_at_basis": None}, "must be unrecorded"), + ( + { + "downloaded_at": "2026-10-28T13:05:00+00:00", + "downloaded_at_basis": "guessed", + }, + "'guessed'", + ), + ], +) +def test_a_download_time_without_a_matching_basis_is_refused(fields, match): + raw = yaml.safe_load(MANIFEST.path.read_text()) + entry = raw["files"]["receipts"] + entry.pop("downloaded_at_basis") + entry.update(fields) + with pytest.raises(SourceError, match=match): + parse_manifest(raw) From 159db44eecbecd5ce19bbf6e9e33dda488a31e05 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 23:18:36 -0400 Subject: [PATCH 6/7] Show the own-unit sub-heading test reads the heading; rewrap two docstrings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test that a "£ billion" sub-heading under table 4.19's "£ billion unless otherwise stated" is not refused now also puts another unit in the same cell and expects a refusal, so it cannot pass by the heading going unread. Two docstring lines over the line length are rewrapped. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/targets/obr_sources.py | 14 ++++++++------ policyengine_uk_data/tests/test_obr_sources.py | 7 ++++++- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/policyengine_uk_data/targets/obr_sources.py b/policyengine_uk_data/targets/obr_sources.py index 93b813909..dafe7bde5 100644 --- a/policyengine_uk_data/targets/obr_sources.py +++ b/policyengine_uk_data/targets/obr_sources.py @@ -31,10 +31,11 @@ folder and then moves them under ``storage/obr_efo//``. It writes the manifest with each file's SHA-256, size and download time, and the basis of that time: ``mtime``, the file's modification time, or ``given`` -with ``--downloaded-at``, which a copied or unzipped file needs. Each part is written under a temporary name and -renamed into place, so an interrupted ingest leaves a ``.partial`` file that -the next attempt names. It never replaces an ingested vintage. Point -``sources.yaml`` at the new manifest to adopt it. +with ``--downloaded-at``, which a copied or unzipped file needs. Each part +is written under a temporary name and renamed into place, so an interrupted +ingest leaves a ``.partial`` file that the next attempt names. It never +replaces an ingested vintage. Point ``sources.yaml`` at the new manifest to +adopt it. ``--dry-run`` checks everything and prints each workbook's title, SHA-256, size and download time and every target's old and new value, writing @@ -576,8 +577,9 @@ def ingest( and a download page that is not https on obr.uk. A dry run returns those as ``refusals`` instead; it checks everything else as a real ingest does, raising on any other problem, returns the diff and writes nothing. The - manifest it returns is not on disk. A dry run of the active vintage on the bytes its - manifest names is a rehearsal, so that manifest is not a refusal. + manifest it returns is not on disk. A dry run of the active vintage on + the bytes its manifest names is a rehearsal, so that manifest is not a + refusal. Everything is checked in a staging folder first: both workbooks read and carry the vintage's titles, and the build's parsers yield every diff --git a/policyengine_uk_data/tests/test_obr_sources.py b/policyengine_uk_data/tests/test_obr_sources.py index 9bd63c1d0..71f2200f9 100644 --- a/policyengine_uk_data/tests/test_obr_sources.py +++ b/policyengine_uk_data/tests/test_obr_sources.py @@ -5,7 +5,8 @@ - other bytes, or a workbook with another title, are refused; - year columns follow the header row, not the column letter; - a missing target or year fails; -- ingesting a vintage reproduces the build's own targets. +- ingesting a vintage reproduces the build's own targets; +- a dry run reports exactly what the real ingest would refuse. """ import io @@ -832,6 +833,10 @@ def test_a_sub_heading_restating_the_tables_own_unit_is_not_refused(heading): values = read_row(ws, row, MANIFEST) ws[f"A{row - 1}"] = heading assert read_row(ws, row, MANIFEST) == values + # The same cell giving another unit is refused, so the heading is read. + ws[f"A{row - 1}"] = heading.replace("billion", "million").replace("bn", "m") + with pytest.raises(SourceError, match="sub-heading"): + read_row(ws, row, MANIFEST) _TIMES = st.datetimes( From 133632aff00b005aea129332253765194855a0ab Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 23:57:43 -0400 Subject: [PATCH 7/7] Give the download pages in the documented ingest command A dry run now exits 1 when the real ingest would refuse, and a missing --receipts-url or --expenditure-url is a refusal, so the module's example command names both obr.uk pages. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/targets/obr_sources.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/policyengine_uk_data/targets/obr_sources.py b/policyengine_uk_data/targets/obr_sources.py index dafe7bde5..8157ec73d 100644 --- a/policyengine_uk_data/targets/obr_sources.py +++ b/policyengine_uk_data/targets/obr_sources.py @@ -19,6 +19,8 @@ python -m policyengine_uk_data.targets.obr_sources ingest \\ --vintage october_2026 --label "October 2026" \\ --receipts receipts.xlsx --expenditure expenditure.xlsx \\ + --receipts-url https://obr.uk/download// \\ + --expenditure-url https://obr.uk/download// \\ --first-year 2025 --last-year 2031 --dry-run obr.uk refuses scripted downloads (HTTP 403 to GitHub Actions runners, as