From 73881516a1410a079a1467bc379e6a55d04e8621 Mon Sep 17 00:00:00 2001 From: helloiamvu Date: Thu, 4 Jun 2026 09:50:54 +0200 Subject: [PATCH] fix(drift): repair broken weekly Drift Watchdog (#6, #56) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit capture_drift.py called `mostlyright.research(...)` without importing `mostlyright`, so every weekly cron NameError'd on all 5 cases, wrote zero captures, and soft-exited 0. compare.py then reported every case as "drift capture missing", wrote drift-report.md, and the workflow opened a GH issue from it — a 100% false-positive "Drift detected" issue every Monday (#6, #56), never once a real comparison. - capture_drift.py: add the missing `import mostlyright`. - compare.py: split genuine value/dtype/shape drift (opens an issue) from missing captures (capture/network/infra failure -> stderr warning, never written to the report, never opens an issue). If every capture is missing, no report is written. main() now takes optional parity_dir/drift_dir for testability; the CLI path (`python compare.py`) is unchanged. - test_drift.py: regression tests covering missing-capture (no report), real value drift (report written), matching (no report), and that capture_drift imports mostlyright. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/fixtures/drift/capture_drift.py | 2 + tests/fixtures/drift/compare.py | 60 ++++++++++---- tests/test_drift.py | 108 ++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 16 deletions(-) diff --git a/tests/fixtures/drift/capture_drift.py b/tests/fixtures/drift/capture_drift.py index 211036c9..d010f061 100644 --- a/tests/fixtures/drift/capture_drift.py +++ b/tests/fixtures/drift/capture_drift.py @@ -43,6 +43,8 @@ import traceback from pathlib import Path +import mostlyright + #: Mirror of CASES in tests/test_parity.py and tests/fixtures/parity/capture_fixtures.py. #: Any change to the case list MUST land in all three locations in the same commit. CASES: list[tuple[int, str, str, str, str]] = [ diff --git a/tests/fixtures/drift/compare.py b/tests/fixtures/drift/compare.py index 84d7a256..8359a94a 100644 --- a/tests/fixtures/drift/compare.py +++ b/tests/fixtures/drift/compare.py @@ -14,11 +14,17 @@ ---------------- - If parity/ files don't exist → exit 1 (parity baseline is required). - If drift/ files don't exist → exit 0 (fresh clone; nothing to diff). -- If diff finds mismatches → write ``drift-report.md`` to this directory - and exit 0. The workflow that calls this script opens a GH issue from - the report; CI is **never** failed by a mismatch. -- If a drift/case is missing but parity/case exists → log as a capture - failure in the report; do not raise. +- If diff finds genuine value/dtype/shape mismatches → write + ``drift-report.md`` to this directory and exit 0. The workflow that calls + this script opens a GH issue from the report; CI is **never** failed by a + mismatch. +- If a drift/case is MISSING but parity/case exists → log a WARNING to + stderr and continue. A missing capture is a capture/network/infra failure + (e.g. ``capture_drift.py`` errored or an upstream API was down), NOT + ``research()`` drift, so it is **never** written to the report and **never** + opens an issue. If every capture is missing, no report is written at all. + This is the guard against the watchdog spamming false-positive issues + (see issues #6, #56). Usage ----- @@ -131,16 +137,25 @@ def _compare_frames(case_num: int, parity_df: pd.DataFrame, drift_df: pd.DataFra return findings -def main() -> int: +def main(parity_dir: Path | None = None, drift_dir: Path | None = None) -> int: here = Path(__file__).resolve().parent - parity_dir = here.parent / "parity" - drift_dir = here + parity_dir = parity_dir if parity_dir is not None else here.parent / "parity" + drift_dir = drift_dir if drift_dir is not None else here if not parity_dir.exists(): print(f"ERROR: parity baseline missing at {parity_dir}", file=sys.stderr) return 1 - all_findings: list[str] = [] + # Two deliberately separate buckets: + # drift_findings — genuine value/dtype/shape divergence vs parity. These + # open a GH issue (the watchdog's real job). + # missing_captures — a drift/case parquet capture_drift.py never produced + # (network outage, upstream API down, capture bug). That + # is infra failure, NOT research() drift, so it is logged + # as a warning and NEVER opens an issue (see #6, #56). + drift_findings: list[str] = [] + missing_captures: list[str] = [] + for case_num, station, frm, to in CASES: fname = f"case_{case_num}_{station}_{frm}_{to}.parquet" parity_path = parity_dir / fname @@ -150,21 +165,33 @@ def main() -> int: print(f"ERROR: parity fixture missing — {parity_path}", file=sys.stderr) return 1 if not drift_path.exists(): - # Fresh clone or failed capture — log to report, do not raise. - all_findings.append(f"case {case_num}: drift capture missing ({drift_path.name})") + msg = f"case {case_num}: drift capture missing ({drift_path.name})" + print( + f"WARNING: {msg} — capture/network failure, not research() drift", + file=sys.stderr, + ) + missing_captures.append(msg) continue parity_df = pd.read_parquet(parity_path) drift_df = pd.read_parquet(drift_path) case_findings = _compare_frames(case_num, parity_df, drift_df) - all_findings.extend(case_findings) + drift_findings.extend(case_findings) if not case_findings: print(f"case {case_num}: OK") else: for finding in case_findings: print(finding) - if all_findings: + if missing_captures: + # Surface in CI logs for visibility, but do NOT escalate to an issue. + print( + f"\n{len(missing_captures)} drift capture(s) missing — treated as " + "capture/network failure, NOT drift. No issue opened for these.", + file=sys.stderr, + ) + + if drift_findings: report = drift_dir / "drift-report.md" lines = [ "# Drift Report", @@ -181,16 +208,17 @@ def main() -> int: "## Findings", "", ] - for f in all_findings: + for f in drift_findings: lines.append(f"- {f}") lines.append("") report.write_text("\n".join(lines)) - print(f"\nWrote {report} with {len(all_findings)} finding(s).") + print(f"\nWrote {report} with {len(drift_findings)} finding(s).") # Soft-fail: do NOT non-zero-exit on drift. The workflow opens an # issue from the report; CI is never blocked by this script. return 0 - # No findings — clean week. + # No drift findings — clean week. (Missing captures, if any, were warned + # about above but never escalate to an issue.) print("OK: drift matches parity within tolerance.") return 0 diff --git a/tests/test_drift.py b/tests/test_drift.py index c8a0886d..c9381fdf 100644 --- a/tests/test_drift.py +++ b/tests/test_drift.py @@ -15,10 +15,12 @@ from __future__ import annotations +import importlib.util import subprocess import sys from pathlib import Path +import pandas as pd import pytest DRIFT_DIR = Path(__file__).resolve().parent / "fixtures" / "drift" @@ -64,3 +66,109 @@ def test_drift_scripts_present() -> None: """Both drift scripts must exist (capture + compare).""" assert (DRIFT_DIR / "capture_drift.py").exists() assert (DRIFT_DIR / "compare.py").exists() + + +# --------------------------------------------------------------------------- +# Watchdog false-positive regression suite (issues #6, #56). +# +# The weekly Drift Watchdog opened a "Drift detected" issue every Monday +# because (1) capture_drift.py referenced ``mostlyright.research(...)`` without +# importing ``mostlyright`` — so every run NameError'd, wrote zero captures, +# and (2) compare.py then escalated every "capture missing" entry into the +# report that opens the issue. A missing capture is a capture/network failure, +# not research() drift, and must never open an issue. +# --------------------------------------------------------------------------- + + +def _load_script(name: str): + """Import a drift script (capture_drift / compare) by file path.""" + path = DRIFT_DIR / f"{name}.py" + spec = importlib.util.spec_from_file_location(f"drift_{name}", path) + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +def _write_case(directory: Path, *, high: float = 40.0) -> None: + """Write a minimal parity/drift case parquet under the test case name.""" + df = pd.DataFrame( + { + "date": pd.to_datetime(["2025-01-06", "2025-01-07"]), + "station": ["TEST", "TEST"], + "obs_high_f": [high, 41.0], + } + ) + df.to_parquet(directory / "case_1_TEST_2025-01-06_2025-01-07.parquet") + + +def test_capture_drift_imports_mostlyright() -> None: + """capture_drift.py must import ``mostlyright`` (root cause of #6/#56). + + main() calls ``mostlyright.research(...)``; without the import every case + raises NameError, the soft-fail try/except swallows it, zero captures are + written, and the watchdog opens a false-positive issue. + """ + mod = _load_script("capture_drift") + assert hasattr(mod, "mostlyright"), ( + "capture_drift.py must `import mostlyright` — main() calls mostlyright.research(...)" + ) + + +def test_compare_missing_capture_opens_no_issue(tmp_path: Path, monkeypatch) -> None: + """Missing drift captures are infra/network failures — never open an issue. + + With the parity baseline present but no drift capture, compare.py must NOT + write drift-report.md (the file the workflow turns into a GH issue). + """ + mod = _load_script("compare") + monkeypatch.setattr(mod, "CASES", [(1, "TEST", "2025-01-06", "2025-01-07")]) + parity_dir = tmp_path / "parity" + parity_dir.mkdir() + drift_dir = tmp_path / "drift" + drift_dir.mkdir() + _write_case(parity_dir) + # Intentionally write NO drift capture — simulates a failed/skipped capture. + + rc = mod.main(parity_dir=parity_dir, drift_dir=drift_dir) + + assert rc == 0 # soft-fail policy preserved + assert not (drift_dir / "drift-report.md").exists(), ( + "missing capture must not open a drift issue" + ) + + +def test_compare_value_drift_opens_issue(tmp_path: Path, monkeypatch) -> None: + """A genuine value mismatch beyond tolerance DOES write the report.""" + mod = _load_script("compare") + monkeypatch.setattr(mod, "CASES", [(1, "TEST", "2025-01-06", "2025-01-07")]) + parity_dir = tmp_path / "parity" + parity_dir.mkdir() + drift_dir = tmp_path / "drift" + drift_dir.mkdir() + _write_case(parity_dir, high=40.0) + _write_case(drift_dir, high=99.0) # numeric drift far above ATOL=1e-12 + + rc = mod.main(parity_dir=parity_dir, drift_dir=drift_dir) + + assert rc == 0 + report = drift_dir / "drift-report.md" + assert report.exists(), "real value drift must open an issue" + assert "drift" in report.read_text().lower() + + +def test_compare_matching_opens_no_issue(tmp_path: Path, monkeypatch) -> None: + """Identical drift vs parity → clean week, no report written.""" + mod = _load_script("compare") + monkeypatch.setattr(mod, "CASES", [(1, "TEST", "2025-01-06", "2025-01-07")]) + parity_dir = tmp_path / "parity" + parity_dir.mkdir() + drift_dir = tmp_path / "drift" + drift_dir.mkdir() + _write_case(parity_dir, high=40.0) + _write_case(drift_dir, high=40.0) + + rc = mod.main(parity_dir=parity_dir, drift_dir=drift_dir) + + assert rc == 0 + assert not (drift_dir / "drift-report.md").exists()