diff --git a/CHANGELOG.md b/CHANGELOG.md index 065508a..cadf5dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ ## Unreleased +* New: `save_ec_model` records the ecModel format (`gecko_format: "4"`) and + the release that wrote the file (`generator: geckopy `) in the + `metaData`, the keys GECKO's `saveEcModel` writes, in place of + `geckopy_version`. The model's own metaData, its date included, is kept. + `load_ec_model` refuses a `gecko_format` above 4 and warns about a GECKO 3 + ecModel with a message that names `upgrade_project`; it turns the same + protein reactions as GECKO's `loadEcModel`, including a usage reaction fixed + to zero. `upgrade_project` reports an ecModel it cannot read as failed + and goes on with the other steps. * New: `level_complex_concs` and `level_complex_abundances` give every enzyme complex one abundance, the median per copy over its measured subunits, so a single badly quantified subunit no longer sets a complex's capacity. diff --git a/docs/raven_integration.md b/docs/raven_integration.md index 970ad8f..30f83db 100644 --- a/docs/raven_integration.md +++ b/docs/raven_integration.md @@ -38,7 +38,7 @@ add three application-level concerns on top: 1. **File-extension validation** — only `.yml` / `.yaml` is accepted (SBML ecModel I/O was dropped; the message points users at the new contract). 2. **Adapter-aware path resolution** — relative filenames resolve under `/models/`. -3. **Provenance + diagnostics** — `save_ec_model` injects a `metaData` block (date + geckopy version + description); both wrappers fast-fail with clear messages when the YAML isn't an ecModel or the in-memory model has an empty `ec`. +3. **Provenance + diagnostics** — `save_ec_model` writes a `metaData` block (the model's own metaData with its date, the ecModel format `gecko_format`, the release as `generator`, and a description); both wrappers fast-fail with clear messages when the YAML isn't an ecModel or the in-memory model has an empty `ec`. On-disk format is the same RAVEN/cobrapy YAML in both directions: files written by geckopy load in raven-toolbox (as a `cobra.Model` with diff --git a/src/geckopy/upgrade.py b/src/geckopy/upgrade.py index 433f960..2ef4d6c 100644 --- a/src/geckopy/upgrade.py +++ b/src/geckopy/upgrade.py @@ -246,12 +246,16 @@ def keep_original(path: Path) -> None: from .utilities.load_ec_model import load_ec_model from .utilities.save_ec_model import save_ec_model - # read_yaml_model already turns GECKO 3 protein reactions forward, with a - # warning that names how many; the file itself keeps the old direction. + # load_ec_model turns GECKO 3 protein reactions forward, with a warning + # that names how many; the file itself keeps the old direction. with warnings.catch_warnings(record=True) as caught: warnings.simplefilter("always") - model = load_ec_model(path) - flipped = [re.search(r"for (\d+) protein", str(w.message)) for w in caught] + try: + model = load_ec_model(path) + except Exception as err: # an ecModel this geckopy cannot read stops only its own row + rows.append(("saved ecModel", "failed", f"{path.relative_to(project)}: {err}")) + continue + flipped = [re.search(r"(\d+) protein usage", str(w.message)) for w in caught] flipped = [int(m.group(1)) for m in flipped if m] changed = upgrade_ec_model(model) n_turned = sum(flipped) + len(changed) diff --git a/src/geckopy/utilities/load_ec_model.py b/src/geckopy/utilities/load_ec_model.py index f10b87f..8853935 100644 --- a/src/geckopy/utilities/load_ec_model.py +++ b/src/geckopy/utilities/load_ec_model.py @@ -20,6 +20,13 @@ clear message if the YAML lacks the ec sections, and attach the adapter to ``model.adapter`` so downstream geckopy code finds it. +4. **ecModel format** — a file whose protein usage and pool reactions + raven-toolbox turned forward from the GECKO 3 direction gives a + warning that names :func:`~geckopy.upgrade_project`; an ecModel + format (``gecko_format`` in the file's ``metaData``, see + :func:`~geckopy.save_ec_model`) above 4 is an error. GECKO's + ``loadEcModel`` does the same. + geckopy has no global default adapter: pass one explicitly, or use an absolute filename. @@ -27,17 +34,22 @@ """ from __future__ import annotations +import re +import threading +import warnings from pathlib import Path from typing import TYPE_CHECKING, Optional, Union from raven_toolbox.io import EcData, read_yaml_model from ..ec_model.ec_model import EcModel +from ..upgrade import upgrade_ec_model if TYPE_CHECKING: from ..adapter import ModelAdapter +_HOOK_LOCK = threading.Lock() # warnings.showwarning is one global per process _DEFAULT_FILENAME = "ecModel.yml" _YAML_SUFFIXES = {".yml", ".yaml"} @@ -71,14 +83,39 @@ def load_ec_model( ------ ValueError If the extension isn't YAML, the filename is relative without an - adapter, or the YAML lacks the GECKO ``ec-rxns`` / ``ec-enzymes`` - sections (in which case it isn't an ecModel). + adapter, the YAML lacks the GECKO ``ec-rxns`` / ``ec-enzymes`` + sections (in which case it isn't an ecModel), or its ecModel + format is above 4. FileNotFoundError If the resolved file doesn't exist. """ path = _resolve_path(filename, adapter) - cobra_model = read_yaml_model(path) + # read_yaml_model turns GECKO 3 protein reactions forward, with a warning + # that names how many. That warning is counted and replaced; every other + # warning goes through the caller's filters and display unchanged. + turned = 0 + show = warnings.showwarning + + def count_turned(message, category, filename, lineno, file=None, line=None): + nonlocal turned + m = re.search(r"reverse-direction convention for (\d+) protein", str(message)) + if m: + turned += int(m.group(1)) + else: + show(message, category, filename, lineno, file, line) + + with _HOOK_LOCK, warnings.catch_warnings(): + warnings.filterwarnings("always", message=r"ecModel uses the older reverse-direction convention") + warnings.showwarning = count_turned + cobra_model = read_yaml_model(path) + meta = (cobra_model.notes or {}).get("metaData") or {} + if _format_number(meta.get("gecko_format")) > 4: + raise ValueError( + f"{path} has ecModel format {meta['gecko_format']} (written by " + f"{meta.get('generator', 'an unknown release')}); this geckopy reads " + "format 4 and older. Update geckopy to load it." + ) ec = getattr(cobra_model, "ec", None) if not isinstance(ec, EcData) or ec.n_rxns == 0 or ec.n_enzymes == 0: raise ValueError( @@ -90,9 +127,29 @@ def load_ec_model( cobra_model, adapter=adapter, gecko_light=ec.gecko_light, ) ec_model.ec = ec + # raven-toolbox turns the reactions with a negative lower bound; GECKO + # turns those whose stoichiometry has the GECKO 3 sign, which also + # covers a usage reaction fixed to zero. + turned += len(upgrade_ec_model(ec_model)) + if turned: + warnings.warn( + f"{path} has {turned} protein usage and pool reactions in the GECKO 3 direction; " + "they are turned forward in the loaded model. upgrade_project upgrades the " + "files of the project.", + stacklevel=2, + ) return ec_model +def _format_number(value) -> float: + """``gecko_format`` as a number; a value that is not a number counts as + no format, as GECKO's ``loadEcModel`` reads it with ``str2double``.""" + try: + return float(value) + except (TypeError, ValueError): + return float("nan") + + def _resolve_path( filename: Optional[Union[str, Path]], adapter: Optional["ModelAdapter"], diff --git a/src/geckopy/utilities/save_ec_model.py b/src/geckopy/utilities/save_ec_model.py index 31875a7..f49c97a 100644 --- a/src/geckopy/utilities/save_ec_model.py +++ b/src/geckopy/utilities/save_ec_model.py @@ -16,8 +16,8 @@ ``/models/``. 3. **Empty-ecModel guard + provenance injection** — fast-fail when the caller forgot to run ``make_ec_model``; write a small ``metaData`` - block (date + geckopy version + description) so saved files carry - provenance information. + block (date, ecModel format, geckopy release, description) so saved + files carry provenance information. The provenance/description metadata is written only into the saved YAML; the in-memory model's ``notes`` are mutated only transiently for @@ -133,17 +133,24 @@ def _resolve_path( def _build_metadata(model: EcModel) -> dict: """Provenance block written under the top-level ``metaData`` key. - Save date, geckopy version, and a description string of the form - ``"Enzyme-constrained model of "``. Author / email / - organization / taxonomy could be sourced from ``adapter.params`` - once those fields are added to ``ModelParameters``. + The model's own ``metaData`` (from the file it was read from, or the + conventional model it was made from) with ``date`` kept or set to + today, as RAVEN keeps ``model.date``; ``gecko_format``, the ecModel + format the file follows (as ``project_format`` in + ``model_adapter.toml``, written as text as GECKO's ``saveEcModel`` + writes it); ``generator``, the release that wrote the file; and a + description string of the form ``"Enzyme-constrained model of "``. + GECKO's ``saveEcModel`` writes the same ``gecko_format`` and + ``generator``. """ try: geckopy_version = version("geckopy") except PackageNotFoundError: geckopy_version = "unknown" - return { - "date": date.today().isoformat(), - "geckopy_version": geckopy_version, - "description": f"Enzyme-constrained model of {model.id}", - } + meta = dict((model.notes or {}).get("metaData") or {}) + meta.pop("geckopy_version", None) + meta["date"] = meta.get("date") or date.today().isoformat() + meta["gecko_format"] = "4" + meta["generator"] = f"geckopy {geckopy_version}" + meta["description"] = f"Enzyme-constrained model of {model.id}" + return meta diff --git a/tests/test_load_ec_model.py b/tests/test_load_ec_model.py index 618a543..7cb875b 100644 --- a/tests/test_load_ec_model.py +++ b/tests/test_load_ec_model.py @@ -421,9 +421,10 @@ def test_autoflip_legacy_reverse_direction(tmp_path): flipped = load_ec_model("ecModel.yml", adapter=adapter) msgs = [str(w.message) for w in caught] - assert any("forward convention" in m for m in msgs), ( - f"expected a flip warning, got: {msgs}" + assert any("in the GECKO 3 direction" in m and "upgrade_project" in m for m in msgs), ( + f"expected a GECKO 3 warning, got: {msgs}" ) + assert not any("forward convention" in m for m in msgs) # Every flipped reaction should now look like the canonical fixture. canonical_doc = _canonical_yaml() @@ -486,3 +487,64 @@ def test_legacy_metadata_and_smiles_normalized(tmp_path): assert model.name == "demo ecModel" met = model.metabolites.get_by_id("A_c") assert met.annotation.get("smiles") in ("C(C)O", ["C(C)O"]) + + +def test_newer_ecmodel_format_raises(tmp_path): + """A file of an ecModel format above 4 is refused, naming its writer.""" + adapter = _adapter(tmp_path) + doc = _canonical_yaml() + doc["metaData"] = {"id": "demo", "gecko_format": "5", "generator": "geckopy 9.0.0"} + _write_yaml(tmp_path / "models" / "ecModel.yml", doc) + with pytest.raises(ValueError, match=r"ecModel format 5 \(written by geckopy 9\.0\.0\)"): + load_ec_model("ecModel.yml", adapter=adapter) + + +def test_format_4_file_loads_without_warning(tmp_path): + import warnings + + adapter = _adapter(tmp_path) + doc = _canonical_yaml() + doc["metaData"] = {"id": "demo", "gecko_format": "4"} + _write_yaml(tmp_path / "models" / "ecModel.yml", doc) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + load_ec_model("ecModel.yml", adapter=adapter) + assert not [w for w in caught if "ecModel" in str(w.message)] + + +def test_format_that_is_not_a_number_loads(tmp_path): + """As GECKO's str2double, a gecko_format that is not a number counts as none.""" + adapter = _adapter(tmp_path) + doc = _canonical_yaml() + doc["metaData"] = {"id": "demo", "gecko_format": "4.1.0"} + _write_yaml(tmp_path / "models" / "ecModel.yml", doc) + load_ec_model("ecModel.yml", adapter=adapter) + + +def test_newer_format_without_generator_names_unknown_release(tmp_path): + adapter = _adapter(tmp_path) + doc = _canonical_yaml() + doc["metaData"] = {"id": "demo", "gecko_format": "5"} + _write_yaml(tmp_path / "models" / "ecModel.yml", doc) + with pytest.raises(ValueError, match="written by an unknown release"): + load_ec_model("ecModel.yml", adapter=adapter) + + +def test_other_reader_warnings_pass_through(tmp_path, monkeypatch): + """Only raven-toolbox's GECKO 3 warning is replaced; other warnings reach the caller.""" + import importlib + import warnings + + lem = importlib.import_module("geckopy.utilities.load_ec_model") + + adapter = _adapter(tmp_path) + _write_yaml(tmp_path / "models" / "ecModel.yml", _canonical_yaml()) + read = lem.read_yaml_model + + def read_with_warning(path): + warnings.warn("unrelated reader warning", UserWarning) + return read(path) + + monkeypatch.setattr(lem, "read_yaml_model", read_with_warning) + with pytest.warns(UserWarning, match="unrelated reader warning"): + load_ec_model("ecModel.yml", adapter=adapter) diff --git a/tests/test_save_ec_model.py b/tests/test_save_ec_model.py index 1b76ff3..9c89601 100644 --- a/tests/test_save_ec_model.py +++ b/tests/test_save_ec_model.py @@ -208,8 +208,27 @@ def test_metadata_includes_date_version_description(tmp_path): doc = _read_yaml(path) md = doc["metaData"] assert md["date"] == date.today().isoformat() - assert "geckopy_version" in md + assert md["gecko_format"] == "4" + assert md["generator"].startswith("geckopy ") + assert "geckopy_version" not in md assert md["description"] == "Enzyme-constrained model of demo" + # The same lines as GECKO's saveEcModel writes. + text = path.read_text() + assert ' - gecko_format: "4"\n' in text + assert " - generator: geckopy " in text + + +def test_metadata_keeps_model_metadata(tmp_path): + """The model's own metaData, its date included, is kept, as RAVEN keeps model.date and annotation.""" + adapter = _adapter(tmp_path) + model = _build_full_ec_model(adapter) + model.notes["metaData"] = {"date": "2025-01-31", "geckopy_version": "4.0.0b1", + "taxonomy": "taxonomy/4932", "organization": "Chalmers"} + doc = _read_yaml(save_ec_model(model)) + assert doc["metaData"]["date"] == "2025-01-31" + assert doc["metaData"]["taxonomy"] == "taxonomy/4932" + assert doc["metaData"]["organization"] == "Chalmers" + assert "geckopy_version" not in doc["metaData"] def test_input_model_not_mutated(tmp_path): diff --git a/tests/test_upgrade.py b/tests/test_upgrade.py index 0c7369b..f15ab64 100644 --- a/tests/test_upgrade.py +++ b/tests/test_upgrade.py @@ -111,8 +111,40 @@ def test_upgrade_project(tmp_path, capsys): assert (legacy / "data" / "customKcats.tsv").is_file() assert (legacy / "models" / "ecModel.yml").is_file() assert upgrade_ec_model(load_ec_model(project / "models" / "ecModel.yml")) == [] + assert ' - gecko_format: "4"\n' in (project / "models" / "ecModel.yml").read_text(encoding="utf-8") again = upgrade_project(project) assert set(again["status"]) == {"by hand"} assert main(["upgrade", str(project)]) == 0 assert "select_kcat_value -> apply_kcat_list" in capsys.readouterr().out + + +def test_upgrade_project_reports_newer_format_as_failed(tmp_path): + model, adapter, project = _project(tmp_path) + save_ec_model(_gecko3(model), project / "models" / "ecModel.yml") + save_ec_model(model, project / "models" / "newer.yml") + newer = project / "models" / "newer.yml" + newer.write_text(newer.read_text(encoding="utf-8").replace('gecko_format: "4"', 'gecko_format: "5"'), + encoding="utf-8") + report = upgrade_project(project) + rows = report[report["step"] == "saved ecModel"] + failed = rows[rows["note"].str.contains("newer.yml", regex=False)] + assert list(failed["status"]) == ["failed"] and "format 5" in failed["note"].iloc[0] + assert "ecModel.yml" in " ".join(rows[rows["status"] == "to do"]["note"]) + + +def test_usage_reaction_fixed_to_zero_is_turned(tmp_path): + """As GECKO's loadEcModel, a GECKO 3 usage reaction with both bounds 0 is turned forward too.""" + import warnings + + model, adapter, project = _project(tmp_path) + old = _gecko3(model) + usage = next(r for r in old.reactions if r.id.startswith("usage_prot_")) + usage.bounds = (0, 0) + save_ec_model(old, project / "models" / "ecModel.yml") + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + loaded = load_ec_model(project / "models" / "ecModel.yml") + assert {m.id: c for m, c in loaded.reactions.get_by_id(usage.id).metabolites.items()} == \ + {m.id: c for m, c in model.reactions.get_by_id(usage.id).metabolites.items()} + assert any("GECKO 3 direction" in str(w.message) for w in caught)