Adopt ecoli-sources data bundle as the ParCa input source#426
Open
cplong90 wants to merge 24 commits into
Open
Adopt ecoli-sources data bundle as the ParCa input source#426cplong90 wants to merge 24 commits into
cplong90 wants to merge 24 commits into
Conversation
Wires the cross-repo data-bundle plumbing on the vEcoli side. The new
ecoli-sources package provides a complete {canonical_key -> source_path}
manifest at ecoli_sources/data/reference_bundle.tsv; SourceBundle reads
that manifest and exposes a small lookup API for ParCa-time consumers.
Dependency: ecoli-sources added to project dependencies. During
development, sourced as a local editable install via [tool.uv.sources]
({ path = "../ecoli-sources", editable = true }) — switches to a
git+ssh pin-by-commit at PR time so cloud campaigns get a deterministic
SHA.
Resolver (wholecell/io/sources.py):
bundle = SourceBundle() # default: ecoli_sources.BUNDLE_PATH
path = bundle.get("rnaseq_basal_tpms") # absolute path; raises with
# available keys if missing
Pure resolver — does not validate file content (Pandera schemas live
in ecoli-sources). Cached at construction time; one bundle per ParCa
run is the intended pattern.
Internal checkpoint: cross-repo bundle resolution mechanism wired up
and validated end-to-end against the rnaseq founding entries
(rnaseq_basal_tpms, rnaseq_experimental_tpms). ParCa-side wiring
(runscripts/parca.py + transcription.py rewire to bundle.get(...))
follows in a later commit.
Part of the cross-repo data-bundle migration. See
SMS/.claude/data-ingestion/bundle-migration-plan.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire the bundle resolver through the ParCa entry point and reroute transcription.py's rnaseq ingestion to read from canonical keys instead of the PR CovertLab#393 manifest+dataset_id pair. Threading: - runscripts/parca.py: new --bundle-manifest-path CLI arg, passed through to fitSimData_1 alongside the legacy rnaseq args. - configs/default.json: bundle_manifest_path defaults to null (resolver falls back to ecoli_sources.BUNDLE_PATH). - fit_sim_data_1.py: threads bundle_manifest_path into sim_data. - simulation_data.py: stores sim_data.bundle_manifest_path; validates the path early when explicitly set. Consumer rewrite (transcription.py): - Drops the if/else against sim_data.rnaseq_manifest_path. SourceBundle is the single mechanism for rnaseq path resolution. - Reads rnaseq_experimental_tpms (Tier 2 — primary TPM source) and, when rnaseq_fill_missing_genes_from_ref is set, also reads rnaseq_basal_tpms (Tier 1 — cross-fill source). - Both use ingest_rnaseq_tpm_table(path) directly; the dataset_id selector goes away from the consumer side. To swap rnaseq sources, pin a different bundle. - Drops RNA_SEQ_ANALYSIS = "rsem_tpm" (orphaned constant; only used by the legacy column-lookup branch we just removed). - Drops the basal_expression_condition column-lookup pattern; the new bundle's basal key resolves to a single-condition file in the new RnaseqTpmTableSchema format (already converted in ecoli-sources). Legacy CLI args (--rnaseq-manifest-path, --rnaseq-basal-dataset-id) and their sim_data attributes are kept in place but unused by the rnaseq consumer — marked DEPRECATED in help text. Removed in a later cleanup commit alongside the rest of the legacy paths. Internal checkpoint scope: imports + bundle resolution validated; a ParCa run end-to-end is the natural next check. Part of the cross-repo data-bundle migration. See SMS/.claude/data-ingestion/bundle-migration-plan.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When SourceBundle is instantiated, validate the loaded manifest against ecoli-sources' ReferenceBundleSchema before building the index. This catches malformed or incomplete bundles at construction time rather than at the first bundle.get(...) call deep inside ParCa. Validation includes the canonical-key contract (the schema's REQUIRED_CANONICAL_KEYS check) so a variant bundle missing a required key fails immediately with a clear error naming the missing key, not later with a confusing KeyError from a downstream consumer. Loaded from `from schemas import ReferenceBundleSchema` — ecoli-sources is already a runtime dependency. Part of the cross-repo data-bundle migration. See SMS/.claude/data-ingestion/bundle-migration-plan.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the FLAT_DIR-based path joins in
reconstruction/ecoli/knowledge_base_raw.py with bundle-driven canonical
key resolution. Each filename string in LIST_OF_DICT_FILENAMES /
LIST_OF_PARAMETER_FILENAMES is converted to a canonical key
(``mass_fractions/glycogen_fractions.tsv`` ->
``mass_fractions__glycogen_fractions``) and looked up via
``SourceBundle.get(...)`` from the ecoli-sources data bundle. The
filename lists themselves remain unchanged and continue to define
vEcoli's data contract — the bundle just provides the resolved paths.
Plumbing:
- KnowledgeBaseEcoli accepts a new ``bundle_manifest_path`` kwarg.
None (default) resolves to ecoli_sources.BUNDLE_PATH; campaigns
pin alternative bundles by passing a path.
- parca.py threads ``config["bundle_manifest_path"]`` through to
KnowledgeBaseEcoli alongside the operons / new_genes options.
- New helper ``_filename_to_canonical_key(filename)`` does the
extension-strip + separator-replace conversion. Mirrors the
canonical-key naming convention in
``ecoli-sources/data/reference_bundle.tsv``.
Filesystem fallback for option-driven subdirectories: the new_genes_data
directory existence assertion and per-subdir file existence checks now
resolve against ``self._flat_root`` (= bundle_root/flat). Bundle-based
``has(...)`` would also work, but composing the path against the
filesystem keeps the existing assertion semantics intact and is robust
to canonical keys not yet registered for newly-introduced new-gene
subdirs.
``__getstate__`` excludes ``_bundle`` and ``_flat_root`` from the
pickled state of ``KnowledgeBaseEcoli`` instances. The bundle is
infrastructure used during __init__'s load phase; including it in
rawData.cPickle would bake absolute machine paths into the artifact
and balloon the file by ~130KB. Verified via ParCa: rawData.cPickle
is byte-identical (15771164 bytes) to a pre-rewire rnaseq-only
run, confirming the loaded data is unchanged and only the resolution
mechanism shifted.
simData.cPickle hash differs run-to-run (sim_data hash:
f6306945b04be7694eb73f7650e71f6af8d518c1a7db06fb2e09f2b6f7452803 in
this run). Likely from ParCa's NLO/fitting nondeterminism, since
rawData is byte-identical and the bundle-resolution change is a pure
path-shifting refactor. Worth verifying with a back-to-back rerun if
hash stability matters.
Part of the cross-repo data-bundle migration. See
SMS/.claude/data-ingestion/bundle-migration-plan.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…truth Removes --rnaseq-manifest-path / --rnaseq-basal-dataset-id and their threading through parca.py -> fitSimData_1 -> SimulationDataEcoli.initialize. The bundle's `rnaseq_basal_tpms` / `rnaseq_experimental_tpms` canonical keys fully subsume what these flags used to address; pointing ParCa at a different bundle replaces the manifest-path-plus-dataset-id pattern. Also deletes the now-obviated configs/test_rnaseq_ingestion.json fixture and removes the matching keys from configs/templates/parca_standalone.json. Doc updates (doc/data_ingestion.rst, doc/workflows.rst) deferred to a follow-up rewrite that describes the bundle pattern positively rather than line-by-line deletions of the legacy fields.
With the bundle pattern, vEcoli is purely a consumer of validated source data. Comparing experimental RNA-seq tables against a reference, computing correlation / RMSE / coverage, etc. is upstream-of-vEcoli concern and naturally lives in ecoli-sources alongside the schema validation pipeline (scripts/validate_all.py, ReferenceBundleSchema, per-source Pandera schemas) -- where data is curated rather than where it is consumed. Deletes: - wholecell/io/data_qc.py - wholecell/io/qc_standalone.py (Marimo notebook; only consumer) - wholecell/tests/io/test_data_qc.py
doc/data_ingestion.rst describes vEcoli as a bundle consumer: how SourceBundle resolves canonical keys, what ParCa configuration controls which bundle is loaded, the three-layer validation pipeline, and pointers to ecoli-sources for source-side authoring. Replaces the prior PR-CovertLab#393-era doc that described the deprecated rnaseq_manifest_path / rnaseq_basal_dataset_id config interface. doc/workflows.rst: swap the two deprecated rnaseq config bullets for the single bundle_manifest_path bullet, with a reference back to data_ingestion. The "currently supported inputs" table reflects the actual surface of the default 135-key bundle (transcriptomics, translation efficiencies, metabolite pool sizes, AA uptake fluxes, secretion bounds, media composition, growth-rate-dependent parameters, reaction networks, regulation). Macroscopic exchange fluxes and intracellular flux distributions are flagged as known gaps.
…ugh sim_data
Adds two columns to ``sim_data.process.transcription.cistron_data`` that
were previously only available by re-reading ``rnas.tsv`` from the
filesystem:
- ``common_name``: per-cistron string column on the structured array
(sourced from ``raw_data.rnas[i]["common_name"]``).
- ``cistron_id_to_synonyms``: separate dict attribute on Transcription,
cistron_id -> list[str] (variable-length lists don't fit cleanly in a
fixed-width structured-array column).
Rewrites the two non-ParCa consumers that previously hardcoded the
flat-file path:
- ``ecoli/analysis/antibiotics_colony/subgen_gene_plots/count_subgen.py``:
drops ``RNAS_TSV_PATH`` and pulls ``common_name`` from cistron_data.
(Note: the script remains gated by an existing
``NotImplementedError("Still need to convert to use DuckDB!")`` --
unrelated migration target.)
- ``data/marA_binding/get_TU_ID.py``: drops ``pd.read_table``/literal_eval
on rnas.tsv and builds the synonym -> cistron-row DataFrame from
sim_data instead.
Together these eliminate the last two consumers that reached past
sim_data into ``reconstruction/ecoli/flat/rnas.tsv``.
…handoff policy
Establishes a new ``sim_data.process.antibiotics`` sub-namespace and
threads cell-wall reference data through it end-to-end, so that nothing
downstream of ParCa reaches back into the bundle for this data.
The change is meant as a **proposed policy** for sim_data flow, not just
a one-off migration: when ParCa loads bundle data on behalf of any
downstream consumer, the consumer should receive that data via sim_data
through the standard ``LoadSimData.get_<process>_config`` pathway --
not by re-reading flat files (or pinning ``ecoli_sources.BUNDLE_PATH``)
at module-import time. sim_data becomes the single, configurable
handoff between ParCa and the simulation.
ParCa side:
- ``reconstruction/ecoli/knowledge_base_raw.py`` gains a ``_load_csv``
loader and a small ``LIST_OF_CSV_FILENAMES`` (currently just
``cell_wall/murein_strand_length_distribution.csv``). Loaded as a
list of dicts on ``raw_data.cell_wall.murein_strand_length_distribution``,
matching the existing TSV-loader convention.
- New ``reconstruction/ecoli/dataclasses/process/antibiotics/`` subpackage
with a ``CellWall`` dataclass that exposes
``sim_data.process.antibiotics.cell_wall.{strand_length_distribution,
strand_term_p}``. ``strand_term_p`` is fitted via the existing
``fit_strand_term_p`` helper at ParCa time so runtime processes can
pull it directly from sim_data.
- ``Antibiotics`` is a new namespace under ``Process`` distinct from the
existing flat sibling pattern (transcription/translation/metabolism/
...). Documented in the dataclass docstring: those siblings back
*universal* whole-cell processes that ParCa fits parameters for; this
namespace carries reference data for *conditional* processes (cell
wall, PBP binding, future MIC tables, etc.) that ParCa does not fit
but does package and ship.
Wiring into runtime processes:
- ``ecoli/library/sim_data.py`` adds ``get_cell_wall_config`` and
``get_pbp_binding_config`` (returning ``strand_term_p`` from sim_data)
and registers both on the ``LoadSimData`` dispatch dict.
- ``ecoli/library/parameters.py`` drops the ``strand_length_data``
Parameter (which read the CSV at module load) and the ``strand_term_p``
derivation rule. Citation comments are preserved in place.
- ``ecoli/processes/antibiotics/cell_wall.py`` and
``pbp_binding.py``: defaults ``strand_term_p`` is now ``None`` with an
init-time assertion that the value must be supplied via the new
config pathway. Bypassers fail loudly rather than silently using a
stale ``param_store`` value.
Plotting / lattice:
- ``ecoli/library/cell_wall/lattice.py``:
``plot_strand_length_distribution`` takes the experimental reference
data structure as an argument; the in-file smoke test loads sim_data
and pulls it from there.
Tests:
- ``test_cell_wall.py`` and ``test_pbp_binding`` (in pbp_binding.py)
were instantiating their processes with ``CellWall({})`` /
``PBPBinding({})``; both now inject the canonical fitted value
(0.058) directly, with a comment pointing at the LoadSimData pathway
that real simulations use.
Open question for review:
- Is establishing ``Antibiotics`` as a separate namespace under
``sim_data.process`` the right cut, vs. flat
``sim_data.process.cell_wall``, vs. top-level ``sim_data.cell_wall``?
Picked it because cell_wall + pbp_binding (and likely future
antibiotics-related reference data) are conditional/optional, not
fit by ParCa, and grouping them coherently makes the asymmetry with
the other ``process.*`` siblings legible.
…tion Adds a migration-pointer note to the docstring of each dormant script under ``reconstruction/ecoli/scripts/`` (six subdirs + one top-level script). With the legacy ``reconstruction/ecoli/flat/`` and ``experimental_data/rnaseq/`` directories deleted in the next commit, these upstream data-prep tools become non-functional in vEcoli; they belong with the curated data they produce, which now lives in ``vivarium-collective/ecoli-sources``. Annotated: - ``fold_changes/calculate_fc.py`` - ``metabolism_kinetics/convert_to_flat.py`` - ``metabolite_concentrations/convert_to_flat.py`` - ``metabolite_concentrations/merge_files.py`` - ``protein_half_lives/convert_to_flat.py`` - ``rna_half_lives/convert_to_flat.py`` - ``update_biocyc_files.py`` Each note points at the cross-repo migration tracking issue (ecoli-sources#2), where coordination of the move (and of the co-located ``nca/`` subdir which is actively maintained on master) will happen as a follow-up. Not migrating here to keep the blast radius bounded and to allow ``nca/``'s owner to weigh in before any files move.
ParCa now resolves every flat-file input through the ecoli-sources reference bundle, and all non-ParCa consumers that previously hardcoded paths into these directories have been migrated to sim_data (or, in the case of upstream data-prep scripts, annotated for follow-up migration to ecoli-sources -- see the previous commit). The legacy directories are no longer read by any runtime code path. Removes 140 files (~13 MB): - ``reconstruction/ecoli/flat/`` -- 133 TSVs/CSVs/FASTA covering the complete ParCa input surface. All canonical keys are preserved in the ecoli-sources reference bundle (see ``ecoli_sources/data/reference_bundle.tsv``). - ``reconstruction/ecoli/experimental_data/rnaseq/`` -- 7 files (6 TSVs + README) from the legacy DI v1 ingestion era. Superseded by ``ecoli_sources/data/rnaseq_experimental/``; the canonical RNA-seq pair (``rnaseq_basal_tpms``, ``rnaseq_experimental_tpms``) in the bundle points at the ecoli-sources copy. Doc / comment cleanups for references that pointed into these dirs: - ``README.md`` -- "raw experimental data" link now points at the ecoli-sources repo and the data_ingestion doc. - ``doc/stores.rst`` -- bulk-molecules location-tag note updated to reference the ``compartments`` canonical key. - ``doc/workflows.rst`` -- ParCa overview and ``new_genes`` config description updated to describe the bundle-based flow. - ``reconstruction/ecoli/dataclasses/adjustments.py`` -- module docstring updated to point at the bundle's ``adjustments__*`` canonical keys instead of the deleted directory. Sequencing reminder: ecoli-sources#1 must merge first; this PR's ``pyproject.toml`` pin updates to that merge SHA.
Surfaced by the end-to-end ParCa runtime check: ``rnas.tsv`` has 25 rows with ``common_name == null`` (parsed by the TSV reader as Python ``None``). 24 of those are pseudo/phantom RNAs filtered out by ``EXCLUDED_RNA_TYPES``, but one (``G0-10706_RNA``, an unnamed mRNA) survives the filter. The new ``common_name`` column build in e115af6 raised ``TypeError: object of type 'NoneType' has no len()`` on that row. Coerce ``None`` -> ``""`` for both the dtype-width computation and the column population. Also guard ``max_common_name_length`` against the all-empty edge case (defaults to 1 for the ``U1`` dtype).
Surfaced by the variant-bundle end-to-end test: when a bundle manifest lives outside ``ecoli_sources/data/`` (e.g. a ``/tmp/`` variant pointing at absolute file paths), ``_load_tsv`` / ``_load_parameters`` / ``_load_csv`` were using path arithmetic between the bundle's ``_flat_root`` and the resolved absolute file path to walk the ``self.<subdir>.<file>`` attribute tree. That arithmetic only works when the resolved file is a child of ``_flat_root`` -- which is true for the default bundle but generally false for variants. The fix decouples the two concerns. Each loader now takes: - ``logical_path``: the historical relative path from ``LIST_OF_DICT_FILENAMES`` / ``LIST_OF_PARAMETER_FILENAMES`` / ``LIST_OF_CSV_FILENAMES`` (e.g. ``mass_fractions/glycogen_fractions.tsv``). Used purely for attribute-tree walking, so files always land at the same ``self.<subdir>.<file>`` location regardless of where the bundle resolved them. - ``file_path``: the absolute path resolved by ``SourceBundle.get(...)``, used for I/O. Call sites updated to pass the ``filename`` iteration variable directly (already the right value) and the bundle-resolved path separately. This makes variant bundles robust: a campaign can save its manifest anywhere and use absolute ``source_path`` values, and the resulting ``raw_data`` attribute tree is identical to the default-bundle case.
… test injections
Two unrelated improvements surfaced by the variant-bundle end-to-end
test:
1) ``_apply_rnaseq_correction`` (transcription.py) builds a list of
cistron indices whose expression was rewritten by the operon-NNLS
correction step, then uses it to flag those rows in
``cistron_data["uses_corrected_seq_counts"]``:
self.cistron_data["uses_corrected_seq_counts"][
np.array(corrected_indexes)
] = True
``np.array([])`` defaults to ``dtype=float64`` -- not a valid index
dtype -- so the line raises ``IndexError`` whenever
``corrected_indexes`` is empty. The default bundle always had at
least one cistron requiring correction (its rnaseq source file
contains zero-expression mRNAs) so the empty path was never
exercised. The ``precise_abx_media:m9_ctrl`` variant exposes it:
after cross-fill from the basal reference (the existing
``rnaseq_fill_missing_genes_from_ref`` flow), no cistron retains
zero expression, so the secondary operon-NNLS correction step
finds nothing to do.
Guarded with ``if corrected_indexes:`` and added explicit
``dtype=int``. Cross-fill semantics (the 403-gene fill) are
independent of this step and unaffected.
2) Test injections in ``test_cell_wall.py`` and ``test_pbp_binding``
(``pbp_binding.py``) used ``strand_term_p=0.058``, which I had
assumed was the canonical fitted value but actually came from an
older ``geom_sampler`` test-data hardcode in ``lattice.py``.
ParCa over the default bundle's strand-length CSV (Obermann &
Höltje 1994, MC4100 strain) actually fits ``0.07675577224377576``.
Updated both injections to the real fitted value with a comment
pointing at the LoadSimData pathway production runs use. The tests
validate plumbing (None-default + assertion + injection threading)
regardless of magnitude, but using the real value is more honest.
Replaces the local-editable ``[tool.uv.sources]`` entry with a
git+ssh pin-by-commit. Verified via ``uv sync`` (clones the SHA from
GitHub and replaces the editable install) and a programmatic check
that ``ecoli_sources.BUNDLE_PATH`` resolves and the bundled data
loads (4747 rnas, 62 strand-length rows -- same shape as the local
checkout).
This satisfies the ``pyproject.toml pin updated`` and ``uv sync
installs ecoli-sources cleanly`` test-plan items for review.
The pin will need one more bump after ecoli-sources#1 merges, since
the merge commit's SHA on ``main`` will differ from the
data-bundle-migration branch tip. That bump happens before this PR
moves out of draft.
For local development against an in-progress ecoli-sources checkout,
override after ``uv sync`` with::
uv pip install -e /path/to/ecoli-sources
A user config that places a parca_options key (e.g. bundle_manifest_path) at the top level instead of nested under parca_options is silently ignored: parca.py only consults config["parca_options"], so the top-level value never reaches run_parca and the run falls back to defaults. Caught by a DESeq-overlay Tier-2 ingestion experiment that produced bit-identical fits across three nominally-different bundles because all three configs misplaced the key. Silent fallback to default is exactly the failure class RFC-010's "manifest is the contract" framing is meant to prevent. After the user-config merge, check whether any default parca_options keys leaked to the top level and raise ValueError if so. Workflow JSONs that short-circuit ParCa via sim_data_path are exempted — those keys are dead config there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings in 122 commits from CovertLab/vEcoli master (security updates, selected_fluxes, emit_paths, different_seeds_per_variant, monomer counts listener tf_ids additions, parquet emitter improvements, etc.) on top of the bundle migration. Conflict resolution: - reconstruction/ecoli/flat/modified_proteins.tsv — modify/delete. vEcoli deleted the file as part of the migration to ecoli-sources; upstream (aa86e78) ported PHOR-MONOMER / DCUS-MONOMER compartment tags from [i] to [c] in four rows. Kept the delete; the upstream change is ported to ecoli-sources data-bundle-migration commit 6691922 (preserves mpg19@stanford.edu authorship). pyproject.toml pin and uv.lock bumped to the new SHA in this commit. Auto-merges audited: - configs/default.json — different_seeds_per_variant added at top level; bundle_manifest_path preserved under parca_options; deprecated rnaseq_manifest_path / rnaseq_basal_dataset_id correctly stay deleted on our side. - ecoli/library/sim_data.py — upstream's tf_ids += [...] and monomer_counts_listener registration coexist with our get_cell_wall_config / get_pbp_binding_config additions. - doc/workflows.rst, README.md — bundle-pattern rewrites preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GitHub Actions runners don't have SSH credentials, so every CI check that runs `uv sync` was failing during the ecoli-sources clone with `git@github.com: Permission denied (publickey)`. The repo is public — HTTPS works without auth. This is also the portable form for the eventual CovertLab/vEcoli PR; their CI has no reason to be configured with vivarium-collective deploy keys either. Local installs resolve to the same commit SHA either way; rerunning uv sync materializes the HTTPS-sourced wheel identically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Left over from the cell_wall refactor that moved strand_term_p fitting out of this module into ParCa. Ruff flags both as F401. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
get_TU_ID.py (the marA-regulation parameter extractor in the Pytest "Extract tetracycline gene regulation parameters" step) was rewritten to source from sim_data when rnas.tsv left vEcoli, but never picked up the rnas.tsv monomer_ids column. It read de_genes["monomer_ids"] and hit KeyError: 'monomer_ids' in CI. Add cistron_id_to_monomer_ids alongside the existing cistron_id_to_synonyms dict on the Transcription dataclass, populated from all_cistrons[i]["monomer_ids"] (the same source the script used to read flat). Update get_TU_ID.py to consume the dict and drop the now-unnecessary json.loads call — synonyms-style native lists rather than JSON-encoded strings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Docker build's uv sync step failed with "Git executable not found" because the base image (ghcr.io/astral-sh/uv) doesn't ship git, but uv needs git to clone ecoli-sources from its HTTPS pin. Adding git to the existing apt-get install line is the minimal fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…undle-migration
get_cell_wall_config and get_pbp_binding_config were returning time_step=1 alongside strand_term_p. In ecoli_master.py's deep-merge of LoadSimData defaults with user process_configs, that time_step override silently replaced cell_wall.py's natural default (10 s), causing the wall to update 10x more often and triggering test_cell_wall_division's "cracked" assertion post-division. Pre-PR, ecoli-cell-wall / ecoli-pbp-binding weren't in the LoadSimData dispatch dict, so get_config_by_name raised KeyError and the composer fell back to process.defaults (preserving time_step=10). Adding the dispatch entries flipped the path; the time_step override is the regression. Fix: omit time_step from the returned dicts. Process defaults propagate through the deep-merge unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thalassemia
previously approved these changes
Jun 8, 2026
thalassemia
left a comment
thalassemia
left a comment
Contributor
There was a problem hiding this comment.
This is a much needed and well thought out change. Addressing the questions raised in the PR body:
- I think it makes sense to bundle any flat file data used by downstream consumers into
sim_datalike you did withstrand_term_p. - No strong preference regarding the antibiotic namespace.
- Of the currently broken scripts,
update_biocyc_files.pyis the most pressing to migrate as it is how we have historically updated the model with new EcoCyc data (in wcEcoli, untested in vEcoli). - Personally, I feel that
ecoli-sourcesshould live under the CovertLab org, but you should ask others and especially Markus before making a final decision.
| # AWS CLI installation necessary for Nextflow's AWS Batch executor | ||
| RUN apt-get update && apt-get install -y gcc procps nano curl g++ ca-certificates unzip \ | ||
| # git is required by uv to fetch git-pinned dependencies (ecoli-sources) | ||
| RUN apt-get update && apt-get install -y gcc procps nano curl g++ ca-certificates unzip git \ |
Contributor
There was a problem hiding this comment.
Can you update runscripts/container/Singularity as well?
Contributor
Author
There was a problem hiding this comment.
Done in b3bb993 — added git to the Apptainer %post install line, mirroring the Dockerfile fix. Thanks!
| have been deleted; curated data now lives in | ||
| ``vivarium-collective/ecoli-sources``. Migrating this script alongside the | ||
| rest of ``reconstruction/ecoli/scripts/`` is tracked at | ||
| https://github.com/vivarium-collective/ecoli-sources/issues/2. |
Contributor
There was a problem hiding this comment.
I really appreciate these notes and the linked issue for scripts that still need to be migrated.
| } | ||
| # Map cistron_id -> list of monomer IDs encoded by the cistron. | ||
| # Same shape rationale as ``cistron_id_to_synonyms``. | ||
| self.cistron_id_to_monomer_ids: dict[str, list[str]] = { |
Contributor
There was a problem hiding this comment.
Appreciate you taking the time to make some of our old antibiotics scripts work with the new bundle system.
Mirror the Dockerfile fix (f2734dc) in the Apptainer recipe: the uv base image doesn't ship git, but uv needs it to clone ecoli-sources from its HTTPS pin during `uv sync`. Per @thalassemia's review on CovertLab#426. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Motivation — scalable, traceable perturbation at the ParCa level
vEcoli's existing variant mechanism does a good job of supporting post-ParCa (sim_data-level) perturbations — swap a parameter on the fitted sim_data object and run a sim. That's where almost all current variant work lives.
What it does not support well is perturbation at the ParCa input level — changing what ParCa fits over, e.g.:
Today these changes require editing files inside
reconstruction/ecoli/flat/(or stitching path-string CLI args into ParCa, where they still exist). That works for one-off exploration, but it doesn't scale and doesn't carry provenance: there's no way to enumerate "the set of input variants ParCa was run against," no content-addressable identity for an input set, and no contract that says "these are the 135 inputs ParCa expects." Trying to systematically run ParCa over many RNA-seq conditions, or programmatically generate transcription-level KO perturbation sets, lands in exploratory-branch territory rather than as a production workflow.This PR turns ParCa's input surface into a first-class, content-addressable contract: a bundle manifest is the unit of input, the
ReferenceBundleSchemaenforces the 135-key canonical contract, and every bundle is a pinned ecoli-sources artifact. Swapping inputs is a config knob (--bundle-manifest-path), not a file edit. Variants are bundles, with the same identity / provenance / validation guarantees as the default reference set.Paired with the multi-ParCa workflow PR (incoming), this is the foundation for running ParCa-level perturbation campaigns at scale — systematic data-ingestion sensitivity sweeps and transcription-level genetic perturbation studies, both with full provenance from input variant → fitted sim_data → simulation output. That capability is load-bearing for the SMS CD3 data-ingestion deliverables and groundwork for CD1/CD2 strain-design and antibiotic-efficacy workstreams that need to interrogate large input perturbation spaces.
The remainder of this PR description is the technical "what" — the input contract, the resolver, the post-ParCa propagation, and the e2e verification.
Summary
Migrates ParCa's flat-file ingestion path off
reconstruction/ecoli/flat/and onto canonical-key resolution through the ecoli-sources reference bundle. ParCa now consumes its 135-key input surface entirely through aSourceBundleresolver pinned to a specific ecoli-sources commit; swapping inputs (different RNA-seq condition, perturbed metabolite pools, alternative kinetic parameters) is done by pointing ParCa at a different bundle manifest, not by editing files inside vEcoli.This PR also completes the post-ParCa propagation of bundle data: every previously identified non-ParCa consumer that read flat files directly is now sourced from sim_data instead. See "Proposed policy" below. With nothing reading from the legacy directories anymore,
reconstruction/ecoli/flat/andreconstruction/ecoli/experimental_data/rnaseq/are deleted.ecoli-sources(vivarium-collective/ecoli-sources) as a git-pinned dependency (pyproject.toml, HTTPS pin so un-authenticated CI/CD runners can fetch it).wholecell/io/sources.py—SourceBundleresolver with eager three-layer validation (manifest schema, path resolution, content schemas) at load time.rnaseq_basal_tpms/rnaseq_experimental_tpmscanonical keys.KnowledgeBaseEcoli's flat-file loading: filename strings become canonical keys resolved via the bundle. AddsLIST_OF_CSV_FILENAMES+ a_load_csvhelper to bringcell_wall/murein_strand_length_distribution.csvonto the load surface. Decouples the loaders' attribute-tree-walking signal from the bundle-resolved I/O path so variant bundles work regardless of where they live on disk.--rnaseq-manifest-path/--rnaseq-basal-dataset-idCLI args (and their plumbing throughparca.py→fitSimData_1→SimulationDataEcoli.initialize).wholecell/io/data_qc.py+ its Marimo notebook + tests: upstream-data QC is no longer vEcoli's responsibility — that work belongs in ecoli-sources alongside the schema validation pipeline.sim_data.process.transcription.cistron_datawithcommon_name(column on the structured array); addscistron_id_to_synonymsandcistron_id_to_monomer_idsdict attributes. Rewrites the two non-ParCa consumers that previously hardcodedrnas.tsv(count_subgen.py,marA_binding/get_TU_ID.py) to source from sim_data.sim_data.process.antibioticssub-namespace (see "Proposed policy" + "Open question" below) carrying cell-wall reference data and the fittedstrand_term_p. Thecell_wall.pyandpbp_binding.pyruntime processes now receivestrand_term_pvia the standardLoadSimData.get_<process>_configpathway;parameters.pydrops thestrand_length_dataParameter and thestrand_term_pderivation rule (citation comments preserved).reconstruction/ecoli/flat/(133 files) andreconstruction/ecoli/experimental_data/rnaseq/(7 files). ~13 MB / ~171 K lines of TSV/CSV/FASTA. All canonical keys preserved in the ecoli-sources reference bundle.reconstruction/ecoli/scripts/as non-functional after the deletion, with a migration pointer to ecoli-sources#2 (tracking issue).doc/data_ingestion.rstanddoc/workflows.rstto describe the bundle pattern; cleans up legacy-path references inREADME.md,doc/stores.rst, andreconstruction/ecoli/dataclasses/adjustments.py.parca_optionskeys at config top level (a footgun surfaced during DESeq-overlay experimentation where top-levelbundle_manifest_pathwas silently ignored).gitin the Docker image souvcan fetch the git-pinnedecoli-sourcespackage.When run with no override, ParCa loads the default reference bundle shipped with the installed ecoli-sources package and produces the same inputs as before the migration.
Companion fork PR (review context)
vivarium-collective/vEcoli#6is the review home for this work — reviewed and approved by @ryan-spangler. This upstream PR carries the same diff againstCovertLab/vEcoli:master(upstream merge9ea8285fintegrated cleanly).Proposed policy: sim_data is the clean handoff
This PR proposes (and implements as a worked example) the policy that sim_data is the single, configurable handoff between ParCa and downstream consumers. Concretely:
LoadSimData.get_<process>_config— the same pathway already used for ~25 other processes. They do not reach back into the bundle, into static module-load-time stores likeparam_store, or into hardcoded flat-file paths.upper_mean, Daly 2011'scritical_radius, etc.) stay inparam_storebecause they have no flat-file source — but bundle-derived values do not.Worked example in this PR:
strand_term_pwas fitted at module-load time insideparameters.pyfrom a hardcoded CSV path. It now (a) lives onsim_data.process.antibiotics.cell_wall.strand_term_p(fitted at ParCa time), (b) flows into the cell_wall and pbp_binding runtime processes via two newLoadSimDataconfig-getters, and (c) is dropped fromparam_store. Bypassers fail loudly via init-time assertions rather than silently picking up a stale value.We'd like explicit reviewer feedback on whether to adopt this as the default for new bundle-derived data going forward. The cell_wall + pbp_binding refactor is the pilot.
Pointing at a custom bundle
Default behavior is unchanged: with no override, ParCa resolves to
ecoli_sources.BUNDLE_PATH— the reference bundle shipped with the pinnedecoli-sourcesinstall — and produces the same inputs as before the migration.To use a different bundle:
runscripts/parca.py --bundle-manifest-path /path/to/my_variant/reference_bundle.tsv ...parca_options.bundle_manifest_pathin the workflow config JSON to the path.The variant manifest must satisfy the canonical-key contract enforced by
ReferenceBundleSchema. Validation runs atSourceBundle.__init__time, so a malformed bundle fails at ParCa load with a clear error naming the offending key.See
doc/data_ingestion.rstfor full consumer-side docs and vivarium-collective/ecoli-sourcesBUNDLES.mdfor the supplier-side authoring guide.Sequencing
The ecoli-sources counterpart — vivarium-collective/ecoli-sources#1 — merged 2026-05-09. The
pyproject.tomlpin in this PR resolves throughmain.Runtime verification
All verification queued at the original 2026-05-06 review is complete. Headline results:
Default-bundle ParCa: clean ✓
Two back-to-back same-config runs (
configs/templates/parca_standalone.json,--cpus 4, default bundle):54eeb7ce1bd83d061e1511f7fc45698d63d3943c544d3e35207e04e2afcad50f974ff397a85f5850ccb093fc28b76850f8a307fbe0322886b928d29cf7ddc38254eeb7ce1bd83d061e1511f7fc45698d63d3943c544d3e35207e04e2afcad50fb340cc500df0f20bdefec8d74fe642d240a2c9936c6109231c5e4d9e2ee8c5c3rawData is bit-identical between the two runs and matches the historical pre-rewire baseline (
__getstate__exclusion of_bundleis doing its job). simData differs by hash but matches by byte count (67,450,711 / 67,450,711) — same shape, different bytes. NLO nondeterminism is confirmed; the cross-commit simData hash differences are normal operational baseline noise, not semantic regression.Variant 1 —
precise_RPO:wt(expected: clean pass) ✓Variant manifest pointing
rnaseq_experimental_tpmsatprecise_RPO:wt.tsv. ParCa completes cleanly in ~5 min; 403 genes filled from basal (matches reference notes for this dataset); ParCa's km-cache key differs between default and variant, confirming ParCa sees the variant as a genuinely different input.Variant 2 —
precise_abx_media:m9_ctrl(expected: TRP-kcats failure) ✓ParCa fails with
ValueError: Could not find positive forward and reverse kcat for TRP[c]atmetabolism.py:set_mechanistic_supply_constants— exactly the failure reference notes predicted for this dataset. Cross-fill consistent (403-gene fill).Two pre-existing bugs surfaced + fixed during e2e
The variant runs surfaced two latent bugs that the default-bundle runs never exercised. Both are pre-existing vEcoli issues, not introduced by this PR; fixed in-line.
1. Loader attribute-tree walking broke for variants outside the manifest's directory tree (commit
109faf44)._load_tsv/_load_parameters/_load_csvwere using path arithmetic between_flat_rootand the resolved absolute file path. That arithmetic only works when the resolved file is a child of_flat_root. Fix: decouple the logical attribute-tree path from the I/O path; variants now work regardless of where they live on disk.2.
_apply_rnaseq_correctionraisedIndexErrorwhen no cistrons required correction (commit45e196f7).np.array([])defaults todtype=float64, which numpy refuses as an index. The default bundle's rnaseq always has at least one cistron requiring correction; cross-filled variants don't. Fix:if corrected_indexes:guard plus explicitdtype=int.Programmatic verification of new sim_data fields ✓
Loaded
out/parca_e2e_a/kb/simData.cPickleand asserted:sim_data.process.transcription.cistron_data["common_name"]populated,dtype=<U49, 4538 rows.sim_data.process.transcription.cistron_id_to_synonymspopulated, 4538 entries.sim_data.process.transcription.cistron_id_to_monomer_idspopulated; backsmarA_binding/get_TU_ID.py's rewrite off rawrnas.tsv.sim_data.process.antibiotics.cell_wall.strand_length_distributionpopulated with 62 rows of the Obermann & Höltje 1994 reference distribution.sim_data.process.antibiotics.cell_wall.strand_term_pfitted to 0.07675577224377576 at ParCa time.LoadSimData.get_cell_wall_config()andget_pbp_binding_config()return{"time_step": 1, "strand_term_p": 0.07675577224377576}as expected.test_cell_wall_divisionregression caught + fixed during review CI ✓The first clean Pytest run on this PR (after the earlier
monomer_idsfix unblocked the full suite) surfaced a single failure inecoli/processes/antibiotics/test_cell_wall_division.py:69(assert not cell_data["wall_state"]["cracked"]on the first timestep post-division). Reproduced deterministically on both fork CI and CovertLab CI (Linux), so not a platform/flake. Root-caused and fixed:ecoli-cell-wallandecoli-pbp-bindingtoLoadSimData.get_config_by_name's dispatch dict sostrand_term_pcould flow throughsim_data.process.antibiotics.cell_wall.strand_term_p.get_cell_wall_configreturned{"time_step": 1, "strand_term_p": ...}. Theecoli_master.pycomposer deep-merges that returned dict withprocess.defaultsbefore instantiating the process, so thetime_step: 1override silently replacedcell_wall.py's natural default oftime_step: 10. The cell-wall process then updated 10× more often, pushing the lattice through aggressive sampling that the mechanics aren't tuned for → wall cracked post-division.get_config_by_nameraisedKeyErrorand the composer fell back toprocess.defaults(preservingtime_step: 10). Adding the dispatch entries flipped the path; thetime_stepoverride was the regression.2b2bedba): omittime_stepfrom the returned dicts inget_cell_wall_configandget_pbp_binding_config. The process's owntime_stepdefault propagates through the deep-merge unchanged. Tests pass locally (test_cell_wall_division.py+ adjacenttest_cell_wall.py+pbp_binding.py).Reasonable nudge to upstream reviewers on the broader pattern: any future
get_<process>_configwe add should be careful about which keys it overrides versus delegates to the process's defaults, particularly for processes that set non-trivialtime_stepvalues.End-to-end sim downstream of ParCa ✓
60 simulated seconds in 8.11 wall-clock seconds via
runscripts/sim.pyagainstout/parca_e2e_a/kb/simData.cPickle. Defaultecoli_master_simconfiguration (no antibiotics processes; the new LoadSimData pathway for cell_wall + pbp_binding is exercised programmatically above and would fire underconfigs/cell_wall.jsonetc.). Standard runtime warnings — same noise pre-migration.Test plan
precise_RPO:wtpasses;precise_abx_media:m9_ctrlreaches the expected TRP-kcats failuresim_data.process.transcription.cistron_data["common_name"],cistron_id_to_synonyms, andcistron_id_to_monomer_idspopulatedsim_data.process.antibiotics.cell_wall.{strand_length_distribution, strand_term_p}populatedpyproject.tomlpin resolves throughecoli-sources:mainuv syncinstalls ecoli-sources cleanly via the HTTPS git pin in un-authenticated CI environmentsconfigs/cell_wall.json,configs/antibiotics_ampicillin.json) —strand_term_pflows throughLoadSimDatato the runtime processes (deferred — needs the antibiotics initial-state files; programmatic verification above covers the contract)doc/data_ingestion.rst,doc/workflows.rstOpen questions for reviewers
Antibiotics sub-namespace
Cell-wall reference data is now placed at
sim_data.process.antibiotics.cell_wall.*— a new sub-namespace underProcess. The existingsim_data.process.*siblings (transcription, translation, metabolism, ...) are flat and back universal whole-cell processes that ParCa fits. Cell wall is conditional/optional (only runs in antibiotics-context simulations) and isn't fit by ParCa. The newAntibioticsnamespace is meant to group conditional-process reference data coherently and signal the asymmetry with the fit-process siblings.Three placements were considered:
sim_data.process.cell_wall— matches existing shape but oversells (no fitting).sim_data.cell_wall— minimal, but lonely if more antibiotics reference data accretes.sim_data.process.antibiotics.cell_wall(chosen) — semantically accurate; introduces a new sub-namespace.We'd like explicit feedback on whether the sub-namespace cut is the right one to commit to.
Should the data-prep script migration ride along?
The seven dormant scripts in
reconstruction/ecoli/scripts/(update_biocyc_files.py,protein_half_lives/,rna_half_lives/,metabolite_concentrations/,metabolism_kinetics/,fold_changes/, plusnca/which is not dormant — actively maintained on master) are non-functional in vEcoli after this PR's deletions. We chose to annotate them with a migration pointer rather than move them, because:wholecell.utils/wholecell.io.tsvimports. ecoli-sources should not depend back on vEcoli.nca/is actively maintained on master — moving it without coordinating with its owner would step on their work.So the migration is tracked at ecoli-sources#2 as the immediate follow-up. Open question for reviewers: does this scoping land, or would you prefer the dormant six fold into this PR with
nca/explicitly excepted?Pre-merge checklist
test_cell_wall_divisionregression caught + fixed (see Runtime verification).long cilabel added; Reproducibility ✓ (23m28s) and Two-gens ✓ (32m43s) green.Where should
ecoli-sourceslive?The reference-data package was created at
vivarium-collective/ecoli-sources— that's where active development has happened to date, alongside thevivarium-collective/vEcolifork. If this PR merges intoCovertLab/vEcoli, the upstream becomes dependent on a package hosted in a different org.Two options:
vivarium-collective/ecoli-sources(status quo). Cross-org pin works; no migration friction. Active maintainers continue to push there. Independent governance is a feature if the package is meant to serve more than justCovertLab/vEcoli(e.g., other Vivarium ecosystem consumers).CovertLab/ecoli-sources. Single-org ownership of the canonical reference-data surface; cleaner for academic citation / institutional governance; reflects the academic prime's stewardship of the data pipeline. Would require a repo transfer (history preserved), updating thepyproject.tomlpin URL, and re-pointing the open issues / docs links.We're open to either; flagging here as a decision the upstream reviewers should weigh in on. Status quo if no objection.
Known follow-ups (out of scope here)
metabolism.pyad-hoc DI workaround) is a follow-up.🤖 Generated with Claude Code