Give builders their own directory, one per published dataset - #60
Merged
mmcky merged 2 commits intoAug 10, 2026
Merged
Conversation
scripts/ was two unrelated things sharing a name: seven builders that produce published datasets, and the dashboard/catalog toolchain that has nothing to do with data production. Its own README already apologised for the split with a "Not builders" section — while listing 1 of the 7. builders/<stem>.<ext> now builds lectures/<stem>.<ext2>. That is not a new convention: six of the seven already matched their output stem exactly, each with an explicit OUT_FILE = '<stem>.csv'. The move makes a latent pattern enforceable. Where one builder produces a SET of files, it is named for the set and several manifests point at the same path — business_cycle.py writes three, and both incoming notebook builders write two. So the stem rule is the default, not an invariant, and what CI asserts is weaker and truer: * builder_status must be a known value * a `committed*` status must name a builder * a named builder must exist on disk None of that was checked anywhere before — build_audit only records the value and build_catalog only formats it, so a manifest could assert a builder that was never committed, or one that had been moved. Which is exactly what this commit does to six of them. Adds `committed-frozen` to the enum: the builder is here and deliberately will not run, for a dataset built from a source that must not be refreshed. `committed` asserts a runnable four-stage builder and is a false claim for a frozen vintage; `unrecovered` says the builder is absent and is a false claim for one arriving in the same PR. This is the answer to #14's notebook question, and it lets the two high_dim_data builders land as what they are. Also records where a builder reads its input from, since sources/ is about to exist and is easy to misread: the normal case is the third-party upstream at run time — six of seven do that — and sources/ is only for an input that cannot be re-fetched. It is not a general input tree and not "the big-file directory"; the defining property is un-refetchability. Safe to do now, and cheaper now than later: no workflow runs any builder, nothing outside the repo references the paths, and PLAN's warning that the restructure window is spent applies to lectures/, where filenames are the public API — not to scripts/, which is never served. Doing it before the high_dim_data fold means its two builders land in the final shape. Verified: real tree 18/18 hash-checked exit 0 (which also proves the six moved paths resolve), strict audit exit 0, CATALOG.md regenerates byte-identical. Six negative cases exercised — missing builder path, a committed status with no builder, an unknown status, committed-frozen, and the verbatim/not-applicable shape that must keep passing. Part of #14. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Moves dataset builders out of scripts/ into a dedicated builders/ directory and updates docs/manifests/CI guardrails so builder metadata can be validated and referenced consistently.
Changes:
- Introduce
builders/(with README) and relocate/define builder scripts there. - Update manifests, schema docs, and planning docs to reference
builders/<name>.pyand addbuilder_status: committed-frozen. - Extend the consumed-files PR check to validate
builder_statusand builder-path existence.
Reviewed changes
Copilot reviewed 12 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/README.md | Reframes scripts/ as repo tooling and points builder location to builders/. |
| PLAN.md | Updates planning references from scripts/* builders to builders/*. |
| manifest-schema.yml | Updates example builder path and documents committed-frozen status. |
| lectures/us_adult_heights.csv.yml | Updates builder: path to builders/us_adult_heights.py. |
| lectures/japan_population_by_age.csv.yml | Updates builder: path to builders/japan_population_by_age.py. |
| lectures/japan_earthquakes.csv.yml | Updates builder: path to builders/japan_earthquakes.py. |
| lectures/japan_deaths_by_age.csv.yml | Updates builder: path to builders/japan_deaths_by_age.py. |
| lectures/epl_match_goals.csv.yml | Updates builder: path to builders/epl_match_goals.py. |
| lectures/ames_house_prices.csv.yml | Updates builder: path to builders/ames_house_prices.py. |
| builders/README.md | Documents builder naming rule, contract, and coverage table. |
| builders/ames_house_prices.py | Adds/relocates builder implementation for ames_house_prices.csv. |
| builders/epl_match_goals.py | Adds/relocates builder implementation for epl_match_goals.csv. |
| builders/japan_deaths_by_age.py | Adds/relocates builder implementation for japan_deaths_by_age.csv. |
| builders/japan_earthquakes.py | Adds/relocates builder implementation for japan_earthquakes.csv. |
| builders/japan_population_by_age.py | Adds/relocates builder implementation for japan_population_by_age.csv. |
| builders/us_adult_heights.py | Adds/relocates builder implementation for us_adult_heights.csv. |
| builders/business_cycle.py | Moves the manual business_cycle refresh script into builders/. |
| AGENTS.md | Documents committed-frozen and the new builders/tooling directory split. |
| .github/scripts/check_consumed_files.py | Adds builder-status/path consistency checks to the PR guardrail. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot review on #60, confirmed by reproduction — the builder checks had four holes, two of which crashed rather than reported. A non-string `builder_status` (a YAML list, say) raised `TypeError: unhashable type` on the set-membership test, and a non-string `builder` raised on `REPO / builder`. Both surfaced as a traceback with no `::error::` line — the same class of defect the review caught in #56, in a file that already handles it that way for `integrity` and for the manifest itself. The path check was satisfiable by things that are not builders in this repo. `REPO / builder` silently discards REPO when `builder` is absolute, so `/etc/hosts` passed; a relative path can climb out with `../`, so a real file outside the repo passed; and `.exists()` is true for directories, so naming `builders` passed. That last one the review did not name. Resolving and asserting `is_relative_to(REPO)` plus `is_file()` closes all three. Framing note: the read is correctness rather than security — the check only calls `.exists()`, never reads — but an assertion satisfiable by a file that is not a builder here establishes nothing, and the likely trigger is a typo'd relative path that happens to resolve on the runner, not an attack. Verified: real tree 18/18 exit 0; the six cases from the PR body unchanged; five new ones — list status, list builder, escaping relative path, absolute path, directory — all now fail cleanly with no traceback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky
added a commit
that referenced
this pull request
Aug 10, 2026
* Give builders their own directory, one per published dataset scripts/ was two unrelated things sharing a name: seven builders that produce published datasets, and the dashboard/catalog toolchain that has nothing to do with data production. Its own README already apologised for the split with a "Not builders" section — while listing 1 of the 7. builders/<stem>.<ext> now builds lectures/<stem>.<ext2>. That is not a new convention: six of the seven already matched their output stem exactly, each with an explicit OUT_FILE = '<stem>.csv'. The move makes a latent pattern enforceable. Where one builder produces a SET of files, it is named for the set and several manifests point at the same path — business_cycle.py writes three, and both incoming notebook builders write two. So the stem rule is the default, not an invariant, and what CI asserts is weaker and truer: * builder_status must be a known value * a `committed*` status must name a builder * a named builder must exist on disk None of that was checked anywhere before — build_audit only records the value and build_catalog only formats it, so a manifest could assert a builder that was never committed, or one that had been moved. Which is exactly what this commit does to six of them. Adds `committed-frozen` to the enum: the builder is here and deliberately will not run, for a dataset built from a source that must not be refreshed. `committed` asserts a runnable four-stage builder and is a false claim for a frozen vintage; `unrecovered` says the builder is absent and is a false claim for one arriving in the same PR. This is the answer to #14's notebook question, and it lets the two high_dim_data builders land as what they are. Also records where a builder reads its input from, since sources/ is about to exist and is easy to misread: the normal case is the third-party upstream at run time — six of seven do that — and sources/ is only for an input that cannot be re-fetched. It is not a general input tree and not "the big-file directory"; the defining property is un-refetchability. Safe to do now, and cheaper now than later: no workflow runs any builder, nothing outside the repo references the paths, and PLAN's warning that the restructure window is spent applies to lectures/, where filenames are the public API — not to scripts/, which is never served. Doing it before the high_dim_data fold means its two builders land in the final shape. Verified: real tree 18/18 hash-checked exit 0 (which also proves the six moved paths resolve), strict audit exit 0, CATALOG.md regenerates byte-identical. Six negative cases exercised — missing builder path, a committed status with no builder, an unknown status, committed-frozen, and the verbatim/not-applicable shape that must keep passing. Part of #14. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Guardrail: type-check the builder fields, and require an in-repo file Copilot review on #60, confirmed by reproduction — the builder checks had four holes, two of which crashed rather than reported. A non-string `builder_status` (a YAML list, say) raised `TypeError: unhashable type` on the set-membership test, and a non-string `builder` raised on `REPO / builder`. Both surfaced as a traceback with no `::error::` line — the same class of defect the review caught in #56, in a file that already handles it that way for `integrity` and for the manifest itself. The path check was satisfiable by things that are not builders in this repo. `REPO / builder` silently discards REPO when `builder` is absolute, so `/etc/hosts` passed; a relative path can climb out with `../`, so a real file outside the repo passed; and `.exists()` is true for directories, so naming `builders` passed. That last one the review did not name. Resolving and asserting `is_relative_to(REPO)` plus `is_file()` closes all three. Framing note: the read is correctness rather than security — the check only calls `.exists()`, never reads — but an assertion satisfiable by a file that is not a builder here establishes nothing, and the likely trigger is a typo'd relative path that happens to resolve on the runner, not an attack. Verified: real tree 18/18 exit 0; the six cases from the PR body unchanged; five new ones — list status, list builder, escaping relative path, absolute path, directory — all now fail cleanly with no traceback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
mmcky
added a commit
that referenced
this pull request
Aug 10, 2026
* Give builders their own directory, one per published dataset scripts/ was two unrelated things sharing a name: seven builders that produce published datasets, and the dashboard/catalog toolchain that has nothing to do with data production. Its own README already apologised for the split with a "Not builders" section — while listing 1 of the 7. builders/<stem>.<ext> now builds lectures/<stem>.<ext2>. That is not a new convention: six of the seven already matched their output stem exactly, each with an explicit OUT_FILE = '<stem>.csv'. The move makes a latent pattern enforceable. Where one builder produces a SET of files, it is named for the set and several manifests point at the same path — business_cycle.py writes three, and both incoming notebook builders write two. So the stem rule is the default, not an invariant, and what CI asserts is weaker and truer: * builder_status must be a known value * a `committed*` status must name a builder * a named builder must exist on disk None of that was checked anywhere before — build_audit only records the value and build_catalog only formats it, so a manifest could assert a builder that was never committed, or one that had been moved. Which is exactly what this commit does to six of them. Adds `committed-frozen` to the enum: the builder is here and deliberately will not run, for a dataset built from a source that must not be refreshed. `committed` asserts a runnable four-stage builder and is a false claim for a frozen vintage; `unrecovered` says the builder is absent and is a false claim for one arriving in the same PR. This is the answer to #14's notebook question, and it lets the two high_dim_data builders land as what they are. Also records where a builder reads its input from, since sources/ is about to exist and is easy to misread: the normal case is the third-party upstream at run time — six of seven do that — and sources/ is only for an input that cannot be re-fetched. It is not a general input tree and not "the big-file directory"; the defining property is un-refetchability. Safe to do now, and cheaper now than later: no workflow runs any builder, nothing outside the repo references the paths, and PLAN's warning that the restructure window is spent applies to lectures/, where filenames are the public API — not to scripts/, which is never served. Doing it before the high_dim_data fold means its two builders land in the final shape. Verified: real tree 18/18 hash-checked exit 0 (which also proves the six moved paths resolve), strict audit exit 0, CATALOG.md regenerates byte-identical. Six negative cases exercised — missing builder path, a committed status with no builder, an unknown status, committed-frozen, and the verbatim/not-applicable shape that must keep passing. Part of #14. * Guardrail: type-check the builder fields, and require an in-repo file Copilot review on #60, confirmed by reproduction — the builder checks had four holes, two of which crashed rather than reported. A non-string `builder_status` (a YAML list, say) raised `TypeError: unhashable type` on the set-membership test, and a non-string `builder` raised on `REPO / builder`. Both surfaced as a traceback with no `::error::` line — the same class of defect the review caught in #56, in a file that already handles it that way for `integrity` and for the manifest itself. The path check was satisfiable by things that are not builders in this repo. `REPO / builder` silently discards REPO when `builder` is absolute, so `/etc/hosts` passed; a relative path can climb out with `../`, so a real file outside the repo passed; and `.exists()` is true for directories, so naming `builders` passed. That last one the review did not name. Resolving and asserting `is_relative_to(REPO)` plus `is_file()` closes all three. Framing note: the read is correctness rather than security — the check only calls `.exists()`, never reads — but an assertion satisfiable by a file that is not a builder here establishes nothing, and the likely trigger is a typo'd relative path that happens to resolve on the runner, not an attack. Verified: real tree 18/18 exit 0; the six cases from the PR body unchanged; five new ones — list status, list builder, escaping relative path, absolute path, directory — all now fail cleanly with no traceback. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Answers the notebook-builder half of #14. Stacked on #59 — review that first; GitHub will retarget this to
mainwhen #59 merges.Why
scripts/is two unrelated things sharing a name: seven builders that produce published datasets, and the dashboard/catalog toolchain that produces none. Its own README already apologised for the split with a "Not builders" section — while listing 1 of the 7 builders in the table above it.The naming rule
builders/<stem>.<ext>buildslectures/<stem>.<ext2>.This is not a new convention — six of the seven already matched their output stem exactly, each with an explicit
OUT_FILE = '<stem>.csv'. The move makes a latent pattern enforceable.The exception is real and worth stating rather than pretending away: where one builder produces a set of files, it is named for the set and several manifests point at the same path.
business_cycle.pywrites three; both incominghigh_dim_databuilders write two. So the stem rule is the default, not an invariant.What CI now asserts
Weaker than "unique stem", and truer:
builder_statusis a known valuecommitted*status names a builderNone of this was checked anywhere before.
build_audit.pyrecords the value andbuild_catalog.pyformats it with astr()fallback; nothing compares either against reality. The third assertion is exactly the failure this PR would otherwise have introduced to six manifests.committed-frozenNew enum value: the builder is here, and deliberately will not run — for a dataset built from a source that must not be refreshed.
This is the answer to #14's notebook question.
committedasserts a runnable four-stage builder, which is a false claim for a frozen vintage or a scraper we will not re-run.unrecoveredsays the builder is absent, which is a false claim for one arriving in the same PR. Neither was true ofgenerating_mini.mdorwebscrape_forbes.ipynb, and since nothing validated the enum, whatever the fold happened to write would have become the precedent.With location carried by the directory,
builder_statusgets to be purely about runnability.Also records where a builder reads its input from
sources/is about to exist and is easy to misread. The normal case is the third-party upstream fetched at run time — six of the seven builders do exactly that (jse.amstat.org,earthquake.usgs.gov,wwwn.cdc.gov,stat.go.jp,openfootball). A builder reads fromsources/only when the input cannot be re-fetched. It is not a general input tree, and it is not "the big-file directory" — the defining property is un-refetchability, not size.Why it is safe to do now
No workflow runs any builder. Nothing outside the repo references the paths — the blast radius was 6 manifest lines, one example in
manifest-schema.yml, and four prose mentions inPLAN.md.CATALOG.mdregenerates byte-identical (its onlyscripts/mentions are the generator command, which has not moved). Git tracked all seven as renames, so history follows.PLAN's warning that "the restructure window is spent" applies to
lectures/, where filenames are the public API — not toscripts/, which is never served. And doing it before thehigh_dim_datafold means its two builders land in the final shape rather than being moved afterwards.Verification
Real tree: 18/18 hash-checked, exit 0 — which also proves the six moved paths resolve, since that is the third assertion. Strict audit exit 0. Six negative cases exercised in a scratch tree: missing builder path → fail;
committedwith no builder → fail; unknown status → fail;committed-frozenwith a builder → pass; and the verbatimnot-applicable-with-no-builder-key shape (countries.csv's) → pass, which is the one that must keep working.🤖 Generated with Claude Code