Raise during unsupported model imports (#2596)

We can't import multiple extensions with the same name. Raise during `import_model_module` if this is attempted.

Also:
* fix redundant imports
* fix some cases where the wrong model was used

Related to #1936.

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

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
+        )

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

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,
         )

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

swig/modelname.template.i

@@ -9,6 +9,23 @@
 using namespace amici;
 %}
 
+// store the time a module was imported
+%{
+#include <chrono>
+static std::chrono::time_point<std::chrono::system_clock> _module_import_time;
+
+static double _get_import_time() {
+    auto epoch = _module_import_time.time_since_epoch();
+    return std::chrono::duration<double>(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 %{