Repository: gammasim/simtools
Issue category: Performance Issue

Issue:
test_get_corsika_telescope_list takes too long
The `test_get_corsika_telescope_list` test takes roughly 20 seconds to run. Profiling it points to excessive time spent initializing the array model, perhaps related to reading from the DB too many times.

```
  ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000   21.427   21.427 /workdir/external/simtools/simtools/model/array_model.py:41(__init__)
        1    0.000    0.000   20.855   20.855 /workdir/external/simtools/simtools/model/array_model.py:69(_initialize)
       60    0.004    0.000   20.773    0.346 /workdir/external/simtools/simtools/model/model_parameter.py:49(__init__)
       58    0.002    0.000   19.823    0.342 /workdir/external/simtools/simtools/model/telescope_model.py:38(__init__)
        1    0.000    0.000    0.994    0.994 /workdir/external/simtools/simtools/model/site_model.py:27(__init__)
       61    0.001    0.000    0.574    0.009 /workdir/external/simtools/simtools/db/db_handler.py:57(__init__)
```

Need to look into it further (in the middle of something else, opening this issue so it is not forgotten). 


Hints:
The `array_model` consist of the site model, the model of all telescopes and of all calibration devices. So this is naturally that all DB calls are initiated from the array_model_class.

The number of calls to the DB are reduced by having the `DatabaseHandler.*cached` dicts, where the idea is to not query the same parameter twice.

Obviously always good to check that this is still working.  
The main difference to the old style is that now we call the DB for each telescope separately. In the past we read all of the parameters of all of the telescopes in one query (which is very quick). I suggest to extend the caching functionality to do that as well with the new style. Usually the model parameter version does not change from one telescope to the next (at least for now), so when the first telescope is read, we can fill the cache with the parameters of all telescopes for that same version. The should reduce the DB calls to one in most cases.
I can confirm that when running at DESY this test takes 1 second, while running it at home takes 15-20 seconds. 
On GitHub this test is also the slowest at ~50 seconds (see e.g., [this job](https://github.com/gammasim/simtools/actions/runs/10700964938/job/29665863400?pr=1127)). I think that adding another cache level is therefore worth it since it will shorten the running time of the GitHub tests. It's far from urgent though, considering it is just a minute in the worst case scenario.
Started to have a look:

- `test_get_corsika_telescope_list` is the only test for corsika_config.py which actually accesses the database (all other tests are using the fixture `corsika_config_no_db`; I actually introduce at some point a `@pytest.mark.uses_model_database()` marker)
- so no wonder that this is the slowest function in these tests...

Given that we want to test the code and not the database connection, my suggestion is to change also this function and either use the `corsika_config_no_db` or some mocking (aligned with the suggestion in issue #1054). OK?

Additionally:

- we should think about dedicated benchmark tests on accessing the DB for different usage scenarios
- suggest that this would be an integration test and not a unit test, as it tests several modules 
I find the comments/suggestions a little bit contradictory. I will write comments below. They are out of order, but I think follow some logic still.

> - suggest that this would be an integration test and not a unit test, as it tests several modules

If we want to change it to be an integration test, then I wouldn't use mocking but actually connect to the DB as expected in integration tests and as we do in others.

> - we should think about dedicated benchmark tests on accessing the DB for different usage scenarios

This test actually accomplished that in a way. It showed that we are accessing the DB for each telescope separately, which is something we can probably easily avoid. I agree that having a dedicated test for accessing the DB is good though, let's try to come up with it after we decide on the structure of the DB.
 
> Given that we want to test the code and not the database connection, my suggestion is to change also this function and either use the corsika_config_no_db or some mocking (aligned with the suggestion in issue https://github.com/gammasim/simtools/issues/1054). OK?

It's fine to change the unit test to use mocking, but then I would add a separate integration test that does the same, actually using the DB. I think it's important to test this in a real scenario, considering the chain of "events" this test triggers (even though it is just one command).

Oh and I would wait before implementing any changes until we implement the DB restructuring. This problem might resolve itself then because we will probably access the DB once, getting the entries based on the table of production version/parameter versions.
Yes and no:

- independent of any restructuring should we avoid DB calls in unit tests
- we need a test for the DB connections (but not a unit test)

I think both points are not affected by restructuring.
Yes, sorry, I didn't mean that those points would not be valid anymore, just that this specific issue of many unnecessary calls would (hopefully) be resolved. I don't think we should add another cache at the moment (like I proposed above) because it will probably be superfluous once we implement the restructuring.

Patch:
diff --git a/simtools/applications/db_get_parameter_from_db.py b/simtools/applications/db_get_parameter_from_db.py
index 259f7689f..ff4f407fd 100644
--- a/simtools/applications/db_get_parameter_from_db.py
+++ b/simtools/applications/db_get_parameter_from_db.py
@@ -85,8 +85,11 @@ def main():  # noqa: D103
     db = db_handler.DatabaseHandler(mongo_db_config=db_config)
 
     if args_dict["db_collection"] == "configuration_sim_telarray":
-        pars = db.get_sim_telarray_configuration_parameters(
-            args_dict["site"], args_dict["telescope"], args_dict["model_version"]
+        pars = db.get_model_parameters(
+            args_dict["site"],
+            args_dict["telescope"],
+            args_dict["model_version"],
+            collection="configuration_sim_telarray",
         )
     elif args_dict["db_collection"] == "configuration_corsika":
         pars = db.get_corsika_configuration_parameters(args_dict["model_version"])
diff --git a/simtools/db/db_array_elements.py b/simtools/db/db_array_elements.py
index b0e4f961d..cb5d70ab5 100644
--- a/simtools/db/db_array_elements.py
+++ b/simtools/db/db_array_elements.py
@@ -1,12 +1,14 @@
 """Retrieval of array elements from the database."""
 
-from functools import cache
+from functools import lru_cache
 
 from pymongo import ASCENDING
 
+from simtools.utils import names
 
-@cache
-def get_array_elements(db, model_version, collection):
+
+@lru_cache
+def get_array_elements(db_collection, model_version):
     """
     Get all array element names and their design model for a given DB collection.
 
@@ -15,12 +17,10 @@ def get_array_elements(db, model_version, collection):
 
     Parameters
     ----------
-    db: DBHandler
-        Instance of the database handler
+    db_collection:
+        pymongo.collection.Collection
     model_version: str
        Model version.
-    collection: str
-        Database collection (e.g., telescopes, calibration_devices)
 
     Returns
     -------
@@ -35,9 +35,7 @@ def get_array_elements(db, model_version, collection):
         If array element entry in the database is incomplete.
 
     """
-    db_collection = db.get_collection(db_name=None, collection_name=collection)
-
-    query = {"version": db.model_version(model_version)}
+    query = {"version": model_version}
     results = db_collection.find(query, {"instrument": 1, "value": 1, "parameter": 1}).sort(
         "instrument", ASCENDING
     )
@@ -51,7 +49,7 @@ def get_array_elements(db, model_version, collection):
             raise KeyError(f"Incomplete array element entry in the database: {doc}.") from exc
 
     if len(_all_available_array_elements) == 0:
-        raise ValueError(f"No array elements found in DB collection {collection}.")
+        raise ValueError(f"No array elements found in DB collection {db_collection}.")
 
     return _all_available_array_elements
 
@@ -62,6 +60,7 @@ def get_array_element_list_for_db_query(array_element_name, db, model_version, c
 
     Return a list of array element names to be used for querying the database for a given array
     element. This is in most cases the array element itself and its design model.
+    In cases of no design model available, the design model of the array element is returned.
 
     Parameters
     ----------
@@ -80,7 +79,13 @@ def get_array_element_list_for_db_query(array_element_name, db, model_version, c
         List of array element model names as used in the DB.
 
     """
-    _available_array_elements = get_array_elements(db, model_version, collection)
+    try:
+        _available_array_elements = get_array_elements(
+            db.get_collection(db_name=None, collection_name=collection),
+            db.model_version(model_version),
+        )
+    except ValueError:
+        return [names.get_array_element_type_from_name(array_element_name) + "-design"]
     try:
         return [_available_array_elements[array_element_name], array_element_name]
     except KeyError:
@@ -116,7 +121,10 @@ def get_array_elements_of_type(array_element_type, db, model_version, collection
         Sorted list of all array element names found in collection
 
     """
-    _available_array_elements = get_array_elements(db, model_version, collection)
+    _available_array_elements = get_array_elements(
+        db.get_collection(db_name=None, collection_name=collection),
+        db.model_version(model_version),
+    )
     return sorted(
         [entry for entry in _available_array_elements if entry.startswith(array_element_type)]
     )
diff --git a/simtools/db/db_handler.py b/simtools/db/db_handler.py
index 0fa59e2f6..30a9e219d 100644
--- a/simtools/db/db_handler.py
+++ b/simtools/db/db_handler.py
@@ -50,7 +50,6 @@ class DatabaseHandler:
     site_parameters_cached = {}
     model_parameters_cached = {}
     model_versions_cached = {}
-    sim_telarray_configuration_parameters_cached = {}
     corsika_configuration_parameters_cached = {}
 
     def __init__(self, mongo_db_config=None):
@@ -192,7 +191,7 @@ def get_model_parameters(
         pars = {}
         for array_element in array_element_list:
             _array_elements_cache_key = self._parameter_cache_key(
-                site, array_element, model_version
+                site, array_element, model_version, collection
             )
             try:
                 pars.update(DatabaseHandler.model_parameters_cached[_array_elements_cache_key])
@@ -536,8 +535,8 @@ def get_simulation_configuration_parameters(
             return self.get_corsika_configuration_parameters(model_version)
         if simulation_software == "simtel":
             if site and array_element_name:
-                return self.get_sim_telarray_configuration_parameters(
-                    site, array_element_name, model_version
+                return self.get_model_parameters(
+                    site, array_element_name, model_version, collection="configuration_sim_telarray"
                 )
             return {}
         raise ValueError(f"Unknown simulation software: {simulation_software}")
@@ -573,60 +572,6 @@ def get_corsika_configuration_parameters(self, model_version):
         )
         return DatabaseHandler.corsika_configuration_parameters_cached[_corsika_cache_key]
 
-    def get_sim_telarray_configuration_parameters(self, site, array_element_name, model_version):
-        """
-        Get sim_telarray configuration parameters from the DB for a specific array element.
-
-        Parameters
-        ----------
-        site : str
-            Site name.
-        array_element_name : str
-            Name of the array element model (e.g. MSTN).
-        model_version : str
-            Version of the model.
-
-        Returns
-        -------
-        dict
-            Configuration parameters for sim_telarray
-        """
-        _, _array_element_name, _model_version = self._validate_model_input(
-            site, array_element_name, model_version
-        )
-        _array_elements_cache_key = self._parameter_cache_key(
-            site, array_element_name, model_version
-        )
-        try:
-            return DatabaseHandler.sim_telarray_configuration_parameters_cached[
-                _array_elements_cache_key
-            ]
-        except KeyError:
-            pass
-        pars = {}
-        try:
-            pars = self.read_mongo_db(
-                self._get_db_name(),
-                _array_element_name,
-                _model_version,
-                run_location=None,
-                collection_name="configuration_sim_telarray",
-                write_files=False,
-            )
-        except ValueError:
-            pars = self.read_mongo_db(
-                self._get_db_name(),
-                names.get_array_element_type_from_name(_array_element_name) + "-design",
-                _model_version,
-                run_location=None,
-                collection_name="configuration_sim_telarray",
-                write_files=False,
-            )
-        DatabaseHandler.sim_telarray_configuration_parameters_cached[_array_elements_cache_key] = (
-            pars
-        )
-        return pars
-
     def _validate_model_input(self, site, array_element_name, model_version):
         """
         Validate input for model parameter queries.
@@ -1221,7 +1166,7 @@ def get_all_versions(
 
         return DatabaseHandler.model_versions_cached[_cache_key]
 
-    def _parameter_cache_key(self, site, array_element_name, model_version):
+    def _parameter_cache_key(self, site, array_element_name, model_version, collection=None):
         """
         Create a cache key for the parameter cache dictionaries.
 
@@ -1233,6 +1178,8 @@ def _parameter_cache_key(self, site, array_element_name, model_version):
             Array element name.
         model_version: str
             Model version.
+        collection: str
+            DB collection name.
 
         Returns
         -------
@@ -1245,6 +1192,8 @@ def _parameter_cache_key(self, site, array_element_name, model_version):
         if array_element_name:
             parts.append(array_element_name)
         parts.append(model_version)
+        if collection:
+            parts.append(collection)
         return "-".join(parts)
 
     def _reset_parameter_cache(self, site, array_element_name, model_version):


Test patch:
diff --git a/tests/unit_tests/db/test_db_array_elements.py b/tests/unit_tests/db/test_db_array_elements.py
index 2a1bc5570..dcc693abf 100644
--- a/tests/unit_tests/db/test_db_array_elements.py
+++ b/tests/unit_tests/db/test_db_array_elements.py
@@ -1,14 +1,28 @@
 #!/usr/bin/python3
 
+import time
+
 import pytest
 
 from simtools.db import db_array_elements
 
 
 def test_get_array_elements(db, model_version):
+
+    time_1 = time.time()
+    db_array_elements.get_array_elements(
+        db.get_collection(db_name=None, collection_name="telescopes"),
+        db.model_version(model_version),
+    )
+    time_2 = time.time()
     available_telescopes = db_array_elements.get_array_elements(
-        db=db, model_version=model_version, collection="telescopes"
+        db.get_collection(db_name=None, collection_name="telescopes"),
+        db.model_version(model_version),
     )
+    time_3 = time.time()
+
+    # check that the second call is much faster than the first one
+    assert (time_2 - time_1) > 0.1 * (time_3 - time_2)
 
     expected_telescope_names = {
         "LSTN-01": "LSTN-design",
@@ -24,7 +38,8 @@ def test_get_array_elements(db, model_version):
         assert expected_telescope_names[_t] in available_telescopes[_t]
 
     available_calibration_devices = db_array_elements.get_array_elements(
-        db=db, model_version=model_version, collection="calibration_devices"
+        db.get_collection(db_name=None, collection_name="calibration_devices"),
+        db.model_version(model_version),
     )
     expected_calibration_devices = {
         "ILLN-01": "ILLN-design",
@@ -34,11 +49,10 @@ def test_get_array_elements(db, model_version):
         assert _d in available_calibration_devices
         assert expected_calibration_devices[_d] in available_calibration_devices[_d]
 
-    with pytest.raises(
-        ValueError, match=r"^No array elements found in DB collection wrong_collection."
-    ):
+    with pytest.raises(ValueError, match=r"^No array elements found in DB collection"):
         db_array_elements.get_array_elements(
-            db=db, model_version=model_version, collection="wrong_collection"
+            db.get_collection(db_name=None, collection_name="wrong_collection"),
+            db.model_version(model_version),
         )
 
 
@@ -65,6 +79,10 @@ def test_get_array_element_list_for_db_query(db, model_version):
             "MSTS-301", db=db, model_version=model_version, collection="calibration_devices"
         )
 
+    assert db_array_elements.get_array_element_list_for_db_query(
+        "LSTN-02", db=db, model_version=model_version, collection="configuration_sim_telarray"
+    ) == ["LSTN-design"]
+
 
 def test_get_array_elements_of_type(db, model_version):
     available_telescopes = db_array_elements.get_array_elements_of_type(
diff --git a/tests/unit_tests/db/test_db_handler.py b/tests/unit_tests/db/test_db_handler.py
index 84571ced0..3e4a9351f 100644
--- a/tests/unit_tests/db/test_db_handler.py
+++ b/tests/unit_tests/db/test_db_handler.py
@@ -147,10 +147,14 @@ def test_get_derived_values(db, model_version_prod5):
 
 def test_get_sim_telarray_configuration_parameters(db, model_version):
 
-    _pars = db.get_sim_telarray_configuration_parameters("North", "LSTN-01", model_version)
+    _pars = db.get_model_parameters(
+        "North", "LSTN-01", model_version, collection="configuration_sim_telarray"
+    )
     assert "min_photoelectrons" in _pars
 
-    _pars = db.get_sim_telarray_configuration_parameters("North", "LSTN-design", model_version)
+    _pars = db.get_model_parameters(
+        "North", "LSTN-design", model_version, collection="configuration_sim_telarray"
+    )
     assert "min_photoelectrons" in _pars
 
 
diff --git a/tests/unit_tests/model/test_model_parameter.py b/tests/unit_tests/model/test_model_parameter.py
index 446b55fb1..0f18682ab 100644
--- a/tests/unit_tests/model/test_model_parameter.py
+++ b/tests/unit_tests/model/test_model_parameter.py
@@ -146,11 +146,11 @@ def test_load_parameters_from_db(telescope_model_lst, mocker):
     telescope_copy = copy.deepcopy(telescope_model_lst)
     mock_db = mocker.patch.object(DatabaseHandler, "get_model_parameters")
     telescope_copy._load_parameters_from_db()
-    mock_db.assert_called_once()
+    assert mock_db.call_count == 2
 
     telescope_copy.db = None
     telescope_copy._load_parameters_from_db()
-    not mock_db.assert_called_once()
+    assert mock_db.call_count == 2
 
 
 def test_extra_labels(telescope_model_lst):