Codebase hygiene + god-file decomposition (behavior-preserving) - #12
Merged
Conversation
The PEP 621 authors->core-metadata mapping routes entries WITH an email into Author-email and name-only entries into the plain Author field, so PyPI's 'Author:' line showed only the name-only co-author. Make both authors name-only and move the contact email to maintainers so PyPI renders 'Author: Thomas Konstantinovsky, Ayelet Peres'.
src/refdata/{id,allele,pool,config,chain,tests}.rs were never wired into
the module tree (refdata.rs declares only 'mod validation;'). They were
stale duplicates of the live inline definitions in refdata.rs and had
diverged: the orphan Allele struct was missing functional_status and
subregions. cargo build output is byte-identical before and after
deletion since these 423 lines never compiled. No behavior change.
Move the 7 manifest-block helpers, the 2 documented-gap constant tuples, and the ~490-line cartridge_manifest body out of data_config.py into a new dataconfig/_manifest.py. DataConfig.cartridge_manifest is now a thin delegator to build_manifest(); SCHEMA_VERSION is threaded as a param to avoid an import cycle. data_config.py drops from 1209 to 502 lines and now holds just the data model. Behavior-preserving: helpers/body moved verbatim (self->cfg only). The two documented-gap tuples are re-exported from data_config for callers that import them. Two orphan-field audit pins gain _manifest.py in their allowed-consumer set (reporting surface, same category as data_config). 277 affected tests pass.
…n.py Move the segment_rates / v_subregion_rates validators, their constant vocabularies, and the clonal descendant-phase step classifier (8 leaf symbols, ~300 lines with no Experiment dependency) out of experiment.py into a new _step_validation.py. experiment.py drops from 3333 to 3045 lines and re-imports the six symbols it still references. Behavior-preserving: symbols moved verbatim; the canonical v_subregion label/alias/default constants and _validate_v_subregion_rates stay accessible as experiment-module attributes (re-export) per the Slice B DSL-boundary pin. Sentinels (_Unset/_UNSET/_LockInput) stay.
_compile.py (the IR-lowering pass: lower_step/_lower_recombine/etc.) sat one letter away from _compiled.py (the runtime CompiledExperiment result objects) - two unrelated concepts with near-identical names. Rename to _lowering.py, matching its lower_* contents. Rewire the single importer (experiment.py), a _pipeline_ir docstring, and three contract tests that read the file by path. No logic change.
Convert the 1002-line single-module Genotype class into a package: - genotype/model.py: the Genotype builder/query/output methods - genotype/_sampling.py: the 15-method population-sampling engine as a _GenotypeSampling mixin (Genotype subclasses it; cls/MRO keep every call site identical) - genotype/_common.py: the shared _SEGMENTS tuple + _alleles_by_gene accessor, imported by both (single definition, no model<->_sampling cycle) - genotype/__init__.py: re-exports Genotype Public paths GenAIRR.genotype and 'from GenAIRR import Genotype' are unchanged. Behavior-preserving: the sampling block moved byte-for-byte; only the mixin base, the _common import, and the relative-import depth (.utilities -> ..utilities) changed. 149 genotype+invariant tests pass.
- dataconfig/enums.py: drop the duplicate 'from enum import Enum' line. - dataconfig/config_info.py: replace 'from .enums import *' with the explicit 'from .enums import ChainType, Species' it actually uses. (An AlleleNComparer.py -> snake_case rename was reverted: the bundled .pkl cartridges embed the GenAIRR.utilities.AlleleNComparer module path, so renaming it breaks unpickling of all 106 cartridges.)
Move the 5 fluent estimator methods (estimate_allele_usage, estimate_trim_distributions, estimate_np_length_distributions, estimate_np_base_model, estimate_p_nucleotide_lengths, ~1500 lines) plus their 4 estimator-only helpers (_NP_CANONICAL_BASES, _split_tie_set, _allele_names_for_segment, _load_rearrangements) into a _CartridgeEstimators mixin. ReferenceCartridgeBuilder subclasses it, so self/MRO keep every call site identical. cartridge_builder.py drops from 2495 to 926 lines; 8 now-unused reference_models imports removed. Behavior-preserving: methods/helpers moved byte-for-byte. 360 estimation + cartridge tests pass; all 106 cartridges still load.
Decompose the 1365-line result.py (SimulationResult god-file) into: - _result_common.py: shared truth-column helpers (_allele_name_or_empty, _inject_truth_columns) used by both core and validation - _result_export.py: _ResultExport mixin (to_dataframe/to_tsv/to_csv/ to_fasta/to_fastq/to_paired_fastq + column helpers) + AIRR-strict helpers - _result_validation.py: ValidationReport, FamilyValidationReport, and the _ResultValidation mixin (validate_records/validate_families/_with_parents) - result.py (207 lines): just the SimulationResult(+WithLineages) data model SimulationResult subclasses both mixins; MRO keeps every call site identical. Mixins carry __slots__ = () to preserve the slotted __dict__-free layout. Public ValidationReport/FamilyValidationReport and the private helpers that cohort/_compiled/tests import are re-exported from result.py, so no import path changes. Behavior-preserving: 507 consumer tests pass; 106 cartridges load.
Move the giant #[cfg(test)] mod tests blocks out of 5 files into sibling
test modules (declaration + file), matching the crate's existing tests/
convention and shrinking the source files:
- passes/mod.rs 1414->~60 -> passes/tests.rs
- refdata/validation.rs -> refdata/validation/tests.rs
- refdata.rs (2 blocks) -> refdata/tests.rs + refdata/gene_index_tests.rs
- passes/receptor_revision.rs -> passes/receptor_revision/tests.rs
- address.rs -> address/tests.rs
Also delete a stale empty '#[cfg(test)] mod tests {}' in
airr_record/validate.rs whose doc comment pointed at a nonexistent file.
Pure test-code move: super::/crate:: paths resolve identically. cargo
test runs the same 1391 tests, all green; build unchanged (2 pre-existing
pyo3 warnings).
Three source-scanning pins referenced files whose content moved (verified present + equivalent at the new path, behavior unchanged): - result column-order np1/np2 + np*_length strings: result.py -> _result_export.py (moved with _DEFAULT_COLUMN_ORDER) - frozen_address_spellings Rust test fixture: address.rs -> address/tests.rs (moved with the inline test extraction)
Convert the 1584-line _cartridge_estimators.py into a private package with one module per independent estimator: - _common.py: shared parse/load helpers (_split_tie_set, _allele_names_for_segment, _load_rearrangements, _NP_CANONICAL_BASES) - allele_usage / trim / np_lengths / np_base_model / p_nucleotide_lengths: one _*EstimatorMixin each (~280-380 lines) - __init__.py: aggregate _CartridgeEstimators mixin (all five bases) ReferenceCartridgeBuilder still inherits _CartridgeEstimators via the unchanged 'from ._cartridge_estimators import _CartridgeEstimators'. Each estimator moved byte-for-byte; imports narrowed per module; mixins carry __slots__ = (). 360 estimation/cartridge tests pass; 106 load.
… package Split the 3045-line Experiment god-class into a 244-line facade + 10 behavior-mixin modules under _experiment/ (corruption, constraints, clonal, mutation, genotype_alleles, recombination, refdata_controls, compile, run, introspection) plus _common.py for the _Unset/_UNSET/ _LockInput sentinels. Experiment inherits all 10; every cross-cluster helper call resolves via MRO, so no logic or call site changed. The facade keeps __init__/on/properties, __slots__, and re-exports the sentinels + pinned _step_validation names + CompiledExperiment/ dataconfig_to_refdata so GenAIRR.experiment.* import paths are unchanged. _UNSET stays a single object (default-arg identity verified). Two audit pins repointed to the moved content (gene_use_dict -> recombination.py, end_loss_* -> corruption.py). Mixins carry __slots__ = (). Behavior-preserving: methods moved verbatim; ~900 experiment/genotype/ clonal/cohort/paired/productive tests pass; 106 cartridges load.
Convert the 906-line _compiled.py (4 independent Compiled* runtime classes) into a private package with one class per module: - plain.py (CompiledExperiment), clonal.py, lineage.py, repertoire.py - __init__.py re-exports all four, preserving 'from ._compiled import ...' and 'from GenAIRR import _compiled; _compiled.<Class>' Classes are fully independent (cross-name mentions are only repr strings/ comments). Each moved byte-for-byte; only same-depth relative imports (. -> ..) adjusted. 619 tests pass; 106 cartridges load.
- Delete dead utilities/misc.py::normalize_and_filter_convert_to_dict (zero callers) and its now-orphaned defaultdict import. - Remove the dead _Unset/_UNSET/_LockInput re-export from experiment.py (no consumer references GenAIRR.experiment._UNSET; the sentinels live in _experiment/_common and mixins import them from there). - Repoint cohort.py and mcp_server.py through-shim imports to their canonical homes (_result_export._DEFAULT_COLUMN_ORDER/_to_airr_strict; _refdata_resolver._CONFIG_ALIASES) instead of the result/experiment re-export shims. Behavior-preserving; 466 cohort/mcp/experiment/distribution tests pass.
data_config.py (the data-model layer frozen into all 106 bundled cartridge pickles) eagerly imported ReferenceEmpiricalModels / ReferenceRulesSpec / PopulationGenotypeModel purely for field annotations - forcing every pickle load to drag in the whole cartridge-authoring subtree (a dependency-direction inversion). Add 'from __future__ import annotations' and move those three imports under 'if TYPE_CHECKING'. Annotations are strings (no get_type_hints/ runtime eval in the module), defaults are all None, and pickle uses the class path + __dict__ (not annotations), so unpickling is unaffected. Runtime users of these types (e.g. _manifest.py isinstance) import them themselves. 106 cartridges load; 116 manifest/genotype/cartridge tests pass.
genotype_priors, cohort, reference_models, reference_rules had no __all__, so their public contract lived only in docs and 'import *' would leak stdlib/dataclass internals. Add explicit __all__ listing each module's documented public surface (the spec/result classes + reference_models' TRIM_KEYS/NP_KEYS/... constants). Keeps the documented GenAIRR.genotype_priors / GenAIRR.cohort / GenAIRR.reference_models paths namespaced (no top-level surface expansion). __all__ only affects 'import *'; direct imports (incl. SCHEMA_TAG) are unaffected. 105 tests pass.
Split the 844-line utilities/visualize.py into utilities/_visualize/: - parse.py (safe getters + mutation parsing), styles.py (colors + CSS), components.py (8 HTML builders), alignment.py (germline alignment), render.py (visualize_sequence) - visualize.py is now an 8-line facade re-exporting visualize_sequence, which is also re-added to utilities/__init__ as the public entry point. Behavior-preserving: functions moved verbatim; leaves=parse/styles, components->parse+styles, render->all (no cycles). Verified by a byte-identical HTML render gate - a rich seeded record renders to the exact same 84588-byte output (sha256 unchanged) before and after.
mcp_helpers.py was imported by nothing (no static/dynamic/string ref in src) and its functions duplicate the live _mcp_* modules with different signatures (e.g. find_allele(dc, name) vs _mcp_refdata.find_allele(rd, segment, name)). It dates from the pre-_mcp_* era; mcp_server.py uses the _mcp_refdata/_mcp_summary/_mcp_presets/_mcp_validators modules instead. Remove the two dead contract pins that read its source (test_pin_present_mcp_helpers_gene_use_endpoint_is_read_only, test_pin_scaffold_mcp_p_nucleotides_endpoint_is_read_only + its _MCP_HELPERS_PY path) and drop mcp_helpers.py from three allowed/expected consumer sets (the v_region one asserts strict equality). 119 affected pin + mcp_server tests pass; 106 cartridges load.
Decompose airr_record/validate.rs (1461 lines) into a 356-line root + 6 family submodules under validate/: paired_end, structural, counters, junction, allele_oracle, region_invariants. The root keeps the 5 public enums (RecordValidationIssue + the paired-end/decided-by/order enums) and validate_airr_record, and path-qualifies its calls into the families. Family entry points are pub(super); intra-family helpers stay private; no cross-family calls. Public surface at airr_record::validate::* (the mod.rs re-exports) is unchanged. Behavior-preserving: functions moved verbatim; cargo test runs the same 1391 tests, all green; build clean.
The validate.rs family split relocated check_paired_end_geometry (+ its Read* issue constructions) to validate/paired_end.rs and check_counters (+ the MUTATE_UNIFORM/MUTATE_S5F allowlist + mismatch constructions) to validate/counters.rs. Two Python contract pins grep the Rust source by path and broke: - test_paired_end_schema::test_slice_b_geometry_check_helpers_landed_in_engine_source -> validate/paired_end.rs - test_v_subregion_mutation_counters_contract::test_pin_scaffold_validator_recomputes_independently -> validate/counters.rs Verified every required string is present at the new path (content moved, not lost). The sibling pin at line 474 still reads validate.rs (its enum variants stayed) and is unchanged.
Pre-merge cleanup so the tracked repo carries no development-tooling traces: - Remove the stale session-artifact pin test and reword the 'session planning artifacts' prose in docs_website_audit.md + test_docs_website_contract.py / test_docs_mkdocs_migration_contract.py / test_mcp_server.py to generic 'private planning notes'. - Strip dangling .private/ design-doc pointers from module docstrings (cohort.py, genotype_priors.py, engine_rs/src/lib.rs) and from the DataConfig schema-mismatch error message (which pointed users at a non-shipped private migration script) + allele_model_audit.md. Kept: the MCP-client config docs (Claude Code / Cursor) in README + mcp_server.py (legitimate product documentation of the MCP server's clients), and the private build-script references that functional tests conditionally read. No code behavior changes.
Port the legacy .private/scripts/build_imgt_configs.py into a tracked, tested maintainer tool at tools/build_imgt_configs.py. It fetches IMGT V-QUEST germline FASTA and builds structural DataConfig cartridges via ReferenceCartridgeBuilder (from_fasta -> infer_identity -> infer_v_subregions -> build), replacing the removed RandomDataConfigBuilder. - Structural output: germline V/D/J + IMGT FWR/CDR annotations, no data-derived distributions (engine uniform defaults at run time). - --output-dir is required: never silently overwrites the shipped builtin_dataconfigs set (a structural rebuild there would move the golden baselines). - tools/README.md documents usage + the structural/uniform caveat. - tests/test_build_imgt_configs.py: inline-FASTA fixtures (no network) cover VDJ/VJ build, missing-D guard, Experiment.on() compile, and the required --output-dir CLI contract. - Flip the two obsolete legacy-stub pins to pin the new tool; repoint the audit-doc references from .private/scripts to tools/. 92 affected tests pass.
…utions Users should not assume every bundled species/locus cartridge has data-derived parameters. Add a prominent callout in the reference- cartridge concept page (Empirical models section) stating that only the human IGH/IGK/IGL/TCRB cartridges ship distributions fitted from real repertoire data; every other bundled cartridge has a real germline catalogue but uniform/placeholder data-derived parameters (trim/NP length, NP Markov, allele usage) - deliberate uniform coverage, not ground-truth statistics - with a pointer to fitting real params via the cartridge estimators. Add a one-line linked caveat where quick-start lists the non-human shortcuts (mouse_igh, etc.).
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.
Pure reorganization: no algorithm, numeric-output, trace-address, schema, seed, public-import-path, or pickle-load changes. Every commit is an isolated, individually-verified move. 90 files, net −840 lines (code relocated + dead code removed).
Verification
God-files decomposed
experiment.py_experiment/(10 mixins +_commonsentinels)_cartridge_estimators.py_cartridge_estimators/(one estimator per module)_compiled.py_compiled/(one class per runtime shape)result.py_result_export/_result_validation/_result_commondataconfig/data_config.pydataconfig/_manifest.py)genotype.pygenotype/(model+_samplingmixin +_common)utilities/visualize.py_visualize/airr_record/validate.rs(Rust)validate/6 family submodulespasses/mod.rs(+4 Rust files)tests.rsmodulesDead code removed
engine_rs/src/refdata/{id,allele,pool,config,chain,tests}.rs— 423 lines, never compiled, divergent duplicates.utilities/mcp_helpers.py— 946 lines, superseded by the_mcp_*modules (zero importers).utilities/misc.py::normalize_and_filter_convert_to_dict+ a dead sentinel re-export.Design / API hygiene (from a structure & coupling review)
data_config.py(frozen into all 106 pickles) no longer force-loads the authoring layer — deferred toTYPE_CHECKING.__all__added to the documented public modules (genotype_priors,cohort,reference_models,reference_rules).cohort,mcp_server) repointed to canonical homes._compile.py→_lowering.py(ends the_compile/_compiledconfusion).How behavior-preservation was held
self/cls/MRO and__slots__layouts are identical; shared module-level state (sentinels, leaf helpers) lives in a single_common(never duplicated).AlleleNComparerrename that broke all 106 cartridges was reverted).validate.rs.Deferred (documented in the private refactor plan)
mcp_server.py's@mcp.tool()decomposition and ~8 remaining Rust internal files (python/plan.rs,ir/builder.rs,trace_file.rs,python/refdata.rs,python/outcome.rs,address.rs, …), with an execution tracker and gates (incl. running pytest after Rust splits for cross-language pins).