From 3493273bff5079d441df379f0cea50eb5f5bcbb1 Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Tue, 30 Jun 2026 14:32:16 -0700 Subject: [PATCH] Fix build_env after #8: port to modular expert_harmonizer PR #8 replaced the monolith + ablate_monolith with the modular harmonize_sm package + expert_harmonizer, but build_env (from #6) still imported the deleted ablate_monolith -> ModuleNotFoundError, breaking main's test collection. Port: build_env now copies the kept modules (common.py + non-held-out dataset_NN.py) into the env's data/gold/expert_code/harmonize_sm/ (the path the refactored harmonizer skill reads) instead of writing one ablated notebooks/*.py. Uses expert_harmonizer.kept_module_paths/kept_indices/block_indices/resolve_holdout. Tests updated to the modular fixture. Full suite green (36 + doctests). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/folds/build_env.py | 62 +++++++++++----------- tests/test_build_env.py | 111 +++++++++++++++++++++------------------- 2 files changed, 91 insertions(+), 82 deletions(-) diff --git a/src/folds/build_env.py b/src/folds/build_env.py index 0048557..e1ee8ce 100644 --- a/src/folds/build_env.py +++ b/src/folds/build_env.py @@ -6,24 +6,23 @@ materialize a per-config sandbox under ``.runs//`` containing only: * the **skills** (curator + harmonizer), copied verbatim; -* the **ablated mapping JSON** — the gold mapping with the held-out cluster's +* the **filtered mapping JSON** — the gold mapping with the held-out cluster's entries removed, written to the path the skills read for exemplars (``data/processed/ess-dive_wfsfa_soil_datasets/sm_data_harmonization_mapping.json``); -* the **ablated monolith** — the expert harmonization script with the held-out - blocks spliced out (see :mod:`src.folds.ablate_monolith`), written to the path - the harmonizer reads as a code-pattern reference - (``notebooks/harmonize_ess-dive_soilmoisture_data.py``). +* the **held-out-free expert code** — ``common.py`` plus the kept + ``dataset_NN.py`` modules of the modular expert harmonizer (see + :mod:`src.folds.expert_harmonizer`), copied to the path the harmonizer reads + as a code-pattern reference (``data/gold/expert_code/harmonize_sm/``). The + held-out cluster's modules are simply not copied. -Isolation is by *absence*: the two answer-bearing artifacts in the env have the -held-out cluster removed, and they sit at the exact relative paths the skills -resolve, so the skill picks up the ablated copy rather than the original. Shared -*inputs* are deliberately NOT treated as leakage and stay where they are: +Isolation is by *absence*: the held-out cluster is missing from both the +exemplar mapping and the reference modules, which sit at the exact relative +paths the skills resolve. Shared *inputs* are deliberately NOT treated as +leakage and stay where they are: * raw per-dataset CSVs at ``~/ess-dive_wfsfa_soil_datasets//`` — read by generated code via an absolute home path, so they need no env copy; -* cached ESS-DIVE metadata — optionally symlinked in via ``metadata_dir`` (the - curator reads it to evaluate a dataset; seeing a dataset's own metadata is the - task input, not the answer). +* cached ESS-DIVE metadata — optionally symlinked in via ``metadata_dir``. The agent can in principle escape the sandbox (``../`` or absolute paths); the backstop is instruction (``AGENT_INSTRUCTIONS.md``) plus a post-hoc trace audit @@ -38,11 +37,12 @@ import typer -from src.folds.ablate_monolith import ( - DEFAULT_INPUT as DEFAULT_MONOLITH, +from src.folds.expert_harmonizer import ( DEFAULT_MAPPING, - ablate, + PACKAGE_DIR as DEFAULT_PACKAGE, block_indices, + kept_indices, + kept_module_paths, resolve_holdout, ) @@ -50,7 +50,7 @@ DEFAULT_ENV_ROOT = Path(".runs") # Where each artifact lands inside the env, matching the skills' read paths. -MONOLITH_REL = Path("notebooks/harmonize_ess-dive_soilmoisture_data.py") +HARMONIZER_REL = Path("data/gold/expert_code/harmonize_sm") MAPPING_REL = Path("data/processed/ess-dive_wfsfa_soil_datasets/sm_data_harmonization_mapping.json") METADATA_REL = Path("data/external/ess-dive_meta") @@ -80,7 +80,7 @@ def build_env( holdout: set[int], name: Optional[str] = None, env_root: Path = DEFAULT_ENV_ROOT, - monolith_path: Path = DEFAULT_MONOLITH, + package_dir: Path = DEFAULT_PACKAGE, mapping_path: Path = DEFAULT_MAPPING, skills_dir: Path = DEFAULT_SKILLS, metadata_dir: Optional[Path] = None, @@ -89,12 +89,13 @@ def build_env( Returns the env directory. Overwrites an existing env of the same name. """ + holdout = set(holdout) mapping = json.loads(Path(mapping_path).read_text()) - source = Path(monolith_path).read_text() - # ablate() raises if a hold-out index has no block (rejects REF_IDX 0 and - # the excluded datasets) — fail loudly before writing anything. - ablated_py = ablate(source, holdout) + # kept_module_paths() raises if a hold-out index has no module (rejects + # REF_IDX 0 and the excluded datasets) — fail loudly before writing anything. + kept_paths = kept_module_paths(holdout, package_dir) + kept = kept_indices(holdout, package_dir) filtered = filter_mapping(mapping, holdout) env = Path(env_root) / (name or default_name(holdout)) @@ -102,8 +103,11 @@ def build_env( shutil.rmtree(env) env.mkdir(parents=True) - (env / MONOLITH_REL).parent.mkdir(parents=True, exist_ok=True) - (env / MONOLITH_REL).write_text(ablated_py) + # held-out-free expert code: copy common.py + the kept dataset modules + pkg_dest = env / HARMONIZER_REL + pkg_dest.mkdir(parents=True, exist_ok=True) + for path in kept_paths: + shutil.copy(path, pkg_dest / path.name) (env / MAPPING_REL).parent.mkdir(parents=True, exist_ok=True) (env / MAPPING_REL).write_text(json.dumps(filtered, indent=2)) @@ -120,10 +124,10 @@ def build_env( "name": env.name, "holdout_indices": sorted(holdout), "holdout_identifiers": held_ids, - "exemplar_indices": block_indices(ablated_py), + "exemplar_indices": kept, "n_exemplars": len(filtered), "sources": { - "monolith": str(monolith_path), + "package": str(package_dir), "mapping": str(mapping_path), "skills": str(skills_dir), }, @@ -140,7 +144,7 @@ def _instructions(held_ids: list) -> str: "Harmonize the held-out dataset(s) below using ONLY:\n" "- the skills in `skills/`,\n" "- the exemplars in `data/processed/.../sm_data_harmonization_mapping.json` " - "and the code patterns in `notebooks/harmonize_ess-dive_soilmoisture_data.py` " + "and the code patterns in `data/gold/expert_code/harmonize_sm/` " "(both have the held-out cluster removed),\n" "- the shared raw inputs under `~/ess-dive_wfsfa_soil_datasets/` and the " "cached metadata under `data/external/ess-dive_meta/`.\n\n" @@ -158,7 +162,7 @@ def main( holdout: str = typer.Option(..., "--holdout", help="Comma-separated indices or dataset_identifiers to hold out."), name: Optional[str] = typer.Option(None, "--name", help="Env dir name (default: holdout-)."), env_root: Path = typer.Option(DEFAULT_ENV_ROOT, "--env-root", help="Parent dir for run environments."), - monolith: Path = typer.Option(DEFAULT_MONOLITH, "--monolith", help="Expert monolith to ablate."), + package: Path = typer.Option(DEFAULT_PACKAGE, "--package", help="Modular expert harmonizer dir to draw modules from."), mapping: Path = typer.Option(DEFAULT_MAPPING, "--mapping", help="Gold mapping JSON."), skills: Path = typer.Option(DEFAULT_SKILLS, "--skills", help="Skills dir to copy."), metadata_dir: Optional[Path] = typer.Option(None, "--metadata-dir", help="Cached ESS-DIVE metadata to symlink in."), @@ -166,11 +170,11 @@ def main( """Assemble `.runs//` for one hold-out config.""" holdout_idx = resolve_holdout(holdout.split(","), mapping) env = build_env( - holdout_idx, name=name, env_root=env_root, monolith_path=monolith, + holdout_idx, name=name, env_root=env_root, package_dir=package, mapping_path=mapping, skills_dir=skills, metadata_dir=metadata_dir, ) typer.echo(f"built {env} (held out {sorted(holdout_idx)}; " - f"{len(block_indices((env / MONOLITH_REL).read_text()))} exemplar blocks remain)") + f"{len(block_indices(env / HARMONIZER_REL))} exemplar modules remain)") if __name__ == "__main__": diff --git a/tests/test_build_env.py b/tests/test_build_env.py index e26eaf3..1acd754 100644 --- a/tests/test_build_env.py +++ b/tests/test_build_env.py @@ -1,7 +1,6 @@ """Tests for the leave-one-cluster-out run-environment builder.""" from __future__ import annotations -import ast import json from pathlib import Path @@ -9,39 +8,14 @@ from typer.testing import CliRunner from src.folds.build_env import ( + HARMONIZER_REL, MAPPING_REL, - MONOLITH_REL, app, build_env, default_name, filter_mapping, ) -# Synthetic monolith: header + three dataset blocks + footer (mirrors the real -# `# %%` / `idx = N` structure) so the suite needs no corpus. -MONOLITH = """\ -# %% -acc = [] - -# %% -# Dataset 1 -idx = 1 -acc.append(("ds1", idx)) - -# %% -# Dataset 2 -idx = 2 -acc.append(("ds2", idx)) - -# %% -# Dataset 5 -idx = 5 -acc.append(("ds5", idx)) - -# %% -print(acc) # footer -""" - MAPPING = [ {"index": 1, "dataset_identifier": "ess-dive_a", "doi": "doi:1"}, {"index": 2, "dataset_identifier": "ess-dive_b", "doi": "doi:2"}, @@ -51,20 +25,22 @@ @pytest.fixture def sources(tmp_path): - """Write a synthetic monolith, mapping, and skills dir; return their paths.""" - mono = tmp_path / "monolith.py" - mono.write_text(MONOLITH) + """A synthetic modular harmonizer package + mapping + skills dir.""" + pkg = tmp_path / "harmonize_sm" + pkg.mkdir() + (pkg / "common.py").write_text("SHARED = 1\n") + for i in (1, 2, 5): # indices must be in expert_harmonizer.DATASET_INDICES + (pkg / f"dataset_{i:02d}.py").write_text(f"# dataset {i}\nIDX = {i}\n") mapping = tmp_path / "mapping.json" mapping.write_text(json.dumps(MAPPING)) skills = tmp_path / "skills" (skills / "essdive_sm_curator").mkdir(parents=True) (skills / "essdive_sm_curator" / "SKILL.md").write_text("curator") - return mono, mapping, skills + return pkg, mapping, skills def test_filter_mapping_drops_holdout_keeps_index(): - out = filter_mapping(MAPPING, {2}) - assert [e["index"] for e in out] == [1, 5] + assert [e["index"] for e in filter_mapping(MAPPING, {2})] == [1, 5] def test_default_name_is_sorted(): @@ -72,14 +48,16 @@ def test_default_name_is_sorted(): def test_build_env_layout_and_content(tmp_path, sources): - mono, mapping, skills = sources - env = build_env({2}, env_root=tmp_path / ".runs", monolith_path=mono, - mapping_path=mapping, skills_dir=skills) + pkg, mapping, skills = sources + env = build_env({2}, env_root=tmp_path / ".runs", package_dir=pkg, + mapping_path=mapping, skills_dir=skills) - # ablated monolith at the harmonizer's read path, holdout block gone, parseable - py = (env / MONOLITH_REL).read_text() - assert "ds2" not in py and "ds1" in py and "ds5" in py - ast.parse(py) + # held-out-free modular code at the harmonizer's read path + hdir = env / HARMONIZER_REL + assert (hdir / "common.py").exists() + assert (hdir / "dataset_01.py").exists() + assert (hdir / "dataset_05.py").exists() + assert not (hdir / "dataset_02.py").exists() # held out -> not copied # filtered mapping at the exemplar read path, holdout entry gone mp = json.loads((env / MAPPING_REL).read_text()) @@ -91,31 +69,30 @@ def test_build_env_layout_and_content(tmp_path, sources): assert manifest["holdout_indices"] == [2] assert manifest["holdout_identifiers"] == ["ess-dive_b"] assert manifest["exemplar_indices"] == [1, 5] - instr = (env / "AGENT_INSTRUCTIONS.md").read_text() - assert "ess-dive_b" in instr # held-out id named for the trace audit + assert "ess-dive_b" in (env / "AGENT_INSTRUCTIONS.md").read_text() def test_build_env_metadata_symlink(tmp_path, sources): - mono, mapping, skills = sources + pkg, mapping, skills = sources meta = tmp_path / "meta" meta.mkdir() (meta / "x.json").write_text("{}") - env = build_env({1}, env_root=tmp_path / ".runs", monolith_path=mono, + env = build_env({1}, env_root=tmp_path / ".runs", package_dir=pkg, mapping_path=mapping, skills_dir=skills, metadata_dir=meta) link = env / "data" / "external" / "ess-dive_meta" assert link.is_symlink() and (link / "x.json").exists() -def test_build_env_rejects_non_block_holdout(tmp_path, sources): - mono, mapping, skills = sources +def test_build_env_rejects_holdout_without_module(tmp_path, sources): + pkg, mapping, skills = sources with pytest.raises(ValueError): - build_env({99}, env_root=tmp_path / ".runs", monolith_path=mono, - mapping_path=mapping, skills_dir=skills) + build_env({3}, env_root=tmp_path / ".runs", package_dir=pkg, + mapping_path=mapping, skills_dir=skills) # no dataset_03.py def test_build_env_overwrites_existing(tmp_path, sources): - mono, mapping, skills = sources - kw = dict(env_root=tmp_path / ".runs", monolith_path=mono, mapping_path=mapping, skills_dir=skills) + pkg, mapping, skills = sources + kw = dict(env_root=tmp_path / ".runs", package_dir=pkg, mapping_path=mapping, skills_dir=skills) first = build_env({2}, name="cfg", **kw) (first / "stale.txt").write_text("old") second = build_env({1}, name="cfg", **kw) @@ -124,10 +101,38 @@ def test_build_env_overwrites_existing(tmp_path, sources): def test_cli_builds_env(tmp_path, sources): - mono, mapping, skills = sources + pkg, mapping, skills = sources result = CliRunner().invoke(app, [ "--holdout", "2", "--name", "cli", "--env-root", str(tmp_path / ".runs"), - "--monolith", str(mono), "--mapping", str(mapping), "--skills", str(skills), + "--package", str(pkg), "--mapping", str(mapping), "--skills", str(skills), ]) assert result.exit_code == 0, result.output - assert (tmp_path / ".runs" / "cli" / MONOLITH_REL).exists() + assert (tmp_path / ".runs" / "cli" / HARMONIZER_REL / "dataset_01.py").exists() + assert not (tmp_path / ".runs" / "cli" / HARMONIZER_REL / "dataset_02.py").exists() + + +# --- Against the real modular harmonizer, when present --- + +PKG = Path("data/gold/expert_code/harmonize_sm") +needs_pkg = pytest.mark.skipif(not PKG.exists(), reason="modular expert harmonizer not present") + + +@needs_pkg +def test_real_build_env_holds_out_cluster(tmp_path): + holdout = {1, 2, 3, 6, 16, 27} # cluster_1 "sg_ph_micromet" + env = build_env(holdout, env_root=tmp_path / ".runs") + hdir = env / HARMONIZER_REL + assert (hdir / "common.py").exists() + for i in holdout: + assert not (hdir / f"dataset_{i:02d}.py").exists() + for i in (4, 5, 7, 8, 9, 10, 15, 17, 18, 23, 24, 25, 26): + assert (hdir / f"dataset_{i:02d}.py").exists() + mp = json.loads((env / MAPPING_REL).read_text()) + assert not any(e["index"] in holdout for e in mp) + + +@needs_pkg +def test_real_build_env_rejects_ref_idx_and_excluded(tmp_path): + for bad in ({0}, {11}, {19}): + with pytest.raises(ValueError): + build_env(bad, env_root=tmp_path / ".runs")