Refactor hardening round 2: psutil, logging, parity harness, CI (+GWAS --glm fix)

.github/workflows/ci.yml

@@ -0,0 +1,88 @@
+name: CI
+
+on:
+  push:
+    branches: [main, refactor/main]
+  pull_request:
+  workflow_dispatch:  # allow manual runs from the Actions tab (any branch)
+
+concurrency:
+  group: ci-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  test:
+    name: Unit + regression tests
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      - uses: actions/setup-python@v5
+        with:
+          python-version: "3.11"
+          cache: pip
+
+      - name: Cache external tools (PLINK/PLINK2/KING)
+        uses: actions/cache@v4
+        with:
+          path: ~/.genotools
+          key: genotools-deps-${{ runner.os }}-${{ hashFiles('genotools/dependencies.py') }}
+
+      - name: Install genotools + test deps
+        run: |
+          python -m pip install --upgrade pip
+          pip install -e .
+          pip install pytest
+
+      - name: Download PLINK / PLINK2
+        # The genotools dependency resolver fetches the Linux binaries into
+        # ~/.genotools/misc/executables so the QC/GWAS steps (and the parity
+        # genotype comparator) can find them.
+        run: |
+          python -c "from genotools.dependencies import check_plink, check_plink2; print(check_plink()); print(check_plink2())"
+
+      - name: Run unit + regression tests
+        # Parity tests under tests/regression skip cleanly here (no .venv-stable);
+        # they run in the dedicated parity job below.
+        run: pytest tests/unit tests/regression -q
+
+  parity:
+    name: Old-vs-new parity
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          # Full history so the pre-refactor baseline ref (origin/main) is
+          # available to build .venv-stable from.
+          fetch-depth: 0
+
+      - uses: actions/setup-python@v5
+        with:
+          python-version: "3.11"
+          cache: pip
+
+      - name: Cache external tools (PLINK/PLINK2/KING)
+        uses: actions/cache@v4
+        with:
+          path: ~/.genotools
+          key: genotools-deps-${{ runner.os }}-${{ hashFiles('genotools/dependencies.py') }}
+
+      - name: Install genotools (new) + test deps
+        run: |
+          python -m pip install --upgrade pip
+          pip install -e .
+          pip install pytest
+
+      - name: Download PLINK / PLINK2
+        run: |
+          python -c "from genotools.dependencies import check_plink, check_plink2; print(check_plink()); print(check_plink2())"
+
+      - name: Fetch pre-refactor baseline ref
+        run: git fetch origin main:refs/remotes/origin/main
+
+      - name: Build .venv-stable (pre-refactor baseline)
+        # umap-learn==0.5.3 (old dep) needs Python <=3.11, matched by this job.
+        run: PYTHON=python bash tests/scripts/setup_stable_venv.sh origin/main
+
+      - name: Run old-vs-new parity tests
+        run: pytest tests/regression/test_parity.py -v

REFACTOR_HARDENING.md

@@ -24,13 +24,13 @@ Tier list is the "easy wins" audit against `origin/refactor/main`.
 | 1b — geno honors threshold + real pass/fail | ✅ Done | `qc/steps/variant_missingness.py:79`; failure propagates as exception. |
 | 1c — haplotype reports failure | ✅ Fixed here | Step raises correctly; **CLI now catches it under `--warn`** (see below). |
 | 1d — GWAS case/control guard | ✅ Done | `association.py:175-177` uses `.get(k, 0)`. |
-| 1d — GWAS log capture | ❌ Open | No log aggregation in the new path; `_all_logs.log` is header-only. |
+| 1d — GWAS log capture | ✅ Addressed here | Structured logging now routed to `{out}_all_logs.log` (see below); it is no longer header-only. Raw PLINK `.log` aggregation was intentionally **not** resurrected — the FilterResult/GWASResult `log` fields still carry PLINK stderr if per-step raw capture is ever wanted. |
 | 1e — stop destroying input files | ✅ Done | Guarded deletes; `shutil.copy2` not `os.rename`. |
 | 2 — parallelize per-ancestry groups | ❌ Open | Still a serial loop (`cli/runner.py:337-354`). Pure-function design makes it cheap to add. |
 | 3 — cache ancestry PCA | 🟡 Partial | `--model` inference reuses fitted PCA; no `(ref_panel, common_snps)` auto-cache; `get_raw_files` still re-extracts. |
-| logging (structured) | 🟡 Built, unplugged | `core/logging.py` good but `setup_logging()` never called at runtime → `logger.info` dropped. |
+| logging (structured) | ✅ Wired here | `_setup_logging()` now calls `core.logging.setup_logging(log_file={out}_all_logs.log)` after `upfront_check`; step `logger.info`/`error` (with `[step]` markers) land in the consolidated log. Test: `tests/regression/test_logging.py`. |
 | tests | ✅ / parity unverified | 343→346 unit tests pass; golden = new-vs-new (self-consistency), not old-vs-new. |
-| CI | ❌ Missing | No `.github/workflows`. |
+| CI | ✅ Added here | `.github/workflows/ci.yml`: (1) unit+regression on Python 3.11 with PLINK/PLINK2 auto-downloaded; (2) parity job builds `.venv-stable` and runs `test_parity.py`. |
 
 ---
 
@@ -77,6 +77,101 @@ bash tests/scripts/setup_stable_venv.sh origin/main   # install old baseline
 pytest tests/regression/test_parity.py -v             # old vs new
 ```
 
+### Round 2 (`refactor/hardening-round-2`)
+
+1. **`psutil` declared** in `setup.py::install_requires` (imported by
+   `ancestry/model.py` + legacy `ancestry.py`; a clean `pip install .` used to
+   ImportError). Verified via package metadata.
+2. **`--warn` terminal-step-failure test** — `tests/unit/test_cli/test_runner_regression.py`
+   now covers the *last* step raising under `--warn`, asserting the last-passed
+   output is promoted to `{out}.pgen/.pvar/.psam` (`_handle_final_step_failure`),
+   plus fail-fast without `--warn`.
+3. **Structured logging wired** — `cli/runner.py::_setup_logging()` now calls
+   `core.logging.setup_logging(level="INFO", log_file={out}_all_logs.log,
+   console=False)` *after* `upfront_check` (which errors if that log already
+   exists). It opens the file in append mode, so the ASCII banner stays on top
+   and structured, step-tagged (`[callrate_prune]`, …) records follow.
+   `setup_logging` now also closes handlers before clearing to avoid FD leaks
+   across repeated in-process runs. Test: `tests/regression/test_logging.py`.
+
+   **Consolidated-log design decision:** chose the *structured-file-handler*
+   approach over resurrecting legacy `concat_logs` raw-PLINK-`.log` aggregation.
+   Rationale: single mechanism, forward-looking (step-context aware), and it
+   reuses the already-built `core/logging.py`. Trade-off: `{out}_cleaned_logs.log`
+   remains an empty placeholder (legacy filename kept for compatibility); the
+   populated consolidated log is `{out}_all_logs.log`. Raw PLINK output is still
+   available per-step in `FilterResult.log`/`GWASResult.log` if finer capture is
+   wanted later.
+
+4. **GWAS `--glm` tokenization bug fixed** — `gwas/steps/association.py`.
+   `run_gwas` passed `config.glm_options` (`"hide-covar firth-fallback no-x-sex
+   cols=..."`) as a *single* argv token. The legacy code built an f-string that
+   `shell_do` split on whitespace; `run_command` passes list elements verbatim,
+   so PLINK2 got one giant invalid `--glm` argument and produced **no GWAS
+   output on any run** — silently swallowed under `--warn`. (The prior PR fixed
+   the summary crash, but GWAS never actually ran.) Now splits the modifiers
+   into tokens and keeps `allow-no-covars` in the `--glm` group. Test:
+   `tests/unit/test_gwas/test_steps_regression.py` runs a real PLINK2 `--glm`.
+
+5. **Parity harness extended** — `tests/regression/test_parity.py` +
+   `tests/regression/compare.py`:
+   - per-step old/new flag map (old `--case_control`/`--full_output`, new
+     `--case-control`/`--full-output`; single-word `haplotype`/`ld`/`pca`/`gwas`
+     identical);
+   - multi-word QC steps `case_control`, `haplotype`, `ld` (parametrized),
+     `--all_sample --all_variant` full pipeline — all **parity-equal** on
+     synthetic data (IDs + genotype content);
+   - GWAS (`--pca --gwas`): new comparator `compare_gwas_results()`/`compare_gwas()`
+     (+ `find_gwas_output`, `_lambda_gc`) in `compare.py`, unit-tested by
+     `tests/regression/test_compare_gwas.py`.
+   All parity tests pass with `.venv-stable` present and skip cleanly without it.
+
+6. **CI added** — `.github/workflows/ci.yml`: a unit+regression job (Python 3.11,
+   PLINK/PLINK2 auto-downloaded via the dependency resolver) and a parity job
+   that builds `.venv-stable` from `origin/main` and runs `test_parity.py`.
+   Both jobs cache `~/.genotools`; the workflow also supports manual dispatch.
+   **The first CI run flaked, then went green:** the pre-refactor code calls
+   `check_king()` at *import*, so on Linux the old CLI downloaded KING from the
+   flaky `kingrelatedness.com` on every start (a 17-min hang across the parity
+   tests). Fixed by pre-caching KING in `setup_stable_venv.sh` (Linux only) plus
+   `~/.genotools` caching. Details: [TESTING.md §7](TESTING.md#7-king-on-linux).
+
+7. **Handoff tooling & docs** — `TESTING.md` (regression/parity guide for the dev
+   running real-cohort testing) and `tests/scripts/run_parity.py` (turnkey
+   old-vs-new parity on a real cohort: copies the cohort to a workdir, runs both
+   CLIs across QC / full-pipeline / GWAS scenarios, prints a PASS/FAIL report and
+   exits non-zero on divergence).
+
+### ✅ Resolved (decision B): PCA region exclusion is an intentional fix
+
+The GWAS parity run surfaced a **real old-vs-new scientific difference** in PCA
+pruning (`gwas/steps/pca.py` vs legacy `gwas.py`):
+
+- **New** uses `--exclude range {file}` — correctly drops the high-LD/MHC ranges
+  in the exclusion file (synthetic data: 9741 → 9644 variants; prune.in 3971).
+- **Old** used `--exclude {file}` (no `range`) — PLINK2 treats a ranges file as a
+  variant-ID list, matches nothing, and excludes **zero** of those regions
+  (9741 variants; prune.in 3987). i.e. the old region exclusion was a no-op bug.
+
+Consequence: the PCA eigenvectors differ (eigenvalues differ ~0.01, values
+beyond sign flips), so the GWAS covariates — and hence **every** GWAS p-value —
+differ slightly between old and new. The genomic-inflation **lambda still agrees**
+(synthetic: 1.0074 vs 1.0071) and the **set of tested variants is identical**.
+
+**Decision (maintainer, option B): ratified `--exclude range` as an intentional
+correctness fix** — excluding MHC/high-LD regions before PCA is standard practice,
+and the old behavior was a no-op bug. This is a **deliberate, accepted divergence
+from the pre-refactor GWAS baseline**, not a regression: real-cohort GWAS
+per-variant p-values will differ slightly from the old code, and that is expected.
+
+Implications for the harness:
+- The GWAS parity test asserts the invariants that hold under B (GWAS runs, same
+  tested-variant set, lambda within tolerance) and intentionally does **not**
+  assert per-variant p-equality against the old baseline.
+- A guard test locks the fix in so it can't silently revert to the old no-op:
+  `tests/unit/test_gwas/test_steps_regression.py::TestPcaExcludesHighLdRegions`
+  asserts PCA pruning removes every variant inside the exclusion ranges.
+
 ---
 
 ## Remaining work (tracked, not yet done)
@@ -85,15 +180,18 @@ Priority order for making the refactor mergeable to `main`:
 
 1. **Prove parity on real data** — run `test_parity.py` on representative real
    cohorts (not just synthetic), across the full QC set and, ideally, ancestry +
-   GWAS. This is the gate before merging to `main`. (Harness is ready.)
-2. **Wire structured logging** — call `core.logging.setup_logging()` from the CLI
-   so step `logger.info`/error messages aren't dropped; decide on one log path.
-3. **Restore GWAS/consolidated log capture** (1d) — new path aggregates no PLINK
-   logs; `_all_logs.log` is header-only. Regression vs legacy `concat_logs`.
-4. **Add CI** — run unit + regression suites on push; add a parity job that builds
-   `.venv-stable` and runs `test_parity.py`.
-5. **Declare `psutil`** in `setup.py` `install_requires` (imported by
-   `ancestry/model.py`; a clean `pip install .` currently ImportErrors).
+   GWAS. This is the gate before merging to `main`. (Harness extended in round 2:
+   multi-word QC steps, `--all_sample --all_variant`, and GWAS+lambda. Per
+   decision B above, real-cohort GWAS per-variant p-values will differ slightly
+   from the old baseline *by design* — PCA now excludes MHC/high-LD regions — so
+   GWAS parity is asserted at the tested-variant-set + lambda level, not
+   per-variant. QC and full-pipeline parity remain exact.)
+2. ✅ **Wire structured logging** — DONE in round 2 (see above).
+3. ✅ **Restore GWAS/consolidated log capture** (1d) — DONE in round 2 via the
+   structured-file-handler approach (see above).
+4. ✅ **Add CI** — DONE in round 2 (`.github/workflows/ci.yml`): unit+regression
+   job (auto-downloads PLINK/PLINK2) + parity job (builds `.venv-stable`).
+5. ✅ **Declare `psutil`** — DONE in round 2.
 6. **Decouple from legacy** — new CLI still imports `utils.py`
    (`gt_header`/`bfiles_to_pfiles`/`vcf_to_pfiles`/`upfront_check`) and loads
    `ancestry.py` via an importlib file-path hack. Blocks legacy removal (Phase 5/6).