From fb771c19805da23434afc7e73ada908ce2a21308 Mon Sep 17 00:00:00 2001 From: j Date: Sun, 28 Jun 2026 09:10:34 +0700 Subject: [PATCH 1/2] fix+feat: CSV column validation, disagreement gate, and config docs (#25) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CSV loader (loaders.py): Missing 'sequence' column now raises ValueError with the actual column names found, instead of a bare KeyError — prevents cryptic crashes from mis-formatted input files before synthesis. Disagreement gate (pipeline.py + configs): Add max_disagreement to eligibility filter: eligible = [...and scores.get("disagreement", 0.0) <= max_disagreement] pipeline.yaml: max_disagreement=0.40 (lenient) phase3.yaml: max_disagreement=0.30 (strict — final synthesis batch needs consensus between physchem-activity and Boman index scorer) Default is 1.0 (off) so existing callers without the key are unaffected. Science rationale: a candidate with activity=0.9 but boman_activity=0.2 has high uncertainty; blocking it prevents wasting synthesis budget on contradictory predictions. Tests (4 new): test_missing_sequence_column_raises_value_error test_missing_sequence_column_error_names_found_columns test_disagreement_gate_blocks_high_uncertainty_candidate test_disagreement_gate_config_values_documented --- configs/phase3.yaml | 1 + configs/pipeline.yaml | 1 + src/openamp_foundry/data/loaders.py | 7 ++++ src/openamp_foundry/pipeline.py | 2 ++ tests/test_data_loaders.py | 28 ++++++++++++++++ tests/test_pipeline_filters.py | 52 +++++++++++++++++++++++++++++ 6 files changed, 91 insertions(+) diff --git a/configs/phase3.yaml b/configs/phase3.yaml index 4b335c33..4089ad07 100644 --- a/configs/phase3.yaml +++ b/configs/phase3.yaml @@ -22,6 +22,7 @@ selection: top_n: 100 min_novelty: 0.05 max_safety_risk: 0.40 + max_disagreement: 0.30 notes: - "Phase 3 exploration config. Weights differ from pipeline.yaml to prioritize safety." diff --git a/configs/pipeline.yaml b/configs/pipeline.yaml index 82fd99a7..25e80bbb 100644 --- a/configs/pipeline.yaml +++ b/configs/pipeline.yaml @@ -15,6 +15,7 @@ selection: top_n: 100 min_novelty: 0.20 max_safety_risk: 0.70 + max_disagreement: 0.40 notes: - "These weights are transparent baseline heuristics, not validated biological predictors." diff --git a/src/openamp_foundry/data/loaders.py b/src/openamp_foundry/data/loaders.py index 2bb6f836..f8d67fa6 100644 --- a/src/openamp_foundry/data/loaders.py +++ b/src/openamp_foundry/data/loaders.py @@ -25,6 +25,13 @@ def load_candidates_csv(path: str | Path) -> list[PeptideCandidate]: reader = csv.DictReader(f) for i, row in enumerate(reader, start=1): cid = row.get("id") or f"candidate-{i:06d}" + if "sequence" not in row: + cols = list(row.keys()) + raise ValueError( + f"CSV row {i} is missing required 'sequence' column. " + f"Found columns: {cols!r}. " + "Check that the CSV header spells it exactly as 'sequence'." + ) seq = normalize_sequence(row["sequence"]) source = row.get("source", "csv") rows.append(PeptideCandidate(candidate_id=cid, sequence=seq, source=source)) diff --git a/src/openamp_foundry/pipeline.py b/src/openamp_foundry/pipeline.py index e231d22f..be1db91b 100644 --- a/src/openamp_foundry/pipeline.py +++ b/src/openamp_foundry/pipeline.py @@ -123,6 +123,7 @@ def run_ranking_pipeline( selection_cfg = config.get("selection", {}) min_novelty = float(selection_cfg.get("min_novelty", 0.0)) max_safety_risk = float(selection_cfg.get("max_safety_risk", 1.0)) + max_disagreement = float(selection_cfg.get("max_disagreement", 1.0)) ranked = rank_candidates(scored) top_n = int(selection_cfg.get("top_n", len(ranked))) @@ -132,6 +133,7 @@ def run_ranking_pipeline( if getattr(item, "valid", True) and item.scores["novelty"] >= min_novelty and item.scores["safety"] >= (1.0 - max_safety_risk) + and item.scores.get("disagreement", 0.0) <= max_disagreement ] selected = greedy_diverse_select(eligible, top_n=top_n) diff --git a/tests/test_data_loaders.py b/tests/test_data_loaders.py index f9dbc141..6979fbf4 100644 --- a/tests/test_data_loaders.py +++ b/tests/test_data_loaders.py @@ -141,3 +141,31 @@ def test_uses_demo_file_successfully(self): assert len(candidates) > 0 for c in candidates: assert len(c.sequence) > 0 + + def test_missing_sequence_column_raises_value_error(self): + # A CSV with no 'sequence' column should give a helpful error, not a bare KeyError. + # This guards against users passing in a wrongly-formatted file and getting + # a cryptic crash deep in the pipeline. + rows = [{"id": "C1", "seq": "KWKLF", "source": "test"}] # 'seq' not 'sequence' + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "bad.csv" + with path.open("w", newline="", encoding="utf-8") as f: + writer = csv.DictWriter(f, fieldnames=["id", "seq", "source"]) + writer.writeheader() + writer.writerows(rows) + import pytest as _pytest + with _pytest.raises(ValueError, match="sequence"): + load_candidates_csv(path) + + def test_missing_sequence_column_error_names_found_columns(self): + # The error message should list what columns were found to help the user. + rows = [{"peptide": "KWKLF"}] + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "bad.csv" + with path.open("w", newline="", encoding="utf-8") as f: + writer = csv.DictWriter(f, fieldnames=["peptide"]) + writer.writeheader() + writer.writerows(rows) + import pytest as _pytest + with _pytest.raises(ValueError, match="peptide"): + load_candidates_csv(path) diff --git a/tests/test_pipeline_filters.py b/tests/test_pipeline_filters.py index db037814..6e0e1d7c 100644 --- a/tests/test_pipeline_filters.py +++ b/tests/test_pipeline_filters.py @@ -164,6 +164,58 @@ def test_eligibility_novelty_at_exact_minimum_passes(tmp_path): assert rows[0]["selected"] is True +def test_disagreement_gate_blocks_high_uncertainty_candidate(tmp_path): + """Candidates where activity and Boman index strongly disagree should be blocked. + + max_disagreement in config caps the |activity - boman_activity| gap. + A perfectly safe/novel candidate is still rejected when scorer disagreement is too high. + Uses a sequence that naturally has high disagreement (high physchem activity but low + Boman potential) by setting max_disagreement=0.0 so any disagreement blocks selection. + """ + # max_disagreement=0.0 means ONLY candidates with zero scorer disagreement pass. + # In practice disagreement>0 for any real sequence, so nothing should be selected. + strict_config = tmp_path / "strict_disagree.yaml" + strict_config.write_text( + "pipeline_version: '0.1.0'\n" + "filters:\n min_length: 8\n max_length: 35\n" + " allowed_amino_acids: 'ACDEFGHIKLMNPQRSTVWY'\n" + "weights:\n activity: 0.40\n safety: 0.25\n synthesis: 0.15\n novelty: 0.20\n" + "selection:\n top_n: 100\n min_novelty: 0.0\n max_safety_risk: 1.0\n" + " max_disagreement: 0.0\n" + ) + csv_path = tmp_path / "candidates.csv" + refs_path = tmp_path / "refs.csv" + csv_path.write_text("id,sequence,source\nTEST-001,KWKLFKKIGAVLKVL,test\n") + refs_path.write_text("id,sequence,source\n") # empty refs → novelty=1.0 (passes novelty gate) + out = tmp_path / "ranked.jsonl" + run_ranking_pipeline( + candidate_path=csv_path, + reference_path=refs_path, + out_path=out, + config_path=strict_config, + ) + rows = [json.loads(line) for line in out.read_text().splitlines() if line.strip()] + assert len(rows) == 1 + # disagreement > 0 for any real sequence → blocked by max_disagreement=0.0 + assert rows[0]["selected"] is False + assert rows[0]["scores"]["disagreement"] > 0.0 + + +def test_disagreement_gate_config_values_documented(tmp_path): + """pipeline.yaml max_disagreement=0.40, phase3.yaml max_disagreement=0.30 (stricter).""" + repo_root = Path(__file__).parents[1] + pipeline_cfg = load_config(repo_root / "configs" / "pipeline.yaml") + phase3_cfg = load_config(repo_root / "configs" / "phase3.yaml") + + pipeline_max = float(pipeline_cfg["selection"]["max_disagreement"]) + phase3_max = float(phase3_cfg["selection"]["max_disagreement"]) + + # phase3 is stricter (lower tolerance for disagreement) + assert phase3_max < pipeline_max + assert abs(pipeline_max - 0.40) < 0.01 + assert abs(phase3_max - 0.30) < 0.01 + + def test_phase3_config_has_stricter_safety_filter_than_pipeline(): """phase3.yaml max_safety_risk=0.40 (safety>=0.60) vs pipeline.yaml 0.70 (safety>=0.30). From 172beb5dcfb9c2b0a712c49eb792d643bfe61deb Mon Sep 17 00:00:00 2001 From: j Date: Sun, 28 Jun 2026 09:16:52 +0700 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20code=20review=20=E2=80=94?= =?UTF-8?q?=20bracket=20access,=20header-first=20validation,=20positive=20?= =?UTF-8?q?control?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pipeline.py: scores.get('disagreement', 0.0) → scores['disagreement'] Consistent with bracket access used for novelty/safety; if the key ever goes missing the pipeline fails loudly instead of silently passing all candidates through the disagreement gate. loaders.py: validate CSV header before iterating rows Check reader.fieldnames before the row loop so header-only files (zero data rows) also raise ValueError. Per-row check was redundant once the header is validated — removed. Error message now says 'CSV header is missing' not 'CSV row 1 is missing' to avoid confusion. test_data_loaders.py: - import pytest at module level (was inside function — ruff PLC0415) - add test_missing_sequence_column_fires_on_header_only_csv for zero-row case - remove inline imports test_pipeline_filters.py: - add positive control to test_disagreement_gate_blocks_high_uncertainty_candidate: same candidate is selected when max_disagreement=1.0 (relaxed gate), proving the disagreement gate is the cause of blocking, not some other filter - fix ruff E741: rename l → ln in list comprehensions --- src/openamp_foundry/data/loaders.py | 14 ++++---- src/openamp_foundry/pipeline.py | 2 +- tests/test_data_loaders.py | 22 +++++++----- tests/test_pipeline_filters.py | 52 +++++++++++++++++------------ 4 files changed, 53 insertions(+), 37 deletions(-) diff --git a/src/openamp_foundry/data/loaders.py b/src/openamp_foundry/data/loaders.py index f8d67fa6..b8ec4273 100644 --- a/src/openamp_foundry/data/loaders.py +++ b/src/openamp_foundry/data/loaders.py @@ -23,15 +23,15 @@ def load_candidates_csv(path: str | Path) -> list[PeptideCandidate]: rows: list[PeptideCandidate] = [] with Path(path).open("r", encoding="utf-8") as f: reader = csv.DictReader(f) + fieldnames = reader.fieldnames or [] + if "sequence" not in fieldnames: + raise ValueError( + f"CSV header is missing required 'sequence' column. " + f"Found columns: {list(fieldnames)!r}. " + "Check that the CSV header spells it exactly as 'sequence'." + ) for i, row in enumerate(reader, start=1): cid = row.get("id") or f"candidate-{i:06d}" - if "sequence" not in row: - cols = list(row.keys()) - raise ValueError( - f"CSV row {i} is missing required 'sequence' column. " - f"Found columns: {cols!r}. " - "Check that the CSV header spells it exactly as 'sequence'." - ) seq = normalize_sequence(row["sequence"]) source = row.get("source", "csv") rows.append(PeptideCandidate(candidate_id=cid, sequence=seq, source=source)) diff --git a/src/openamp_foundry/pipeline.py b/src/openamp_foundry/pipeline.py index be1db91b..f85b3739 100644 --- a/src/openamp_foundry/pipeline.py +++ b/src/openamp_foundry/pipeline.py @@ -133,7 +133,7 @@ def run_ranking_pipeline( if getattr(item, "valid", True) and item.scores["novelty"] >= min_novelty and item.scores["safety"] >= (1.0 - max_safety_risk) - and item.scores.get("disagreement", 0.0) <= max_disagreement + and item.scores["disagreement"] <= max_disagreement ] selected = greedy_diverse_select(eligible, top_n=top_n) diff --git a/tests/test_data_loaders.py b/tests/test_data_loaders.py index 6979fbf4..dd808282 100644 --- a/tests/test_data_loaders.py +++ b/tests/test_data_loaders.py @@ -5,6 +5,8 @@ import tempfile from pathlib import Path +import pytest + from openamp_foundry.data.loaders import ( CANONICAL_AA, is_valid_sequence, @@ -146,26 +148,30 @@ def test_missing_sequence_column_raises_value_error(self): # A CSV with no 'sequence' column should give a helpful error, not a bare KeyError. # This guards against users passing in a wrongly-formatted file and getting # a cryptic crash deep in the pipeline. - rows = [{"id": "C1", "seq": "KWKLF", "source": "test"}] # 'seq' not 'sequence' with tempfile.TemporaryDirectory() as d: path = Path(d) / "bad.csv" with path.open("w", newline="", encoding="utf-8") as f: writer = csv.DictWriter(f, fieldnames=["id", "seq", "source"]) writer.writeheader() - writer.writerows(rows) - import pytest as _pytest - with _pytest.raises(ValueError, match="sequence"): + writer.writerow({"id": "C1", "seq": "KWKLF", "source": "test"}) + with pytest.raises(ValueError, match="sequence"): + load_candidates_csv(path) + + def test_missing_sequence_column_fires_on_header_only_csv(self): + # Header validation happens before iterating rows — zero-row files are caught too. + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "header_only.csv" + path.write_text("id,seq,source\n") # header only, no data rows + with pytest.raises(ValueError, match="sequence"): load_candidates_csv(path) def test_missing_sequence_column_error_names_found_columns(self): # The error message should list what columns were found to help the user. - rows = [{"peptide": "KWKLF"}] with tempfile.TemporaryDirectory() as d: path = Path(d) / "bad.csv" with path.open("w", newline="", encoding="utf-8") as f: writer = csv.DictWriter(f, fieldnames=["peptide"]) writer.writeheader() - writer.writerows(rows) - import pytest as _pytest - with _pytest.raises(ValueError, match="peptide"): + writer.writerow({"peptide": "KWKLF"}) + with pytest.raises(ValueError, match="peptide"): load_candidates_csv(path) diff --git a/tests/test_pipeline_filters.py b/tests/test_pipeline_filters.py index 6e0e1d7c..7ed53284 100644 --- a/tests/test_pipeline_filters.py +++ b/tests/test_pipeline_filters.py @@ -167,38 +167,48 @@ def test_eligibility_novelty_at_exact_minimum_passes(tmp_path): def test_disagreement_gate_blocks_high_uncertainty_candidate(tmp_path): """Candidates where activity and Boman index strongly disagree should be blocked. - max_disagreement in config caps the |activity - boman_activity| gap. - A perfectly safe/novel candidate is still rejected when scorer disagreement is too high. - Uses a sequence that naturally has high disagreement (high physchem activity but low - Boman potential) by setting max_disagreement=0.0 so any disagreement blocks selection. + max_disagreement caps the |activity - boman_activity| gap. The test proves the + gate is the cause of blocking by showing the SAME candidate is SELECTED when the + gate is relaxed to 1.0 (no restriction). """ - # max_disagreement=0.0 means ONLY candidates with zero scorer disagreement pass. - # In practice disagreement>0 for any real sequence, so nothing should be selected. - strict_config = tmp_path / "strict_disagree.yaml" - strict_config.write_text( + _COMMON_CONFIG = ( "pipeline_version: '0.1.0'\n" "filters:\n min_length: 8\n max_length: 35\n" " allowed_amino_acids: 'ACDEFGHIKLMNPQRSTVWY'\n" "weights:\n activity: 0.40\n safety: 0.25\n synthesis: 0.15\n novelty: 0.20\n" "selection:\n top_n: 100\n min_novelty: 0.0\n max_safety_risk: 1.0\n" - " max_disagreement: 0.0\n" ) + strict_config = tmp_path / "strict_disagree.yaml" + strict_config.write_text(_COMMON_CONFIG + " max_disagreement: 0.0\n") + permissive_config = tmp_path / "permissive_disagree.yaml" + permissive_config.write_text(_COMMON_CONFIG + " max_disagreement: 1.0\n") + + seq = "KWKLFKKIGAVLKVL" csv_path = tmp_path / "candidates.csv" refs_path = tmp_path / "refs.csv" - csv_path.write_text("id,sequence,source\nTEST-001,KWKLFKKIGAVLKVL,test\n") - refs_path.write_text("id,sequence,source\n") # empty refs → novelty=1.0 (passes novelty gate) - out = tmp_path / "ranked.jsonl" + csv_path.write_text(f"id,sequence,source\nTEST-001,{seq},test\n") + refs_path.write_text("id,sequence,source\n") # empty refs → novelty=1.0 + + # Strict gate: disagreement > 0 for any real sequence → blocked + out_strict = tmp_path / "strict.jsonl" run_ranking_pipeline( - candidate_path=csv_path, - reference_path=refs_path, - out_path=out, - config_path=strict_config, + candidate_path=csv_path, reference_path=refs_path, + out_path=out_strict, config_path=strict_config, ) - rows = [json.loads(line) for line in out.read_text().splitlines() if line.strip()] - assert len(rows) == 1 - # disagreement > 0 for any real sequence → blocked by max_disagreement=0.0 - assert rows[0]["selected"] is False - assert rows[0]["scores"]["disagreement"] > 0.0 + rows_strict = [json.loads(ln) for ln in out_strict.read_text().splitlines() if ln.strip()] + assert len(rows_strict) == 1 + assert rows_strict[0]["scores"]["disagreement"] > 0.0 + assert rows_strict[0]["selected"] is False # disagreement gate blocks it + + # Positive control: same candidate passes when gate is off (max_disagreement=1.0) + out_permissive = tmp_path / "permissive.jsonl" + run_ranking_pipeline( + candidate_path=csv_path, reference_path=refs_path, + out_path=out_permissive, config_path=permissive_config, + ) + rows_permissive = [json.loads(ln) for ln in out_permissive.read_text().splitlines() if ln.strip()] + assert len(rows_permissive) == 1 + assert rows_permissive[0]["selected"] is True # relaxed gate allows it through def test_disagreement_gate_config_values_documented(tmp_path):