refactor: declare real types in pipeline.py and pipeline_config.py - #123
Merged
Conversation
Clears 17 pyrefly baseline entries (152 -> 135). Five root causes, all the
same shape: a declared type looser than what the code actually holds.
`_run_batch(plink_data: object | None)` — `object` has no attributes, so all
seven reads of `.genotypes`, `.n_snps`, `.chromosome`, `.sid`, `.bp_position`,
`.allele_1` and `.allele_2` error. The value is always a `PlinkData`, which
`jamma.io.plink` already exports and `pipeline.py` already imports from.
`_PhenoLoopOutcome.runner_timing: dict[str, float]` — the value comes from
`get_last_run_timing()`, typed `RunnerTiming`. Both readers use `.get()` with
keys the TypedDict declares, so adopting the real type costs nothing.
`run_result = None` before the per-phenotype loop, then `run_result.pve` after
it. Replaced with `pve`/`pve_se` locals captured each iteration, which is what
the code meant: the last phenotype's estimate. On an empty column list the old
code raised `AttributeError`; it now yields the `None` the field already
declares.
`JAMMA_BACKEND` reached `select_execution_mode` as an unvalidated `str`.
Validated at the boundary instead via `_parse_backend_override`, so a typo
fails before the pipeline reads PLINK metadata off disk rather than after.
The literal triple now lives once, as `BackendRequest` / `VALID_BACKENDS` in
`pipeline_config`, replacing two of the three copies (`select_execution_mode`
keeps its own).
`PipelineConfig.phenotype_columns: list[int] | None`, set by `__post_init__`
and never None afterwards, so all four readers had to handle a None that
cannot occur. Declared `list[int]` with an empty-list default: empty is now
the "unspecified" sentinel, and `phenotype_columns=[]` derives from
`phenotype_column` instead of raising IndexError. Passing None explicitly
still works at runtime. This does not touch the phenotype_column /
phenotype_columns duality itself, which stays outstanding.
Also `PipelineResult.timing = field(default_factory=dict)`, which infers
`dict[Unknown, Unknown]` rather than `PipelineTiming`; the TypedDict is
callable and returns the same `{}`.
Verified against real runs, not just the type checker: single- and
multi-phenotype pipelines on mouse_hs1940 across both backends produce
byte-identical .assoc.txt output and identical pve/pve_se/timing before and
after. `pytest tests/` — 2241 passed, 10 skipped, 3 xfailed.
One pipeline.py entry remains (line 765): `compute_loco_kinship_streaming`
returns a union driven by its `return_snp_stats` flag. That needs @overload
on the callee and is a separate change.
Baseline regenerated on Python 3.11 / numpy 2.4.6 per #121; `pyrefly check
--baseline` reports 0 errors on both 3.11.13/2.4.6 and 3.13.5/2.5.1.
This was referenced Jul 26, 2026
michael-denyer
added a commit
that referenced
this pull request
Jul 27, 2026
…#129) Clears 7 pyrefly baseline entries across five src modules (77 -> 70). Each is a declaration that did not describe what the code actually passes or holds. `core/progress.progress_iterator(iterable: Iterator)` only ever does `for i, item in enumerate(iterable)`, so `Iterable` is what it requires; `Iterator` rejected the `range` that `io/plink.py` passes it. Widening a parameter cannot break a caller. `io/plink.py` also fed `total=` an `int | ndarray`, because `bed.sid_count` carries that type through `n_chunks`. It is an int at runtime, so it says so. `lmm/likelihood._frozen(data: list[int])` is called with tuples on two of its five call sites. It passes `data` straight to `np.array`, which takes any sequence, so the parameter is `Sequence[int]`. `lmm/loco.py` annotated `snps_global_mask: np.ndarray | None` on the assignment *inside* the branch that builds it, which made the declared type a union at the point of the very next item assignment. Declared before the branch instead, with the array built through a local. `lmm/special.chi2_sf_batch` called `.astype` on the result of a `np.frompyfunc` ufunc, declared as returning a scalar. `np.asarray(..., dtype=)` is the same cast and types correctly. `gwas.GWASResult.timing = field(default_factory=dict)` infers `dict[Unknown, Unknown]` rather than `GWASTiming`; the TypedDict is callable and returns the same `{}`. Same fix as `PipelineResult.timing` in #123. No behaviour change, and checked rather than argued, because `special.py` feeds p-value computation and `loco.py` gates SNP selection: - `chi2_sf_batch` is bit-identical across 13 edge cases (NaN, negative, zero, 1e-300, 1e-12, 700, 1e6, inf) against the previous expression evaluated side by side. - All four LMM modes on both backends produce byte-identical .assoc.txt. - The LOCO path is identical: same .assoc.txt, pve/pve_se, per-chromosome kinship sums and stats cache. `pytest tests/` -- 2243 passed, 10 skipped, 3 xfailed. Baseline regenerated on Python 3.11 / numpy 2.4.6 per #121; 0 errors on both 3.11.13/2.4.6 and 3.13.5/2.5.1.
michael-denyer
added a commit
that referenced
this pull request
Jul 27, 2026
Takes src/ to zero (70 -> 63; everything left is tests and scripts). Three root causes. `RunnerTiming` is a TypedDict, so `last_run_timing.clear()` and `dict(last_run_timing)` are both rejected -- `clear` would violate required keys in general, and `dict()` of a TypedDict degrades to `dict[str, object]`. The snapshot now uses `.copy()`, which keeps the TypedDict type, and the two reset sites rebind the module global instead of mutating it. Rebinding also means a concurrent reader sees either the previous run's complete timings or the new ones, never a half-populated dict -- the module comment already warned this is not thread-safe, and this narrows the window rather than widening it. `run_lmm_association_numpy_streaming` assigned a `_LazySnpMeta` to its own `snp_info: list | None` parameter, then carried an `assert snp_info is not None` to make the following lines work. The resolved value now lives in its own local typed `_LazySnpMeta | list` -- which is exactly what both sinks declare -- and the assert is gone. That matters beyond typing: an assert vanishes under `python -O`, which is the reasoning already written down for `_require` in compute_numpy.py. `backend` arrived at `PipelineConfig` as a bare `str` from `cli._run_lmm` and `gwas.run_gwas`, though the CLI already constrains it with `click.Choice` and `PipelineConfig.backend` has been `BackendRequest` since #123. Both signatures now say so, and `BackendRequest` joins `jamma.pipeline.__all__` since it is now part of `run_gwas`'s public signature. Runtime is unaffected: `click.Choice` rejects bad values before the call, and `PipelineConfig.__post_init__` still validates for untyped callers. `cli.py`'s handler dispatch tested `if gk is not None: ... else:`, relying on mutual-exclusion checks about seventy lines earlier to know `lmm` was set. Each arm now tests its own flag, so the mode argument is narrowed at the call, with a final `else` restating the same UsageError for the case the earlier checks already reject. Verified beyond the type checker: - `get_last_run_timing()` returns `{}` before any run, the three expected float keys after one, an independent snapshot (mutating it does not affect the module), and reads the live global -- so the rebinding is visible. - Both CLI arms run end to end (`-gk 1` writes the kinship, `-lmm 1` analyses 10,768 SNPs), and the three error paths still report correctly: neither flag, both flags, and an invalid `--backend`. - All four LMM modes on both backends produce byte-identical .assoc.txt, and the LOCO path is unchanged. `pytest tests/` -- 2243 passed, 10 skipped, 3 xfailed. Baseline regenerated on Python 3.11 / numpy 2.4.6 per #121; 0 errors on both 3.11.13/2.4.6 and 3.13.5/2.5.1.
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
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.
Continues the pyrefly burn-down. 152 → 135 entries.
Five root causes, all the same shape: a declared type looser than what the code actually
holds, so every read of the value has to answer for a case that cannot happen.
_run_batch(plink_data: object | None)PlinkData | NonePipelineConfig.phenotype_columns: list[int] | Nonelist[int], empty-list defaultJAMMA_BACKENDas unvalidatedstrrun_result = Nonebefore the phenotype looppve/pve_seper iteration_PhenoLoopOutcome.runner_timing: dict[str, float]RunnerTimingPipelineResult.timing = field(default_factory=dict)default_factory=PipelineTimingplink_data: object | None— 7 errorsobjecthas no attributes, so all seven reads (.genotypes,.n_snps,.chromosome,.sid,.bp_position,.allele_1,.allele_2) error. The value is always aPlinkData,which
jamma.io.plinkexports andpipeline.pyalready imports from — no new dependency,no cycle.
phenotype_columns— 4 errorslist[int] | None, assigned in__post_init__and never None afterwards, so all fourreaders had to handle an impossible None. Declared
list[int]with an empty-list default:empty is now the "unspecified" sentinel. Side effect —
phenotype_columns=[]derives fromphenotype_columnrather than raisingIndexErroronself.phenotype_columns[0], whichis the behaviour the field default now implies. Passing
Noneexplicitly still works atruntime (verified below).
This does not touch the
phenotype_column/phenotype_columnsduality itself — themutation-based sync in
__post_init__and the range check living inPipelineRunner.validate_inputsare both still outstanding.JAMMA_BACKEND— 2 errorsos.environ.get("JAMMA_BACKEND")isstr | None, sorequestedwidened tostrand nolonger matched
select_execution_mode'sLiteral. Rather than cast, it's validated whereit enters the process.
select_execution_modedid already reject unknown values, so thisis not a new guard — but it now fires before the pipeline reads PLINK metadata off
disk, and with a message that names the env var.
The
("auto", "numpy", "numpy-streaming")triple had three copies. Two now shareBackendRequest/VALID_BACKENDSinpipeline_config;select_execution_modekeeps itsown, since collapsing that one means touching
lmm/runner.py.run_result = None— 2 errorsInitialised to
Nonebefore the per-phenotype loop, then read asrun_result.pveafterit. Replaced with
pve/pve_selocals captured each iteration — which is what the codemeant, the last phenotype's estimate. On an empty column list the old code raised
AttributeError; it now yields theNonethe field already declares.Verification
The type checker says nothing about behaviour, so the loop restructure was checked against
real runs. Single- and multi-phenotype (
-n 1and-n 1,6) on mouse_hs1940, both thebatch and streaming backends, before and after:
.assoc.txtoutput, all 6 filescmp)pve_estimate,pve_se,n_snps_tested,n_covariates,timingkeyspytest tests/ruff check/ruff format --checkConfig derivation checked directly:
PipelineResult().timingis still{}at runtime.Baseline
Regenerated on Python 3.11.13 / numpy 2.4.6 per #121, verified on both interpreters:
pyrefly check --baselineWhat's left in this file
One entry,
pipeline.py:765:compute_loco_kinship_streamingreturnsIterator | tuple[Iterator, SnpStatsCache]depending on itsreturn_snp_statsflag.Fixing it means
@overloadon the callee inkinship/compute.py, which also clears thatfile's own 6 entries and lets
lmm/loco.pydrop acastwhose comment already claims thefunction "is overloaded on return_snp_stats" — it isn't, yet. Separate PR.