From 81470491b0db53d2a76904a4196764a2c75fcc5c Mon Sep 17 00:00:00 2001 From: Daniel Weindl Date: Mon, 25 Nov 2024 22:21:40 +0100 Subject: [PATCH] Raise during unsupported model imports We can't import multiple extensions with the same name. Raise if this is attempted. Related to #1936. --- python/sdist/amici/__init__.py | 50 +++++++++++++- python/tests/conftest.py | 24 ++++++- python/tests/test_edata.py | 3 +- python/tests/test_observable_events.py | 12 ++-- python/tests/test_sbml_import.py | 96 ++++++++++++++++++-------- swig/modelname.template.i | 17 +++++ 6 files changed, 164 insertions(+), 38 deletions(-) diff --git a/python/sdist/amici/__init__.py b/python/sdist/amici/__init__.py index ba0391900c..f08d9e9349 100644 --- a/python/sdist/amici/__init__.py +++ b/python/sdist/amici/__init__.py @@ -7,10 +7,12 @@ """ import contextlib +import datetime import importlib import os import re import sys +import sysconfig from pathlib import Path from types import ModuleType as ModelModule from typing import Any @@ -140,8 +142,7 @@ def get_model(self) -> amici.Model: """Create a model instance.""" ... - def get_jax_model(self) -> JAXModel: - ... + def get_jax_model(self) -> JAXModel: ... AmiciModel = Union[amici.Model, amici.ModelPtr] @@ -183,8 +184,53 @@ def import_model_module( raise ValueError(f"module_path '{module_path}' is not a directory.") module_path = os.path.abspath(module_path) + ext_suffix = sysconfig.get_config_var("EXT_SUFFIX") + ext_mod_name = f"{module_name}._{module_name}" # module already loaded? + if (m := sys.modules.get(ext_mod_name)) and m.__file__.endswith( + ext_suffix + ): + # this is the c++ extension we can't unload + loaded_file = Path(m.__file__) + needed_file = Path( + module_path, + module_name, + f"_{module_name}{ext_suffix}", + ) + if not needed_file.exists(): + # if we import a matlab-generated model where the extension + # is in a different directory + needed_file = Path( + module_path, + f"_{module_name}{ext_suffix}", + ) + + if not loaded_file.samefile(needed_file): + # this is not the right module, and we can't unload it + raise RuntimeError( + f"Cannot import extension for {module_name} from " + f"{module_path}, because an extension with the same name was " + f"has already been imported from {loaded_file.parent}. " + "Import the module with a different name or restart the " + "Python kernel." + ) + # this is the right file, but did it change on disk? + t_imported = m._get_import_time() # noqa: protected-access + t_modified = os.path.getmtime(m.__file__) + if t_imported < t_modified: + t_imp_str = datetime.datetime.fromtimestamp(t_imported).isoformat() + t_mod_str = datetime.datetime.fromtimestamp(t_modified).isoformat() + raise RuntimeError( + f"Cannot import extension for {module_name} from " + f"{module_path}, because an extension in the same location " + f"has already been imported, but the file was modified on " + f"disk. \nImported at {t_imp_str}\nModified at {t_mod_str}.\n" + "Import the module with a different name or restart the " + "Python kernel." + ) + + # unlike extension modules, Python modules can be unloaded if module_name in sys.modules: # if a module with that name is already in sys.modules, we remove it, # along with all other modules from that package. otherwise, there diff --git a/python/tests/conftest.py b/python/tests/conftest.py index d8d882fcfd..3d4856d084 100644 --- a/python/tests/conftest.py +++ b/python/tests/conftest.py @@ -7,7 +7,10 @@ import amici import pytest -from amici.testing import TemporaryDirectoryWinSafe +from amici.testing import TemporaryDirectoryWinSafe as TemporaryDirectory +from pathlib import Path + +EXAMPLES_DIR = Path(__file__).parent / ".." / "examples" @pytest.fixture(scope="session") @@ -32,7 +35,7 @@ def sbml_example_presimulation_module(): ) module_name = "test_model_presimulation" - with TemporaryDirectoryWinSafe(prefix=module_name) as outdir: + with TemporaryDirectory(prefix=module_name) as outdir: sbml_importer.sbml2amici( model_name=module_name, output_dir=outdir, @@ -71,7 +74,7 @@ def pysb_example_presimulation_module(): model.name = "test_model_presimulation_pysb" - with TemporaryDirectoryWinSafe(prefix=model.name) as outdir: + with TemporaryDirectory(prefix=model.name) as outdir: pysb2amici( model, outdir, @@ -81,3 +84,18 @@ def pysb_example_presimulation_module(): ) yield amici.import_model_module(model.name, outdir) + + +@pytest.fixture(scope="session") +def model_units_module(): + sbml_file = EXAMPLES_DIR / "example_units" / "model_units.xml" + module_name = "test_model_units" + + sbml_importer = amici.SbmlImporter(sbml_file) + + with TemporaryDirectory() as outdir: + sbml_importer.sbml2amici(model_name=module_name, output_dir=outdir) + + yield amici.import_model_module( + module_name=module_name, module_path=outdir + ) diff --git a/python/tests/test_edata.py b/python/tests/test_edata.py index fab49c160e..0baa7443fb 100644 --- a/python/tests/test_edata.py +++ b/python/tests/test_edata.py @@ -2,11 +2,12 @@ import amici import numpy as np +import pytest from amici.testing import skip_on_valgrind -from test_sbml_import import model_units_module # noqa: F401 @skip_on_valgrind +@pytest.mark.usefixtures("model_units_module") def test_edata_sensi_unscaling(model_units_module): # noqa: F811 """ ExpData parameters should be used for unscaling initial state diff --git a/python/tests/test_observable_events.py b/python/tests/test_observable_events.py index 2887308ff6..db6fc3d452 100644 --- a/python/tests/test_observable_events.py +++ b/python/tests/test_observable_events.py @@ -149,8 +149,8 @@ def model_events_def(): models = [ - (model_neuron_def, "model_neuron", ["v0", "I0"]), - (model_events_def, "model_events", ["k1", "k2", "k3", "k4"]), + (model_neuron_def, "model_neuron_py", ["v0", "I0"]), + (model_events_def, "model_events_py", ["k1", "k2", "k3", "k4"]), ] @@ -197,6 +197,10 @@ def run_test_cases(model): solver = model.getSolver() model_name = model.getName() + # we need a different name for the model module to avoid collisions + # with the matlab-pregenerated models, but we need the old name for + # the expected results + model_name = model_name.removesuffix("_py") for case in list(expected_results[model_name].keys()): if case.startswith("sensi2"): @@ -210,7 +214,7 @@ def run_test_cases(model): ) edata = None - if "data" in expected_results[model.getName()][case].keys(): + if "data" in expected_results[model_name][case].keys(): edata = amici.readSimulationExpData( str(expected_results_file), f"/{model_name}/{case}/data", @@ -226,6 +230,6 @@ def run_test_cases(model): verify_simulation_results( rdata, - expected_results[model.getName()][case]["results"], + expected_results[model_name][case]["results"], **verify_simulation_opts, ) diff --git a/python/tests/test_sbml_import.py b/python/tests/test_sbml_import.py index 4936a3c901..7a3f0a2720 100644 --- a/python/tests/test_sbml_import.py +++ b/python/tests/test_sbml_import.py @@ -2,6 +2,7 @@ import os import re +import sys from numbers import Number from pathlib import Path @@ -15,13 +16,13 @@ from amici.testing import skip_on_valgrind from numpy.testing import assert_allclose, assert_array_equal -EXAMPLES_DIR = Path(__file__).parent / ".." / "examples" +from conftest import EXAMPLES_DIR + STEADYSTATE_MODEL_FILE = ( EXAMPLES_DIR / "example_steadystate" / "model_steadystate_scaled.xml" ) -@pytest.fixture def simple_sbml_model(): """Some testmodel""" document = libsbml.SBMLDocument(3, 1) @@ -44,9 +45,9 @@ def simple_sbml_model(): return document, model -def test_sbml2amici_no_observables(simple_sbml_model): +def test_sbml2amici_no_observables(): """Test model generation works for model without observables""" - sbml_doc, sbml_model = simple_sbml_model + sbml_doc, sbml_model = simple_sbml_model() sbml_importer = SbmlImporter(sbml_source=sbml_model, from_file=False) model_name = "test_sbml2amici_no_observables" with TemporaryDirectory() as tmpdir: @@ -63,9 +64,9 @@ def test_sbml2amici_no_observables(simple_sbml_model): @skip_on_valgrind -def test_sbml2amici_nested_observables_fail(simple_sbml_model): +def test_sbml2amici_nested_observables_fail(): """Test model generation works for model without observables""" - sbml_doc, sbml_model = simple_sbml_model + sbml_doc, sbml_model = simple_sbml_model() sbml_importer = SbmlImporter(sbml_source=sbml_model, from_file=False) model_name = "test_sbml2amici_nested_observables_fail" with TemporaryDirectory() as tmpdir: @@ -83,8 +84,8 @@ def test_sbml2amici_nested_observables_fail(simple_sbml_model): ) -def test_nosensi(simple_sbml_model): - sbml_doc, sbml_model = simple_sbml_model +def test_nosensi(): + sbml_doc, sbml_model = simple_sbml_model() sbml_importer = SbmlImporter(sbml_source=sbml_model, from_file=False) model_name = "test_nosensi" with TemporaryDirectory() as tmpdir: @@ -109,9 +110,9 @@ def test_nosensi(simple_sbml_model): assert rdata.status == amici.AMICI_ERROR -@pytest.fixture -def observable_dependent_error_model(simple_sbml_model): - sbml_doc, sbml_model = simple_sbml_model +@pytest.fixture(scope="session") +def observable_dependent_error_model(): + sbml_doc, sbml_model = simple_sbml_model() # add parameter and rate rule sbml_model.getSpecies("S1").setInitialConcentration(1.0) sbml_model.getParameter("p1").setValue(0.2) @@ -229,21 +230,6 @@ def model_steadystate_module(): ) -@pytest.fixture(scope="session") -def model_units_module(): - sbml_file = EXAMPLES_DIR / "example_units" / "model_units.xml" - module_name = "test_model_units" - - sbml_importer = amici.SbmlImporter(sbml_file) - - with TemporaryDirectory() as outdir: - sbml_importer.sbml2amici(model_name=module_name, output_dir=outdir) - - yield amici.import_model_module( - module_name=module_name, module_path=outdir - ) - - def test_presimulation(sbml_example_presimulation_module): """Test 'presimulation' test model""" model = sbml_example_presimulation_module.getModel() @@ -521,6 +507,7 @@ def test_likelihoods_error(): @skip_on_valgrind +@pytest.mark.usefixtures("model_units_module") def test_units(model_units_module): """ Test whether SBML import works for models using sbml:units annotations. @@ -694,9 +681,9 @@ def test_code_gen_uses_lhs_symbol_ids(): @skip_on_valgrind -def test_hardcode_parameters(simple_sbml_model): +def test_hardcode_parameters(): """Test model generation works for model without observables""" - sbml_doc, sbml_model = simple_sbml_model + sbml_doc, sbml_model = simple_sbml_model() sbml_importer = SbmlImporter(sbml_source=sbml_model, from_file=False) r = sbml_model.createRateRule() r.setVariable("S1") @@ -773,3 +760,56 @@ def test_constraints(): amici_solver.getAbsoluteTolerance(), ) ) + + +@skip_on_valgrind +def test_same_extension_error(): + """Test for error when loading a model with the same extension name as an + already loaded model.""" + from amici.antimony_import import antimony2amici + + ant_model_1 = """ + model test_same_extension_error + species A = 0 + p = 1 + A' = p + end + """ + ant_model_2 = ant_model_1.replace("1", "2") + + module_name = "test_same_extension" + with TemporaryDirectory(prefix=module_name, delete=False) as outdir: + antimony2amici( + ant_model_1, + model_name=module_name, + output_dir=outdir, + compute_conservation_laws=False, + ) + model_module_1 = amici.import_model_module( + module_name=module_name, module_path=outdir + ) + assert model_module_1.get_model().getParameters()[0] == 1.0 + # no error if the same model is loaded again without changes on disk + model_module_1 = amici.import_model_module( + module_name=module_name, module_path=outdir + ) + assert model_module_1.get_model().getParameters()[0] == 1.0 + + # Try to import another model with the same name + + # On Windows, this will give "permission denied" when building the + # extension + if sys.platform == "win32": + return + + antimony2amici( + ant_model_2, + model_name=module_name, + output_dir=outdir, + compute_conservation_laws=False, + ) + with pytest.raises(RuntimeError, match="has already been imported"): + amici.import_model_module( + module_name=module_name, module_path=outdir + ) + assert model_module_1.get_model().getParameters()[0] == 1.0 diff --git a/swig/modelname.template.i b/swig/modelname.template.i index 69015dc793..d7aab8ed8a 100644 --- a/swig/modelname.template.i +++ b/swig/modelname.template.i @@ -9,6 +9,23 @@ using namespace amici; %} +// store the time a module was imported +%{ +#include +static std::chrono::time_point _module_import_time; + +static double _get_import_time() { + auto epoch = _module_import_time.time_since_epoch(); + return std::chrono::duration(epoch).count(); +} +%} + +static double _get_import_time(); + +%init %{ + _module_import_time = std::chrono::system_clock::now(); +%} + // Make model module accessible from the model %feature("pythonappend") amici::generic_model::getModel %{