Skip additional tests under valgrind (#1863)

Valgrind runs are taking too long again.

* Let's skip some more tests under valgrind. (`python/tests/test_events.py::test_models[events_plus_heavisides]` alone takes ~1.5h)
* Split Valgrind C++ and Python tests into 2 parallel jobs
* Clean up running-under-valgrind test logic, to also work without GHA or setting extra env vars

Valgrind jobs now finish in about 30min instead of 3h.

.github/workflows/test_valgrind.yml

@@ -1,4 +1,4 @@
-name: C++ Tests
+name: Valgrind tests
 on:
   push:
     branches:
@@ -12,15 +12,15 @@ on:
     - cron:  '48 4 * * *'
 
 jobs:
-  valgrind:
-    name: Tests Valgrind
+  valgrind_cpp:
+    name: Valgrind C++
 
     # TODO: prepare image with more deps preinstalled
     runs-on: ubuntu-22.04
 
     strategy:
       matrix:
-        python-version: [ "3.8" ]
+        python-version: [ "3.9" ]
 
     env:
       ENABLE_AMICI_DEBUGGING: "TRUE"
@@ -57,6 +57,46 @@ jobs:
       run: |
         scripts/run-valgrind-cpp.sh
 
+  valgrind_python:
+    name: Valgrind Python
+
+    runs-on: ubuntu-22.04
+
+    strategy:
+      matrix:
+        python-version: [ "3.9" ]
+
+    env:
+      ENABLE_AMICI_DEBUGGING: "TRUE"
+
+    steps:
+    - name: Set up Python ${{ matrix.python-version }}
+      uses: actions/setup-python@v2
+      with:
+        python-version: ${{ matrix.python-version }}
+
+    - uses: actions/checkout@master
+    - run: git fetch --prune --unshallow
+
+    # install amici dependencies
+    - name: apt
+      run: |
+        sudo apt-get update \
+          && sudo apt-get install -y \
+            cmake \
+            g++ \
+            libatlas-base-dev \
+            libboost-serialization-dev \
+            libhdf5-serial-dev \
+            python3-venv \
+            swig \
+            valgrind \
+            libboost-math-dev
+
+    - name: Build AMICI
+      run: |
+        scripts/buildAll.sh
+
     - name: Install python package
       run: |
         scripts/installAmiciSource.sh

pytest.ini

@@ -1,6 +1,6 @@
 [pytest]
 
-addopts = -vv
+addopts = -vv --strict-markers
 
 filterwarnings =
     # hundreds of SBML <=5.17 warnings

python/amici/testing.py

@@ -1,8 +1,25 @@
 """Test support functions"""
-
+import os
 import sys
 from tempfile import TemporaryDirectory
 
+import pytest
+
+# Indicates whether we are currently running under valgrind
+#  see also https://stackoverflow.com/a/62364698
+ON_VALGRIND = any(
+    needle in haystack
+    for needle in ('valgrind', 'vgpreload')
+    for haystack in (os.getenv("LD_PRELOAD", ""),
+                     os.getenv("DYLD_INSERT_LIBRARIES", ""))
+)
+
+# Decorator to skip certain tests when we are under valgrind
+#  (those that are independent of the AMICI C++ parts, or that take too long,
+#  or that test performance)
+skip_on_valgrind = pytest.mark.skipif(
+    ON_VALGRIND, reason="Takes too long or is meaningless under valgrind")
+
 
 class TemporaryDirectoryWinSafe(TemporaryDirectory):
     """TemporaryDirectory that will not raise if cleanup fails.

python/tests/test_bngl.py

@@ -9,6 +9,8 @@
 from amici.bngl_import import bngl2amici
 from pysb.simulator import ScipyOdeSimulator
 from pysb.importers.bngl import model_from_bngl
+from amici.testing import skip_on_valgrind
+
 
 tests = [
     'CaOscillate_Func', 'deleteMolecules', 'empty_compartments_block',
@@ -21,8 +23,7 @@
 ]
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Takes too long under valgrind")
+@skip_on_valgrind
 @pytest.mark.parametrize('example', tests)
 def test_compare_to_pysb_simulation(example):
 

python/tests/test_conserved_quantities_demartino.py

@@ -12,6 +12,8 @@
     compute_moiety_conservation_laws
 )
 from amici.logging import get_logger, log_execution_time
+from amici.testing import skip_on_valgrind
+
 
 logger = get_logger(__name__)
 
@@ -58,8 +60,7 @@ def data_demartino2014():
     return S, row_names
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Python-only")
+@skip_on_valgrind
 def test_kernel_demartino2014(data_demartino2014, quiet=True):
     """Invoke test case and benchmarking for De Martino's published results
     for E. coli network. Kernel-only."""
@@ -96,8 +97,7 @@ def test_kernel_demartino2014(data_demartino2014, quiet=True):
             f"Moiety #{i + 1} failed for test case (De Martino et al.)"
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Python-only")
+@skip_on_valgrind
 def test_fill_demartino2014(data_demartino2014):
     """Test creation of interaction matrix"""
     stoichiometric_list, row_names = data_demartino2014
@@ -190,8 +190,7 @@ def test_fill_demartino2014(data_demartino2014):
     assert not any(fields[len(ref_for_fields):])
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Python-only")
+@skip_on_valgrind
 def test_compute_moiety_conservation_laws_demartino2014(
         data_demartino2014, quiet=False
 ):
@@ -217,9 +216,7 @@ def test_compute_moiety_conservation_laws_demartino2014(
     return runtime
 
 
-@pytest.mark.skipif(
-    os.environ.get('GITHUB_JOB') == 'valgrind',
-    reason="Performance test under valgrind is not meaningful.")
+@skip_on_valgrind
 @log_execution_time("Detecting moiety conservation laws", logger)
 def test_cl_detect_execution_time(data_demartino2014):
     """Test execution time stays within a certain predefined bound.
@@ -239,8 +236,7 @@ def test_cl_detect_execution_time(data_demartino2014):
     assert runtime < max_time_seconds, "Took too long"
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Python-only")
+@skip_on_valgrind
 def test_compute_moiety_conservation_laws_simple():
     """Test a simple example, ensure the conservation laws are identified
      reliably. Requires the Monte Carlo to identify all."""

python/tests/test_conserved_quantities_rref.py

@@ -5,6 +5,7 @@
 import sympy as sp
 
 from amici.conserved_quantities_rref import nullspace_by_rref, pivots, rref
+from amici.testing import skip_on_valgrind
 
 
 def random_matrix_generator(min_dim, max_dim, count):
@@ -14,8 +15,7 @@ def random_matrix_generator(min_dim, max_dim, count):
         yield np.random.rand(rows, cols)
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Python-only")
+@skip_on_valgrind
 @pytest.mark.parametrize("mat", random_matrix_generator(0, 10, 200))
 def test_rref(mat):
     """Create some random matrices and compare output of ``rref`` and
@@ -28,8 +28,7 @@ def test_rref(mat):
     assert np.allclose(expected_rref, actual_rref)
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Python-only")
+@skip_on_valgrind
 @pytest.mark.parametrize("mat", random_matrix_generator(0, 50, 50))
 def test_nullspace_by_rref(mat):
     """Test ``nullspace_by_rref`` on a number of random matrices and compare

python/tests/test_edata.py

@@ -2,10 +2,11 @@
 import numpy as np
 
 import amici
+from amici.testing import skip_on_valgrind
 
 from test_sbml_import import model_units_module
 
-
+@skip_on_valgrind
 def test_edata_sensi_unscaling(model_units_module):
     """
     ExpData parameters should be used for unscaling initial state

python/tests/test_events.py

@@ -7,10 +7,11 @@
 from util import (check_trajectories_with_forward_sensitivities,
                   check_trajectories_without_sensitivities, create_amici_model,
                   create_sbml_model)
+from amici.testing import skip_on_valgrind
 
 
 @pytest.fixture(params=[
-    'events_plus_heavisides',
+    pytest.param('events_plus_heavisides', marks=skip_on_valgrind),
     'nested_events',
 ])
 def model(request):
@@ -214,7 +215,6 @@ def sx_pected(t, parameters):
     )
 
 
-
 def model_definition_nested_events():
     """Test model for state- and parameter-dependent heavisides.
 

python/tests/test_misc.py

@@ -3,14 +3,13 @@
 import os
 import subprocess
 
-import libsbml
 import pytest
 import sympy as sp
 
 import amici
 from amici.ode_export import _custom_pow_eval_derivative, _monkeypatched, \
     smart_subs_dict
-from amici.testing import TemporaryDirectoryWinSafe as TemporaryDirectory
+from amici.testing import skip_on_valgrind
 
 
 def test_parameter_scaling_from_int_vector():
@@ -27,7 +26,7 @@ def test_parameter_scaling_from_int_vector():
     assert scale_vector[1] == amici.ParameterScaling.ln
     assert scale_vector[2] == amici.ParameterScaling.none
 
-
+@skip_on_valgrind
 def test_hill_function_dwdx():
     """Kinetic laws with Hill functions, may lead to NaNs in the Jacobian
     if involved states are zero if not properly arranged symbolically.
@@ -49,6 +48,7 @@ def test_hill_function_dwdx():
     _ = str(res)
 
 
+@skip_on_valgrind
 @pytest.mark.skipif(os.environ.get('AMICI_SKIP_CMAKE_TESTS', '') == 'TRUE',
                     reason='skipping cmake based test')
 def test_cmake_compilation(sbml_example_presimulation_module):
@@ -64,6 +64,7 @@ def test_cmake_compilation(sbml_example_presimulation_module):
                    stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
 
+@skip_on_valgrind
 def test_smart_subs_dict():
     expr_str = 'c + d'
     subs_dict = {
@@ -85,6 +86,7 @@ def test_smart_subs_dict():
     assert sp.simplify(result_reverse - expected_reverse).is_zero
 
 
+@skip_on_valgrind
 def test_monkeypatch():
     t = sp.Symbol('t')
     n = sp.Symbol('n')

python/tests/test_ode_export.py

@@ -2,8 +2,9 @@
 
 import sympy as sp
 from amici.cxxcodeprinter import AmiciCxxCodePrinter
+from amici.testing import skip_on_valgrind
 
-
+@skip_on_valgrind
 def test_csc_matrix():
     """Test sparse CSC matrix creation"""
     printer = AmiciCxxCodePrinter()
@@ -19,6 +20,7 @@ def test_csc_matrix():
     assert str(sparse_matrix) == 'Matrix([[da1_db1, 0], [da2_db1, da2_db2]])'
 
 
+@skip_on_valgrind
 def test_csc_matrix_empty():
     """Test sparse CSC matrix creation for empty matrix"""
     printer = AmiciCxxCodePrinter()
@@ -33,6 +35,7 @@ def test_csc_matrix_empty():
     assert str(sparse_matrix) == 'Matrix(0, 0, [])'
 
 
+@skip_on_valgrind
 def test_csc_matrix_vector():
     """Test sparse CSC matrix creation from matrix slice"""
     printer = AmiciCxxCodePrinter()

python/tests/test_parameter_mapping.py

@@ -5,10 +5,10 @@
 
 from amici.parameter_mapping import (ParameterMapping,
                                      ParameterMappingForCondition)
+from amici.testing import skip_on_valgrind
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Python-only")
+@skip_on_valgrind
 def test_parameter_mapping_for_condition_default_args():
     """Check we can initialize the mapping with default arguments."""
 
@@ -37,8 +37,7 @@ def test_parameter_mapping_for_condition_default_args():
         expected_scale_map_sim_fix
 
 
-@pytest.mark.skipif(os.environ.get('GITHUB_JOB') == 'valgrind',
-                    reason="Python-only")
+@skip_on_valgrind
 def test_parameter_mapping():
     """Test :class:``amici.parameter_mapping.ParameterMapping``."""
 

python/tests/test_petab_import.py

@@ -3,6 +3,8 @@
 import libsbml
 import pytest
 import pandas as pd
+from amici.testing import skip_on_valgrind
+
 
 petab = pytest.importorskip("petab")
 SbmlModel = pytest.importorskip("petab.models.sbml_model.SbmlModel")
@@ -35,6 +37,7 @@ def simple_sbml_model():
     return document, model
 
 
+@skip_on_valgrind
 def test_get_fixed_parameters(simple_sbml_model):
     """Check for correct identification of fixed parameters:
 

python/tests/test_petab_simulate.py

@@ -1,12 +1,12 @@
 """Tests for petab_simulate.py."""
-
 from pathlib import Path
 import pytest
 import tempfile
 
 from amici.petab_simulate import PetabSimulator
 import petab
 import petabtests
+from amici.testing import skip_on_valgrind
 
 
 @pytest.fixture
@@ -18,6 +18,7 @@ def petab_problem() -> petab.Problem:
     return petab.Problem.from_yaml(str(petab_yaml_path))
 
 
+@skip_on_valgrind
 def test_simulate_without_noise(petab_problem):
     """Test the reproducibility of simulation without noise."""
     simulator = PetabSimulator(petab_problem)
@@ -37,6 +38,7 @@ def test_simulate_without_noise(petab_problem):
     assert synthetic_data_df_c.equals(synthetic_data_df_a)
 
 
+@skip_on_valgrind
 def test_subset_call(petab_problem):
     """
     Test the ability to customize AMICI methods, specifically: