Refactor hardening round 2: psutil, logging, parity harness, CI (+GWAS --glm fix) - #246
Merged
Merged
Conversation
psutil is imported by ancestry/model.py and legacy ancestry.py but was absent from install_requires, so a clean `pip install .` ImportErrors on `import genotools.cli`. Declare it (>=5.9.0).
Only a *middle* raising step under --warn was covered. Add coverage for a
*terminal* (last) step raising: assert the pipeline still produces final
{out}.pgen/.pvar/.psam by promoting the last passed step's output
(_handle_final_step_failure), verified via traceable pfile contents, and that
without --warn a terminal failure still fails fast.
setup_logging() was never called at runtime, so the root logger's WARNING
default dropped every step's logger.info/error and {out}_all_logs.log came out
header-only (a regression vs legacy concat_logs).
- cli/runner.py::_setup_logging now calls core.logging.setup_logging with a
file handler on {out}_all_logs.log, after upfront_check (which errors if that
log already exists). Append mode keeps the ASCII banner on top; structured,
step-tagged ([callrate_prune], ...) records follow.
- core/logging.py::setup_logging closes handlers before clearing to avoid FD
leaks across repeated in-process runs.
- Chose the structured-file-handler approach over resurrecting raw PLINK-.log
aggregation (documented in REFACTOR_HARDENING.md); raw PLINK output remains
available per-step in FilterResult.log/GWASResult.log.
Test: tests/regression/test_logging.py asserts the run log is non-empty and
carries step markers.
run_gwas passed config.glm_options ('hide-covar firth-fallback no-x-sex
cols=...') as a SINGLE argv element. The legacy code built the command as an
f-string that shell_do split on whitespace; run_command passes list elements
verbatim, so PLINK2 received the whole modifier string as one token and rejected
it ('Invalid --glm argument ...'), producing no GWAS output on every run --
silently swallowed under --warn. Split glm_options into tokens and keep
allow-no-covars inside the --glm group.
Regression test: tests/unit/test_gwas/test_steps_regression.py now runs a real
PLINK2 --glm on the synthetic data and asserts output is produced.
…e, GWAS - Per-step old/new flag map so both command lines build (old --case_control vs new --case-control; single-word haplotype/ld/pca/gwas identical). - Multi-word QC steps (case_control, haplotype, ld) + --all_sample/--all_variant full pipeline: parity-equal on synthetic data (IDs + genotype content). - GWAS (--pca --gwas): new compare_gwas_results/compare_gwas/find_gwas_output/ _lambda_gc comparators in compare.py, unit-tested in test_compare_gwas.py. Asserts GWAS runs, identical tested-variant set, and lambda within tolerance. - setup_stable_venv.sh honors a PYTHON env var to pin the interpreter for old deps (umap-learn==0.5.3). Flagged: new PCA uses --exclude range (drops MHC/high-LD) where old used --exclude (a no-op), so GWAS per-variant p-values diverge from the pre-refactor baseline (lambda still agrees). Documented in REFACTOR_HARDENING.md for a deliberate maintainer decision; per-variant p-equality is intentionally not asserted.
- test job: Python 3.11, installs genotools + pytest, auto-downloads PLINK/PLINK2 via the dependency resolver, runs pytest tests/unit tests/regression (parity tests skip cleanly without .venv-stable). - parity job: full-history checkout, builds .venv-stable from origin/main via setup_stable_venv.sh (PYTHON=python -> 3.11 for umap-learn==0.5.3), runs tests/regression/test_parity.py.
GWAS's _create_phenotype_file writes {input}.pheno next to the input pfile
(pre-existing behavior, preserved for parity). The new GWAS unit test and GWAS
parity test ran against tests/data/synthetic/genotools_test, leaving a stray
genotools_test.pheno in the tracked data dir. Copy the pfileset into tmp_path
first so the side-effect lands in the temp dir instead.
Maintainer decision: the new PCA --exclude range behavior (dropping MHC/high-LD regions before PCA) is a deliberate correctness fix, not a regression; the old --exclude was a no-op. GWAS per-variant p-values differ from the pre-refactor baseline by design. - Add TestPcaExcludesHighLdRegions guard test: PCA pruning removes every variant inside the exclusion ranges (non-vacuous; would fail if reverted to the old no-op). - Reframe REFACTOR_HARDENING.md from 'flagged/decision needed' to 'resolved (B)'. - Update the GWAS parity test docstring: per-variant p divergence is ratified and intentional; parity asserted at tested-variant-set + lambda level.
The pre-refactor genotools calls check_king() at module import (top-level in the old qc.py), so on Linux merely starting the old CLI downloads KING from the flaky kingrelatedness.com -- even for QC/GWAS runs that never use KING. That hung the first parity CI run for 17 min of retries. - setup_stable_venv.sh pre-caches KING (Linux only, robust curl with timeout + retries; warns and continues if the host is down) so the old CLI finds it and skips the download. macOS: check_king() returns None, no-op. - ci.yml: cache ~/.genotools (PLINK/PLINK2/KING) in both jobs so external hosts are hit at most once per cache key; add workflow_dispatch for manual runs.
- TESTING.md: how regression/parity testing works -- env setup, golden vs old-vs-new parity, building .venv-stable (Python 3.11), the KING-on-Linux gotcha, decision-B GWAS context, running parity on real cohorts, and CI. - tests/scripts/run_parity.py: turnkey old-vs-new parity for a real cohort. Copies the cohort into a workdir (so source data is never written to), runs both CLIs across QC / full-pipeline / GWAS scenarios, prints a PASS/FAIL summary, and exits non-zero on divergence. Reuses tests/regression/compare.py. - REFACTOR_HARDENING.md: record the CI flake fix and handoff tooling.
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
1 participant
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.
Summary
Round 2 of refactor hardening (into
refactor/main). Five tracked items plus one bug the harness uncovered, each in its own commit. Full suite: 391 passed (the 6 parity tests run when.venv-stableis present, else skip).psutilinsetup.pyinstall_requires— imported byancestry/model.pyand legacyancestry.py, so a cleanpip install .previously ImportErrored onimport genotools.cli.--warnterminal-step-failure coverage — added a test where the last QC step raises under--warn, asserting the last-passed output is promoted to{out}.pgen/.pvar/.psam(_handle_final_step_failure), verified via traceable pfile contents; plus fail-fast without--warn. (Only a middle raising step was covered before.)cli/runner.py::_setup_loggingnow callscore.logging.setup_logging(log_file={out}_all_logs.log)afterupfront_check, in append mode so the banner stays on top and step-tagged records follow. Previouslysetup_loggingwas never called at runtime, so the root logger's WARNING default dropped every step'slogger.info/errorand the consolidated log came out header-only.setup_loggingnow closes handlers before clearing to avoid FD leaks across in-process runs.case_control,haplotype,ld) via a per-step old/new flag map, the--all_sample --all_variantfull pipeline, and GWAS (--pca --gwas) with a new.glm/lambda comparator incompare.py(unit-tested). All parity-equal on synthetic data (see PCA note below for GWAS)..github/workflows/ci.yml: a unit+regression job (auto-downloads PLINK/PLINK2) and a parity job that builds.venv-stableand runstest_parity.py.Bug fixed along the way
--glmwas passed as a single argv token (gwas/steps/association.py). The legacy code built an f-string thatshell_dosplit on whitespace;run_commandpasses list elements verbatim, so PLINK2 received"hide-covar firth-fallback no-x-sex cols=..."as one invalid 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. This is why the parity harness's GWAS coverage was the highest-value gap.✅ Resolved: PCA region exclusion (decision B)
The GWAS parity run surfaced a real old-vs-new scientific difference. New PCA pruning uses
--exclude range <file>(correctly drops MHC/high-LD ranges: 9741 → 9644 variants); old used--exclude <file>with norange, which PLINK2 treats as a variant-ID list, matching nothing — the old region exclusion was a no-op bug. So the eigenvectors differ and every GWAS p-value shifts slightly (lambda still agrees: 1.0074 vs 1.0071; tested-variant set identical).Maintainer decision: (B) ratified
--exclude rangeas an intentional correctness fix — excluding MHC/high-LD regions before PCA is standard practice. This is a deliberate, accepted divergence from the pre-refactor GWAS baseline, not a regression. The GWAS parity test asserts the invariants that hold under B (GWAS runs, same tested-variant set, lambda within tolerance) and does not assert per-variant p-equality; a guard test (TestPcaExcludesHighLdRegions) locks the exclusion in so it can't silently revert to the old no-op. Details inREFACTOR_HARDENING.md.Test plan
pytest tests/unit tests/regression— passes with.venv-stablepresent (parity runs) and without it (parity skips).venv-stable(built fromorigin/main, Python 3.11): QC multi-word steps, full pipeline, GWAS{out}_all_logs.logverified non-empty with[callrate_prune]/[geno_prune]step markerspsutilpresent in package metadata;import genotools.clisucceeds