Skip to content

Refactor NMODL Python targets#3541

Open
JCGoran wants to merge 20 commits intomasterfrom
jelic/refactor_nmodl_python_targets
Open

Refactor NMODL Python targets#3541
JCGoran wants to merge 20 commits intomasterfrom
jelic/refactor_nmodl_python_targets

Conversation

@JCGoran
Copy link
Collaborator

@JCGoran JCGoran commented Jul 23, 2025

Needs #3544.

The pybind/CMakeLists.txt file is used for building two kinds of targets:

  • embedding the interpreter (which is used to run sympy) via pybind11
  • building the NMODL Python bindings via pybind11

While the two can technically be considered related, they should really be built separately (the former must always be built if we're building NMODL, while the latter only if NMODL_ENABLE_PYTHON_BINDINGS=ON). Now the embedded interpreter is still built by pybind/CMakeLists.txt, while the Python bindings are in bindings/CMakeLists.txt.

This is somewhat towards #3519 (at least, it would enable the possibility of considering it).

Other changes:

  • use nrn_add_python_library instead of pybind11_add_module (latter is tied to the version of Python that FindPython discovers, while the former can use any Python specified)
  • add NMODL_PYTHON_TARGETS and NMODL_DEFAULT_PYTHON_TARGET CMake vars
  • use PYBIND11_INCLUDE_DIR instead of PYBIND11_INCLUDE_DIRS (the latter contains Python include dirs, while the former does not)
  • use NRN_DEFAULT_PYTHON_INCLUDES (unless NRN_ENABLE_PYTHON_DYNAMIC=ON) instead of PYTHON_INCLUDE_DIRS
  • do not blindly install the whole dir, instead selectively install the targets
  • remove usage of ARGS in add_custom_command since it's deprecated

Goran Jelic-Cizmek added 3 commits July 23, 2025 14:53
* Move NMODL Python _bindings_ to its own dir
* Add `NMODL_PYTHON_TARGETS` and `NMODL_DEFAULT_PYTHON_TARGET` CMake
  vars
* Use `nrn_add_python_library` instead of `pybind11_add_module`
* Install targets more selectively
@azure-pipelines
Copy link

✔️ 82edccd -> Azure artifacts URL

@github-actions
Copy link
Contributor

✔️ 82edccd -> artifacts URL

@github-actions
Copy link
Contributor

✔️ 5793409 -> artifacts URL

@github-actions
Copy link
Contributor

✔️ 09130e7 -> artifacts URL

@github-actions
Copy link
Contributor

✔️ e22aa17 -> artifacts URL

@azure-pipelines
Copy link

✔️ e22aa17 -> Azure artifacts URL

@codecov
Copy link

codecov bot commented Jul 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.28%. Comparing base (1429b75) to head (295a8d8).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3541      +/-   ##
==========================================
+ Coverage   68.09%   68.28%   +0.18%     
==========================================
  Files         686      686              
  Lines      110908   110908              
==========================================
+ Hits        75525    75729     +204     
+ Misses      35383    35179     -204     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JCGoran JCGoran marked this pull request as ready for review July 24, 2025 12:12
@github-actions
Copy link
Contributor

✔️ 63f765d -> artifacts URL

@azure-pipelines
Copy link

✔️ 63f765d -> Azure artifacts URL

Goran Jelic-Cizmek and others added 7 commits July 25, 2025 15:56
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.
also add comment about using single triple quotes
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 4, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2025

✔️ 4fa6a87 -> artifacts URL

@azure-pipelines
Copy link

✔️ 4fa6a87 -> Azure artifacts URL

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 6, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

✔️ e7dea18 -> artifacts URL

@azure-pipelines
Copy link

✔️ e7dea18 -> Azure artifacts URL

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2025

✔️ 295a8d8 -> artifacts URL

@azure-pipelines
Copy link

✔️ 295a8d8 -> Azure artifacts URL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant