From 5ce1a02594591f12ac2b9863c25dbc9701a0c08e Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Mon, 7 Sep 2026 15:15:50 +1000 Subject: [PATCH 1/2] Validation fixes: pandas-3 band check (#128), relative-path annotation (#129), and the builder layer in CI Both found by the #127 validation run. - builders/business_cycle.py: .dropna() before the band check. pandas 3's stack() keeps NaN, so the manifest's placed nulls failed between() and every World Bank table was rejected under the lectures' pandas -- the line dates from #114 and no check ran the builder layer under pandas 3. - scripts/validate_datasets.py: resolve argument paths; a relative manifest path crashed the failure report on relative_to(REPO) instead of printing the ::error annotation. - The gap that let #128 through is closed: each dynamic builder exposes check_committed() (its own validate() on the committed bytes, no network), `validate_datasets.py --builders` runs them once per builder, and validate-datasets.yml is now a matrix over pandas 2.3.3 and 3.x running both layers. The pre-fix band line fails that job under pandas 3 with a file-anchored annotation; the fixed one passes under both. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/validate-datasets.yml | 25 ++++++++++++---- AGENTS.md | 2 +- builders/README.md | 4 ++- builders/_template.py | 15 +++++++++- builders/business_cycle.py | 16 +++++++++- builders/business_cycle_fred.py | 8 +++++ scripts/validate_datasets.py | 40 ++++++++++++++++++++++++- 7 files changed, 100 insertions(+), 10 deletions(-) diff --git a/.github/workflows/validate-datasets.yml b/.github/workflows/validate-datasets.yml index 747bfda..ac06805 100644 --- a/.github/workflows/validate-datasets.yml +++ b/.github/workflows/validate-datasets.yml @@ -16,10 +16,13 @@ # consumed-file-check.yml stays beside this as the byte-integrity (sha256) and # catalog-freshness gate; this job is the schema half PLAN Phase 5 promised. # -# pandas is pinned to requirements.txt's version (2.3.3), deliberately one -# major behind the lectures' anaconda=2026.07 (pandas 3): the validator -# compares dtypes by family precisely so it is green on both, and running the -# older one here is the standing check that it stays so. +# Runs as a matrix over BOTH pandas majors -- requirements.txt's 2.3.3 (the +# canary's environment) and 3.x (the lectures' anaconda=2026.07) -- and runs +# BOTH validation layers: the shared manifest-driven one, and each dynamic +# builder's own validate() on its committed bytes (`--builders`, no network). +# The 2.3.3-only, shared-layer-only first version claimed to be "the standing +# check that it stays green on both" and was not: business_cycle.py's band +# check failed under pandas 3 for a week without anything noticing (#128). name: validate-datasets @@ -31,6 +34,11 @@ on: jobs: validate: runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + pandas: ["2.3.3", "3"] + name: validate (pandas ${{ matrix.pandas }}) steps: - uses: actions/checkout@v4 with: @@ -38,5 +46,12 @@ jobs: - uses: actions/setup-python@v5 with: python-version: "3.12" - - run: pip install pandas==2.3.3 PyYAML==6.0.3 + # wbgapi: builders/business_cycle.py imports it at module level, and the + # builder layer imports the module (it never calls the network here). + - run: pip install "pandas==${{ matrix.pandas }}.*" PyYAML==6.0.3 wbgapi==1.0.12 + if: matrix.pandas != '3' + - run: pip install "pandas>=3,<4" PyYAML==6.0.3 wbgapi==1.0.12 + if: matrix.pandas == '3' + - run: python -c "import pandas; print('pandas', pandas.__version__)" - run: python scripts/validate_datasets.py + - run: python scripts/validate_datasets.py --builders diff --git a/AGENTS.md b/AGENTS.md index ca703be..7aa0934 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,7 +122,7 @@ Where one builder produces a **set** of files, name it for the set and let each **Where a builder reads its input from.** The normal case is the third-party upstream, fetched at run time: eight of the nine `committed` builders here do that, and it is the fetch stage of the contract below. A builder reads from `sources/` **only when the input cannot be re-fetched** — the upstream is gone, unlocatable, or was inherited with no recoverable source. `sources/` is that exception layer, not a general input tree, and it is emphatically not "the big-file directory": the defining property is un-refetchability, not size. What it must never be is a network read from another QuantEcon repo — that is how a retired repo becomes load-bearing again. -Builders follow four stages — **fetch → pre-process → validate → write** — and only write on validation pass. **The validate stage is shared**: `builders/_validate.py` reads the manifest's `schema` block as its spec (columns and `pattern` runs, dtype families, exact `known_nulls`, the `nulls:` placement rule, `row_count_floor`, `date_range`) and measures the overlap window against the previous vintage; a builder calls `validate(frame.reset_index(), manifest, previous)` and layers on only what the schema cannot say — value bands, a grid check, the per-series revision **bound** (a tracking snapshot is revised by its source, so the test is a tolerance plus a printed summary, never equality). The same function runs over every committed CSV on every PR (`scripts/validate_datasets.py`, `validate-datasets.yml`), so a manifest that drifts from its bytes fails the PR, not the next refresh. Lectures always read the last-good snapshot: an upstream outage may fail a refresh, it must never break a lecture build. +Builders follow four stages — **fetch → pre-process → validate → write** — and only write on validation pass. **The validate stage is shared**: `builders/_validate.py` reads the manifest's `schema` block as its spec (columns and `pattern` runs, dtype families, exact `known_nulls`, the `nulls:` placement rule, `row_count_floor`, `date_range`) and measures the overlap window against the previous vintage; a builder calls `validate(frame.reset_index(), manifest, previous)` and layers on only what the schema cannot say — value bands, a grid check, the per-series revision **bound** (a tracking snapshot is revised by its source, so the test is a tolerance plus a printed summary, never equality). The same function runs over every committed CSV on every PR (`scripts/validate_datasets.py`, `validate-datasets.yml`), so a manifest that drifts from its bytes fails the PR, not the next refresh. **A dynamic builder also exposes `check_committed()`** — its own `validate()` on the committed bytes, no network — which the same workflow runs (`--builders`) under both pandas majors; a builder-specific check that breaks on a pandas change fails the PR that introduces it (#128 was a week-old pandas-3 break the shared layer could not see). Lectures always read the last-good snapshot: an upstream outage may fail a refresh, it must never break a lecture build. **One builder per source, composite files where a lecture reads series together, shared fetch libraries** (decided 2026-09-01, #26): `builders/_fred.py` is the `Fred` class for FRED reads — no `pandas_datareader` in a builder — and a lecture's FRED data is one file with the lecture's own variable names as columns, not six. A builder that writes a set validates every file before writing any. diff --git a/builders/README.md b/builders/README.md index 27c4f43..9cb9191 100644 --- a/builders/README.md +++ b/builders/README.md @@ -39,7 +39,9 @@ and measures the overlap window against the previous vintage calls `validate(frame.reset_index(), manifest, previous)` and adds only what a schema cannot say — value bands, a grid, recency, the revision *bound*. The same function runs over every committed CSV on every PR -(`scripts/validate_datasets.py`, `validate-datasets.yml`). One +(`scripts/validate_datasets.py`, `validate-datasets.yml`), and each dynamic +builder's `check_committed()` runs its own layer there too, under both pandas +majors, with no network. One builder per **source** for a lecture's data, writing a composite file where the lecture reads the series together (decided 2026-09-01 on #26). diff --git a/builders/_template.py b/builders/_template.py index 8543d6e..fab8c6f 100644 --- a/builders/_template.py +++ b/builders/_template.py @@ -85,7 +85,10 @@ def validate(frame, previous=None): raw = frame.reset_index() if frame.index.name else frame prev_raw = previous.reset_index() if previous is not None and previous.index.name else previous shared = validate_schema(raw, manifest, prev_raw) - # builder-specific: bands / grid / recency ... + # builder-specific: bands / grid / recency ... Two idioms that bit once: + # call .dropna() BEFORE a band check (pandas 3's stack() keeps NaN, #128), + # and never compare against a dtype string -- the shared validator already + # did that by family (#122). raise NotImplementedError summary = { 'dataset': OUT_FILE, @@ -101,6 +104,16 @@ def validate(frame, previous=None): return summary +def check_committed(): + """Builder-layer validation of the COMMITTED file(s), no network. Called by + scripts/validate_datasets.py --builders on every PR, under both pandas + majors, so a builder-specific check that breaks on a pandas change fails + the PR rather than the next canary. Yield each file validated.""" + frame = pd.read_csv(os.path.join(PUBLISHED_DIR, OUT_FILE), index_col=0) + validate(frame) + yield OUT_FILE + + def _atomic_write(path, text): tmp = path + '.tmp' with open(tmp, 'w') as f: diff --git a/builders/business_cycle.py b/builders/business_cycle.py index f6df1e7..a0bd58b 100644 --- a/builders/business_cycle.py +++ b/builders/business_cycle.py @@ -141,7 +141,10 @@ def validate(table, frame, previous=None): values = frame[year_cols] _check(values.abs().max().max() >= table['min_abs_max'], f'{name}: values look like ratios') lo, hi = table['band'] - _check(values.stack().between(lo, hi).all(), f'{name}: value out of band [{lo}, {hi}]') + # .dropna() is load-bearing: pandas 3's stack() keeps NaN (no dropna= any + # more), and the structural nulls the manifest places would fail between(). + # Found by the #127 validation (#128). + _check(values.stack().dropna().between(lo, hi).all(), f'{name}: value out of band [{lo}, {hi}]') _check(years[-1] >= dt.date.today().year - MAX_STALENESS_YEARS, f'{name}: newest year is {years[-1]}') if table['first_year_null']: _check(values[year_cols[0]].isnull().all(), f'{name}: a value in {year_cols[0]} -- growth is undefined in the first year') @@ -167,6 +170,17 @@ def validate(table, frame, previous=None): return summary +def check_committed(): + """Run this builder's validate() on the COMMITTED files, no network — the + builder-layer half of PR validation (scripts/validate_datasets.py + --builders), so a pandas or builder change that would fail the next + canary fails the PR that introduces it instead (#128).""" + for table in TABLES: + frame = pd.read_csv(os.path.join(PUBLISHED_DIR, table['file']), index_col=0) + validate(table, frame) + yield table['file'] + + def _atomic_write(path, text): tmp = path + '.tmp' with open(tmp, 'w') as f: diff --git a/builders/business_cycle_fred.py b/builders/business_cycle_fred.py index c346e6d..d9bb9d8 100644 --- a/builders/business_cycle_fred.py +++ b/builders/business_cycle_fred.py @@ -146,6 +146,14 @@ def validate(frame, previous=None): return summary +def check_committed(): + """Builder-layer validation of the COMMITTED file, no network (see + scripts/validate_datasets.py --builders).""" + frame = pd.read_csv(os.path.join(PUBLISHED_DIR, OUT_FILE), index_col=0, parse_dates=True) + validate(frame) + yield OUT_FILE + + def _atomic_write(path, text): tmp = path + '.tmp' with open(tmp, 'w') as f: diff --git a/scripts/validate_datasets.py b/scripts/validate_datasets.py index daa56d9..f721f2b 100644 --- a/scripts/validate_datasets.py +++ b/scripts/validate_datasets.py @@ -7,6 +7,8 @@ python scripts/validate_datasets.py # everything under lectures/ python scripts/validate_datasets.py lectures/gdp_growth_annual.csv.yml ... + python scripts/validate_datasets.py --builders # each dynamic builder's own + # validate() on the committed bytes Two passes per manifest: @@ -91,8 +93,44 @@ def check_columns(cols, where): return errs +def builder_layer() -> int: + """The builder-layer half: every dynamic snapshot's own validate() on its + COMMITTED bytes, no network, through the builder's check_committed() + (#128 -- a builder check that broke under pandas 3 while the shared + layer stayed green, unseen until a validator dry-ran the builder).""" + import importlib + failed = 0 + seen = set() # a set-writing builder is named by several manifests; run it once + for path in sorted(LECTURES.glob('*.yml')): + m = yaml.safe_load(path.read_text(encoding='utf-8')) or {} + if m.get('class') != 'dynamic-snapshot' or m.get('builder_status') != 'committed': + continue + if m['builder'] in seen: + continue + seen.add(m['builder']) + mod_name = pathlib.Path(m['builder']).stem + try: + mod = importlib.import_module(mod_name) + fn = getattr(mod, 'check_committed', None) + if fn is None: + raise AttributeError(f'{m["builder"]} has no check_committed() (see builders/_template.py)') + for f in fn(): + print(f'{"ok builder layer":70s} {f} ({m["builder"]})') + except Exception as e: + failed += 1 + print(f'::error file={m["builder"]}::{type(e).__name__}: {e}') + print(f'FAIL {m["filename"]}: builder layer') + return failed + + def main(argv: list[str]) -> int: - paths = [pathlib.Path(a) for a in argv] or sorted(LECTURES.glob('*.yml')) + if argv and argv[0] == '--builders': + n = builder_layer() + print(f'\nbuilder layer: {"all green" if not n else f"{n} builder(s) failed"}') + return 1 if n else 0 + # Resolve the arguments: REPO is absolute and relative_to() does not + # resolve, so a relative path crashed the failure report (#129). + paths = [pathlib.Path(a).resolve() for a in argv] or sorted(LECTURES.glob('*.yml')) failed = 0 skipped_formats: dict[str, int] = {} for path in paths: From 84aa09d8b206553d62dd4ba5e2e0484585099b21 Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Mon, 7 Sep 2026 15:25:24 +1000 Subject: [PATCH 2/2] Copilot review: annotate a malformed dynamic manifest in --builders instead of raising; pin the pandas 3 leg to 3.0.5 Co-Authored-By: Claude Fable 5.1 --- .github/workflows/validate-datasets.yml | 11 +++++------ scripts/validate_datasets.py | 25 +++++++++++++++++-------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/.github/workflows/validate-datasets.yml b/.github/workflows/validate-datasets.yml index ac06805..9338967 100644 --- a/.github/workflows/validate-datasets.yml +++ b/.github/workflows/validate-datasets.yml @@ -17,7 +17,9 @@ # catalog-freshness gate; this job is the schema half PLAN Phase 5 promised. # # Runs as a matrix over BOTH pandas majors -- requirements.txt's 2.3.3 (the -# canary's environment) and 3.x (the lectures' anaconda=2026.07) -- and runs +# canary's environment) and 3.0.5 (what anaconda=2026.07, the lectures' pin, +# carries; bump it when that pin moves, never float it: a red here must be +# reproducible a week later) -- and runs # BOTH validation layers: the shared manifest-driven one, and each dynamic # builder's own validate() on its committed bytes (`--builders`, no network). # The 2.3.3-only, shared-layer-only first version claimed to be "the standing @@ -37,7 +39,7 @@ jobs: strategy: fail-fast: false matrix: - pandas: ["2.3.3", "3"] + pandas: ["2.3.3", "3.0.5"] name: validate (pandas ${{ matrix.pandas }}) steps: - uses: actions/checkout@v4 @@ -48,10 +50,7 @@ jobs: python-version: "3.12" # wbgapi: builders/business_cycle.py imports it at module level, and the # builder layer imports the module (it never calls the network here). - - run: pip install "pandas==${{ matrix.pandas }}.*" PyYAML==6.0.3 wbgapi==1.0.12 - if: matrix.pandas != '3' - - run: pip install "pandas>=3,<4" PyYAML==6.0.3 wbgapi==1.0.12 - if: matrix.pandas == '3' + - run: pip install pandas==${{ matrix.pandas }} PyYAML==6.0.3 wbgapi==1.0.12 - run: python -c "import pandas; print('pandas', pandas.__version__)" - run: python scripts/validate_datasets.py - run: python scripts/validate_datasets.py --builders diff --git a/scripts/validate_datasets.py b/scripts/validate_datasets.py index f721f2b..64b7b63 100644 --- a/scripts/validate_datasets.py +++ b/scripts/validate_datasets.py @@ -105,21 +105,30 @@ def builder_layer() -> int: m = yaml.safe_load(path.read_text(encoding='utf-8')) or {} if m.get('class') != 'dynamic-snapshot' or m.get('builder_status') != 'committed': continue - if m['builder'] in seen: + # A malformed manifest (no builder, no filename) is annotated on the + # sidecar, never a bare KeyError -- the conformance pass catches it + # first in CI, but --builders also runs on its own (Copilot on #131). + builder, filename = m.get('builder'), m.get('filename') or path.name[:-4] + sidecar = path.relative_to(REPO) + if not builder: + failed += 1 + print(f'::error file={sidecar}::dynamic snapshot with builder_status committed but no builder path') + print(f'FAIL {filename}: builder layer') + continue + if builder in seen: continue - seen.add(m['builder']) - mod_name = pathlib.Path(m['builder']).stem + seen.add(builder) try: - mod = importlib.import_module(mod_name) + mod = importlib.import_module(pathlib.Path(builder).stem) fn = getattr(mod, 'check_committed', None) if fn is None: - raise AttributeError(f'{m["builder"]} has no check_committed() (see builders/_template.py)') + raise AttributeError(f'{builder} has no check_committed() (see builders/_template.py)') for f in fn(): - print(f'{"ok builder layer":70s} {f} ({m["builder"]})') + print(f'{"ok builder layer":70s} {f} ({builder})') except Exception as e: failed += 1 - print(f'::error file={m["builder"]}::{type(e).__name__}: {e}') - print(f'FAIL {m["filename"]}: builder layer') + print(f'::error file={builder}::{type(e).__name__}: {e}') + print(f'FAIL {filename}: builder layer') return failed