Skip to content

Validation fixes: pandas-3 band check (#128), relative-path annotation (#129), and the builder layer in CI - #131

Merged
mmcky merged 2 commits into
mainfrom
fix/validation-128-129
Sep 7, 2026
Merged

Validation fixes: pandas-3 band check (#128), relative-path annotation (#129), and the builder layer in CI#131
mmcky merged 2 commits into
mainfrom
fix/validation-128-129

Conversation

@mmcky

@mmcky mmcky commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Both regressions were filed by the #127 validation run and are reproduced and fixed here, together with the CI gap that let the first one sit unseen for a week.

The two fixes

#128. builders/business_cycle.py checked value bands with values.stack().between(lo, hi).all(). pandas 3's stack() keeps NaN, so the structural nulls the manifest places (every economy's YR1960, GBR's and FRA's leading years, an unpublished newest year) failed between() and all three World Bank tables were rejected under the lectures' pandas. The line dates from #114; #126 kept it. Fix: .dropna() before the band check, with a comment naming the trap, and a note in _template.py so the next builder does not inherit it.

#129. scripts/validate_datasets.py resolved REPO but used the CLI's paths as given, so a relative manifest path that failed crashed on relative_to(REPO) instead of printing the annotation. Fix: resolve the arguments once. A relative-path run now prints ::error file=lectures/lingcod_msy_recovery.csv.yml::F_over_Fmsy: 1 nulls, manifest says exactly 2.

Closing the gap

The workflow's own comment said it was "the standing check that it stays green on both" pandas majors. It ran one major and one layer. Now:

  • each dynamic builder exposes check_committed(): its own validate() on the committed bytes, no network (business_cycle.py, business_cycle_fred.py, and the template);
  • validate_datasets.py --builders runs them, once per builder even when several manifests name the same one;
  • validate-datasets.yml is a matrix over pandas 2.3.3 and 3.x and runs both layers in each.

Verified locally: with the pre-fix band line, --builders under pandas 3 fails with ::error file=builders/business_cycle.py::ValidationError: gdp_growth_annual.csv: value out of band [-50, 50]; with the fix, both layers pass under 2.3.3 and 3.0.5 (44 of 44 manifests; 4 committed snapshots through their builders).

Closes #128. Closes #129.

🤖 Generated with Claude Code

#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 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 7, 2026 05:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new --builders validation path has a robustness bug that can crash without emitting annotations for malformed manifests, and the CI pandas-3 install is unpinned in a way that can cause non-reproducible failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes two validator regressions discovered during the #127 independent validation run and strengthens CI so builder-specific validation runs across both pandas majors (shared schema validator + each dynamic builder’s own committed-bytes checks).

Changes:

  • Fix builders/business_cycle.py band validation under pandas 3 by dropping NaNs before the between() check.
  • Fix scripts/validate_datasets.py handling of relative CLI manifest paths and add --builders to run each committed dynamic builder’s check_committed() validation.
  • Update CI (validate-datasets.yml) to run both validation layers under a pandas 2.3.3 and pandas 3.x matrix; document the new layer in builder docs.
File summaries
File Description
scripts/validate_datasets.py Adds --builders builder-layer validation and fixes relative-path argument resolution.
builders/business_cycle.py Fixes pandas 3 stack() NaN behavior in band checks; adds check_committed().
builders/business_cycle_fred.py Adds check_committed() for builder-layer validation.
builders/_template.py Documents the pandas-3 stack()/NaN pitfall and adds a check_committed() template.
.github/workflows/validate-datasets.yml Runs validation as a pandas-version matrix and executes both shared and builder layers.
builders/README.md Documents that CI runs check_committed() for dynamic builders too.
AGENTS.md Documents the check_committed() requirement and how CI uses it.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/validate_datasets.py
Comment thread .github/workflows/validate-datasets.yml
…nstead of raising; pin the pandas 3 leg to 3.0.5

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mmcky
mmcky merged commit 47017ea into main Sep 7, 2026
3 checks passed
@mmcky
mmcky deleted the fix/validation-128-129 branch September 7, 2026 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants