From f4e4738841477da17de4475818997a5b68ee2029 Mon Sep 17 00:00:00 2001 From: Goran Jelic-Cizmek Date: Fri, 25 Jul 2025 14:17:18 +0200 Subject: [PATCH 1/5] Rework coverage --- CMakeLists.txt | 5 ++ bin/nrnivmodl.in | 2 +- src/nmodl/pybind/CMakeLists.txt | 8 +++ src/nmodl/pybind/ode_py.hpp.inc | 4 +- src/nmodl/pybind/wrapper.cpp | 69 ++++++++++++++++++----- test/nmodl/transpiler/unit/CMakeLists.txt | 32 +++++++++-- 6 files changed, 97 insertions(+), 23 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7fe371c682..3a726b692d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1006,6 +1006,11 @@ if(NRN_ENABLE_TESTS) if(NOT PYTEST_COV_FOUND) message(STATUS "pytest-cov package not installed. Python coverage will not be generated.") endif() + set(NRN_PYTEST_LAUNCHER -m pytest --capture=tee-sys) + # pytest-cov runs extremely slowly under AddressSanitizer + if(PYTEST_COV_FOUND AND NOT "address" IN_LIST NRN_SANITIZERS_LIST) + list(APPEND NRN_PYTEST_LAUNCHER --cov-report=xml --cov=neuron) + endif() endif() add_dependencies(nrniv_lib copy_share_demo_to_build) # Execute neurondemo as part of the build because it lazily calls nrnivmodl. If we don't do this diff --git a/bin/nrnivmodl.in b/bin/nrnivmodl.in index d66908e32b..e53b5b1511 100755 --- a/bin/nrnivmodl.in +++ b/bin/nrnivmodl.in @@ -188,7 +188,7 @@ for i in "${files[@]}" ; do echo "\ ${base_name// /\\ }.cpp: ${f}.mod \$(NOCMODL) @printf \" -> \$(C_GREEN)NMODL\$(C_RESET) \$<\\\n\" - (cd \"$dir_name\"; @NRN_NOCMODL_SANITIZER_ENVIRONMENT_STRING@ MODLUNIT=\$(NRNUNITS) \$(NOCMODL) \"$base_name.mod\" @NRN_NMODL_--neuron@ -o \"$mdir\" $UserNMODLFLAGS) + @NRN_NOCMODL_SANITIZER_ENVIRONMENT_STRING@ MODLUNIT=\$(NRNUNITS) \$(NOCMODL) \"$f.mod\" @NRN_NMODL_--neuron@ -o \"$mdir\" $UserNMODLFLAGS ./${base_name// /\\ }.o: ${base_name// /\\ }.cpp @printf \" -> \$(C_GREEN)Compiling\$(C_RESET) ${PWD}/\$<\\\n\" diff --git a/src/nmodl/pybind/CMakeLists.txt b/src/nmodl/pybind/CMakeLists.txt index 9b8c4ed73d..94a10c13d1 100644 --- a/src/nmodl/pybind/CMakeLists.txt +++ b/src/nmodl/pybind/CMakeLists.txt @@ -29,6 +29,9 @@ if(WIN32) # https://developercommunity.visualstudio.com/t/c-string-literal-max-length-much-shorter-than-docu/758957 string(REGEX REPLACE "\n\n" "\n)jiowi\" R\"jiowi(\n" NMODL_ODE_PY "${NMODL_ODE_PY}") endif() +if(NRN_ENABLE_COVERAGE) + set(NMODL_ODE_PY_PATH "${NMODL_PROJECT_PURELIB_SOURCE_DIR}/ode.py") +endif() configure_file(${CMAKE_CURRENT_SOURCE_DIR}/ode_py.hpp.inc ${CMAKE_CURRENT_BINARY_DIR}/ode_py.hpp @ONLY) @@ -46,6 +49,11 @@ else() target_compile_definitions(pyembed PRIVATE NMODL_STATIC_PYWRAPPER=1) endif() +if(NRN_ENABLE_COVERAGE) + target_compile_definitions(pywrapper PRIVATE NRN_ENABLE_COVERAGE) + target_link_libraries(pywrapper PRIVATE util) +endif() + target_link_libraries(pywrapper PRIVATE fmt::fmt) target_include_directories(pyembed PRIVATE ${PYBIND11_INCLUDE_DIR} ${PYTHON_INCLUDE_DIRS}) diff --git a/src/nmodl/pybind/ode_py.hpp.inc b/src/nmodl/pybind/ode_py.hpp.inc index b6eef4e21e..e3e7371760 100644 --- a/src/nmodl/pybind/ode_py.hpp.inc +++ b/src/nmodl/pybind/ode_py.hpp.inc @@ -15,5 +15,7 @@ namespace nmodl::pybind_wrappers { const std::string ode_py = R"jiowi( @NMODL_ODE_PY@ )jiowi"; - +#ifdef NRN_ENABLE_COVERAGE +const std::string ode_py_path = "@NMODL_ODE_PY_PATH@"; +#endif } diff --git a/src/nmodl/pybind/wrapper.cpp b/src/nmodl/pybind/wrapper.cpp index d59b579d97..73382b1231 100644 --- a/src/nmodl/pybind/wrapper.cpp +++ b/src/nmodl/pybind/wrapper.cpp @@ -4,27 +4,67 @@ * * SPDX-License-Identifier: Apache-2.0 */ +#include +#include +#include +#include -#include "wrapper.hpp" - -#include "codegen/codegen_naming.hpp" -#include "pybind/pyembed.hpp" +// 3rd party headers #include -#include #include #include -#include -#include +// NMODL headers +#include "codegen/codegen_naming.hpp" +#include "pybind/ode_py.hpp" +#include "pybind/wrapper.hpp" +#include "pybind/pyembed.hpp" +#include "utils/common_utils.hpp" -#include "ode_py.hpp" +namespace fs = std::filesystem; namespace py = pybind11; using namespace py::literals; namespace nmodl { namespace pybind_wrappers { +// This wrapper is used for obtaining better coverage in `ode.py`. +// Since we embed the `ode.py` as a string, there is no way to check what was covered via running +// pytest or similar. Instead, we use the coverage.py API directly. +static void run_python_script(const std::string& script, const py::dict& locals) { +#ifdef NRN_ENABLE_COVERAGE + py::exec(fmt::format(R"( +import coverage +cov = coverage.Coverage() +cov.start() +)"), + locals); + const auto& code_with_mapping = std::string("exec(compile(r'''" + ode_py + script + "''', '" + + ode_py_path + "', 'exec'))"); + py::exec(code_with_mapping, locals); +#else + py::exec(ode_py + script, locals); +#endif + +#ifdef NRN_ENABLE_COVERAGE + // to prevent race conditions during testing, we generate a random suffix + const auto& suffix = + nmodl::utils::generate_random_string(20, nmodl::utils::UseNumbersInString::WithoutNumbers); + const auto& path = fs::current_path() / fmt::format("coverage_{}.xml", suffix); + py::exec(fmt::format(R"( +cov.stop() +cov.save() +# Check if we have any coverage data +data = cov.get_data() +if data.measured_files(): + cov.xml_report(outfile='{}') +)", + path.string()), + locals); +#endif +} + std::tuple, std::vector, std::string> call_solve_linear_system(const std::vector& eq_system, const std::vector& state_vars, @@ -57,8 +97,7 @@ except Exception as e: new_local_vars = [""] exception_message = traceback.format_exc() )"; - - py::exec(nmodl::pybind_wrappers::ode_py + script, locals); + run_python_script(script, locals); // returns a vector of solutions, i.e. new statements to add to block: auto solutions = locals["solutions"].cast>(); // and a vector of new local variables that need to be declared in the block: @@ -93,7 +132,7 @@ except Exception as e: exception_message = traceback.format_exc() )"; - py::exec(nmodl::pybind_wrappers::ode_py + script, locals); + run_python_script(script, locals); // returns a vector of solutions, i.e. new statements to add to block: auto solutions = locals["solutions"].cast>(); // may also return a python exception message: @@ -130,7 +169,7 @@ except Exception as e: exception_message = traceback.format_exc() )"; - py::exec(nmodl::pybind_wrappers::ode_py + script, locals); + run_python_script(script, locals); } else if (method == codegen::naming::CNEXP_METHOD) { // replace x' = f(x) differential equation // with analytic solution for x(t+dt) in terms of x(t) @@ -147,7 +186,7 @@ except Exception as e: exception_message = traceback.format_exc() )"; - py::exec(nmodl::pybind_wrappers::ode_py + script, locals); + run_python_script(script, locals); } else { // nothing to do, but the caller should know. return {}; @@ -179,7 +218,7 @@ except Exception as e: exception_message = traceback.format_exc() )"; - py::exec(nmodl::pybind_wrappers::ode_py + script, locals); + run_python_script(script, locals); auto solution = locals["solution"].cast(); auto exception_message = locals["exception_message"].cast(); @@ -223,7 +262,7 @@ except Exception as e: statements, property.has_value() ? fmt::format("{}[{}]", name, property.value()) : name); - py::exec(nmodl::pybind_wrappers::ode_py + script, locals); + run_python_script(script, locals); auto solution = locals["solution"].cast(); auto exception_message = locals["exception_message"].cast(); diff --git a/test/nmodl/transpiler/unit/CMakeLists.txt b/test/nmodl/transpiler/unit/CMakeLists.txt index 477223195f..dab929974b 100644 --- a/test/nmodl/transpiler/unit/CMakeLists.txt +++ b/test/nmodl/transpiler/unit/CMakeLists.txt @@ -207,8 +207,22 @@ endif() # ============================================================================= if(NRN_ENABLE_PYTHON) if(NOT (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND NRN_SANITIZERS)) - add_test(NAME Ode COMMAND ${PYTHON_EXECUTABLE} -m pytest ${CMAKE_CURRENT_SOURCE_DIR}/ode) - set_tests_properties(Ode PROPERTIES ENVIRONMENT "PYTHONPATH=${NMODL_TEST_PYTHONPATH}") + # Most of the Python tests added by `add_nrn_test` run in their own directories. Unfortunately, + # pytest-cov does not seem to have an option to specify the output file (it _always_ outputs to + # `coverage.xml`), which means that the various tests here may clobber each other's coverage + # reports. The way out of this is to a) either specify `WORKING_DIRECTORY` of each test, or b) + # use a coverage config file (see + # https://coverage.readthedocs.io/en/latest/config.html#xml-output); the latter requires far + # more work than the former, so we use option a) in the below. + file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/ode) + add_test( + NAME Ode + COMMAND ${PYTHON_EXECUTABLE} ${NRN_PYTEST_LAUNCHER} ${CMAKE_CURRENT_SOURCE_DIR}/ode + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/ode) + set_tests_properties( + Ode + PROPERTIES ENVIRONMENT + "PYTHONPATH=${NMODL_TEST_PYTHONPATH};NEURONHOME=${PROJECT_BINARY_DIR}/share/nrn") cpp_cc_configure_sanitizers(TEST Ode PRELOAD) endif() @@ -216,11 +230,17 @@ if(NRN_ENABLE_PYTHON) # Apple Clang and ASAN do not play along nicely with NMODL's Python bindings, so we skip these # tests if(NOT (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND NRN_SANITIZERS)) - add_test(NAME Pybind COMMAND ${PYTHON_EXECUTABLE} -m pytest - ${CMAKE_CURRENT_SOURCE_DIR}/pybind) + file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/pybind) + add_test( + NAME Pybind + COMMAND ${PYTHON_EXECUTABLE} ${NRN_PYTEST_LAUNCHER} ${CMAKE_CURRENT_SOURCE_DIR}/pybind + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/pybind) set_tests_properties( - Pybind PROPERTIES ENVIRONMENT - "PYTHONPATH=${NMODL_TEST_PYTHONPATH};NMODLHOME=${PROJECT_BINARY_DIR}") + Pybind + PROPERTIES + ENVIRONMENT + "PYTHONPATH=${NMODL_TEST_PYTHONPATH};NMODLHOME=${PROJECT_BINARY_DIR};NEURONHOME=${PROJECT_BINARY_DIR}/share/nrn" + ) cpp_cc_configure_sanitizers(TEST Pybind PRELOAD) endif() endif() From b656fc7b338cb7a2d2a22657f66af5033f7655f1 Mon Sep 17 00:00:00 2001 From: Goran Jelic-Cizmek Date: Fri, 25 Jul 2025 17:45:43 +0200 Subject: [PATCH 2/5] Use readlink -f for getting the abs path to modfile --- bin/nrnivmodl.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bin/nrnivmodl.in b/bin/nrnivmodl.in index e53b5b1511..3f2768074c 100755 --- a/bin/nrnivmodl.in +++ b/bin/nrnivmodl.in @@ -182,13 +182,14 @@ for i in "${files[@]}" ; do esac base_name="$(basename "$f")" dir_name="$(dirname "$f")" + actual_name="$(readlink -f "${f}")" # Note: indentation for shell lines in make rules must be a tab f=${f// /\\ } f=${f//:/\\:} echo "\ ${base_name// /\\ }.cpp: ${f}.mod \$(NOCMODL) @printf \" -> \$(C_GREEN)NMODL\$(C_RESET) \$<\\\n\" - @NRN_NOCMODL_SANITIZER_ENVIRONMENT_STRING@ MODLUNIT=\$(NRNUNITS) \$(NOCMODL) \"$f.mod\" @NRN_NMODL_--neuron@ -o \"$mdir\" $UserNMODLFLAGS + @NRN_NOCMODL_SANITIZER_ENVIRONMENT_STRING@ MODLUNIT=\$(NRNUNITS) \$(NOCMODL) \"${actual_name}.mod\" @NRN_NMODL_--neuron@ -o \"$mdir\" $UserNMODLFLAGS ./${base_name// /\\ }.o: ${base_name// /\\ }.cpp @printf \" -> \$(C_GREEN)Compiling\$(C_RESET) ${PWD}/\$<\\\n\" From e0e83ea869bea4aaebeb0ea77f5daa7103e6c6bc Mon Sep 17 00:00:00 2001 From: Goran Jelic-Cizmek Date: Sat, 26 Jul 2025 12:30:28 +0200 Subject: [PATCH 3/5] Okay I give up on nrnivmodl Put back whatever hackery it used before to collect the mod files, if some of the files are missing coverage I really don't care. --- bin/nrnivmodl.in | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bin/nrnivmodl.in b/bin/nrnivmodl.in index 3f2768074c..d66908e32b 100755 --- a/bin/nrnivmodl.in +++ b/bin/nrnivmodl.in @@ -182,14 +182,13 @@ for i in "${files[@]}" ; do esac base_name="$(basename "$f")" dir_name="$(dirname "$f")" - actual_name="$(readlink -f "${f}")" # Note: indentation for shell lines in make rules must be a tab f=${f// /\\ } f=${f//:/\\:} echo "\ ${base_name// /\\ }.cpp: ${f}.mod \$(NOCMODL) @printf \" -> \$(C_GREEN)NMODL\$(C_RESET) \$<\\\n\" - @NRN_NOCMODL_SANITIZER_ENVIRONMENT_STRING@ MODLUNIT=\$(NRNUNITS) \$(NOCMODL) \"${actual_name}.mod\" @NRN_NMODL_--neuron@ -o \"$mdir\" $UserNMODLFLAGS + (cd \"$dir_name\"; @NRN_NOCMODL_SANITIZER_ENVIRONMENT_STRING@ MODLUNIT=\$(NRNUNITS) \$(NOCMODL) \"$base_name.mod\" @NRN_NMODL_--neuron@ -o \"$mdir\" $UserNMODLFLAGS) ./${base_name// /\\ }.o: ${base_name// /\\ }.cpp @printf \" -> \$(C_GREEN)Compiling\$(C_RESET) ${PWD}/\$<\\\n\" From c81be96caac3712c53784a1597f56062f0747854 Mon Sep 17 00:00:00 2001 From: Goran Jelic-Cizmek Date: Mon, 28 Jul 2025 12:42:50 +0200 Subject: [PATCH 4/5] Try using a different coverage database file --- src/nmodl/pybind/wrapper.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/nmodl/pybind/wrapper.cpp b/src/nmodl/pybind/wrapper.cpp index 73382b1231..2ad95bf7c3 100644 --- a/src/nmodl/pybind/wrapper.cpp +++ b/src/nmodl/pybind/wrapper.cpp @@ -34,11 +34,16 @@ namespace pybind_wrappers { // pytest or similar. Instead, we use the coverage.py API directly. static void run_python_script(const std::string& script, const py::dict& locals) { #ifdef NRN_ENABLE_COVERAGE + // to prevent race conditions during testing, we generate a random suffix + const auto& suffix = + nmodl::utils::generate_random_string(20, nmodl::utils::UseNumbersInString::WithoutNumbers); + py::exec(fmt::format(R"( import coverage -cov = coverage.Coverage() +cov = coverage.Coverage(data_suffix='{}') cov.start() -)"), +)", + suffix), locals); const auto& code_with_mapping = std::string("exec(compile(r'''" + ode_py + script + "''', '" + ode_py_path + "', 'exec'))"); @@ -48,9 +53,6 @@ cov.start() #endif #ifdef NRN_ENABLE_COVERAGE - // to prevent race conditions during testing, we generate a random suffix - const auto& suffix = - nmodl::utils::generate_random_string(20, nmodl::utils::UseNumbersInString::WithoutNumbers); const auto& path = fs::current_path() / fmt::format("coverage_{}.xml", suffix); py::exec(fmt::format(R"( cov.stop() From d692cea3493823f964df40f4a50b2d8913546e7e Mon Sep 17 00:00:00 2001 From: Goran Jelic-Cizmek Date: Mon, 28 Jul 2025 15:42:00 +0200 Subject: [PATCH 5/5] Update gitignore also add comment about using single triple quotes --- .gitignore | 2 ++ share/lib/python/neuron/nmodl/ode.py | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/.gitignore b/.gitignore index 62594fc0bc..ac94d9959f 100644 --- a/.gitignore +++ b/.gitignore @@ -23,3 +23,5 @@ virtualenv docs/_build docs/_generated .vscode +.coverage* +coverage*.xml diff --git a/share/lib/python/neuron/nmodl/ode.py b/share/lib/python/neuron/nmodl/ode.py index dbb4155ac4..59b23175d9 100644 --- a/share/lib/python/neuron/nmodl/ode.py +++ b/share/lib/python/neuron/nmodl/ode.py @@ -5,6 +5,11 @@ # Lesser General Public License. See top-level LICENSE file for details. # *********************************************************************** +# NOTE: because we are testing this via `exec` for the purposes of obtaining +# the correct coverage, do NOT use triple single quotation marks anywhere in +# the below because the code will break (should also be enforced by the +# formatter, but better safe than sorry)! + import re from importlib import import_module