Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring NMODL transpiler under NEURON project #3265

Draft
wants to merge 855 commits into
base: master
Choose a base branch
from
Draft

Conversation

pramodk
Copy link
Member

@pramodk pramodk commented Dec 2, 2024

  • Script to trim and clean NMODL repository
  • Test NMODL repository
  • Merge nmodl trimed repo into NEURON - A Dry Run!
  • "Marriage" of CMake - merging/linking NEURON and NMODL CMake
  • Cleanup and remove unnecessary/duplicate files

Steps To Follow

Here are 5 major steps that we need to follow:

Clone NRN and NMODL Repos

#!/bin/bash

BASE_DIR=$(pwd)/nrn-nmodl-merge
mkdir -p ${BASE_DIR} && cd ${BASE_DIR}

git clone -b master --single-branch [email protected]:BlueBrain/nmodl.git
git clone -b master --single-branch [email protected]:neuronsimulator/nrn.git

Install git-filter-repo

pip install git-filter-repo

# TODO: change if necessary, `git-filter-repo` should be in $PATH
export PATH=$HOME/.local/bin/:$PATH

Prepare NMODL Repo

Use git filter-repo to do all hard work 👷

#!/bin/bash


cd ${BASE_DIR}/nmodl

# create a commit hook file that will be used by `git filter-repo`
cat <<EOF >> commit_hook.py

commit_message = commit.message.decode()
original_id = commit.original_id.decode()

# lets first remove trailing new lines
commit_message = commit_message.rstrip("\n")

# append now the original SHA in the commit message
commit_message = commit_message + "\n\nNMODL Repo SHA: BlueBrain/nmodl@" + original_id

# make links to PRs
matches = re.findall('#[0-9]+', commit_message, re.DOTALL)
if matches:
    for match in set(matches):
        # rename the PR IDs as they could be misleading / point to nrn PRs
        match.replace('#', '#nmodl-')
        new_prefix = "BlueBrain/nmodl" + match
        commit_message = commit_message.replace(match, new_prefix)

commit.message = commit_message.encode()
EOF

# first change commit messages and PR references
git filter-repo --force --preserve-commit-hashes --commit-callback commit_hook.py
rm commit_hook.py

# rename tags
git filter-repo --tag-rename '':'nmodl-'

# remove files that we don't need after merging in nrn
git filter-repo \
    --path .bbp-project.yaml \
    --path .clang-format.changes \
    --path .clang-tidy \
    --path .clang-tidy \
    --path .gitignore \
    --path .gitlab-ci.yml \
    --path .github \
    --path .gitmodules \
    --path .sanitizers \
    --path .cmake-format.changes.yaml \
    --path AUTHORS.txt \
    --path MANIFEST.in \
    --path azure-pipelines.yml \
    --path ci \
    --path ext \
    --path packaging \
    --path pyproject.toml \
    --path requirements.txt \
    --path setup.cfg \
    --path sonar-project.properties \
    --invert-paths

# now start renaming and remapping of directories and files 
git filter-repo --path-rename cmake/:cmake/nmodl/
git filter-repo --path-rename CMakeLists.txt:cmake/nmodl/CMakeLists.txt
git filter-repo --path-rename CONTRIBUTING.rst:docs/nmodl/transpiler/CONTRIBUTING.rst
git filter-repo --path-rename docs/:docs/nmodl/transpiler/
git filter-repo --path-rename INSTALL.rst:docs/nmodl/transpiler/INSTALL.rst
git filter-repo --path-rename LICENSE:src/nmodl/LICENSE
git filter-repo --path-rename python/:src/nmodl/python/
git filter-repo --path-rename README.rst:src/nmodl/README.rst
git filter-repo --path-rename share/nrnunits.lib:share/nmodl/nrnunits.lib
git filter-repo --path-rename src/:src/nmodl/
git filter-repo --path-rename test/:test/nmodl/transpiler/

We now have a "cleaned" version of NMODL repository. See if generated structure is what you want:

$ tree -L 3
.
├── cmake
│   └── nmodl
│       ├── CMakeLists.txt
│       ├── ClangTidyHelper.cmake
│       ├── CompilerHelper.cmake
│       ├── Config.cmake.in
│       ├── ExternalProjectHelper.cmake
│       ├── FlexHelper.cmake
│       ├── GitRevision.cmake
│       ├── PythonLinkHelper.cmake
│       ├── RpathHelper.cmake
│       └── hpc-coding-conventions
├── docs
│   └── nmodl
│       └── transpiler
├── share
│   └── nmodl
│       └── nrnunits.lib
├── src
│   └── nmodl
│       ├── CMakeLists.txt
│       ├── ast
│       ├── codegen
│       ├── config
│       ├── language
│       ├── lexer
│       ├── main.cpp
│       ├── nmodl
│       ├── parser
│       ├── printer
│       ├── pybind
│       ├── solver
│       ├── symtab
│       ├── units
│       ├── utils
│       └── visitors
└── test
    └── nmodl
        └── transpiler

We can now push this trimmed-down version of the repo into neuronsimulator org to verify if the commits and PRs are properly linked to original NMODL repo:

git remote add  neuronsimulator  [email protected]:neuronsimulator/nmodl-test.git
git push -f --all neuronsimulator

See the result at https://github.com/neuronsimulator/nmodl-test.

Merge NMODL Repo into NEURON

Now comes the part of merging the two repositories. Add "cleaned" nmodl repo as remote and merge the two repos as:

cd ${BASE_DIR}/nrn
git remote add nmodl file://${BASE_DIR}/nmodl
git fetch  nmodl --tags
git merge --allow-unrelated-histories nmodl/master

This should merge the repos cleanly. We can push this to nrn branch:

git push -f origin HEAD:pramodk/nmodl-merge

"Marriage" of CMake and Build System

We have now nmodl and nrn repo merged. But there are various things that needs to be happened:

  • Add any submodules needed for NMODL
  • Update CMakeLists.txt and connect nmodl's CMakeLists.txt
  • Check requirements.txt and other build dependencies
  • Update Spack recipe (?)
  • Update CIs
  • Remove unnecessary files from docs, cmake, tests, etc

Omar Awile and others added 30 commits March 5, 2024 15:20
* Update to latest hpc-coding-conventions
* Pin version 13 for clang-format to avoid code reformatting

NMODL Repo SHA: BlueBrain/nmodl@f930357
* replace `setup.py` with `pyproject.toml`
* move all Python package requirements to single `requirements.txt`
* remove checking Python package requirements in `CMakeLists.txt`
* move Python bindings to own dir (python/nmodl), fixes BlueBrain/nmodl#462
* add separate script for generating docs
* add `packaging/change_name.py` script to as workaround to enable both
  NMODL and NMODL-nightly wheels in CI
* update documentation and CI to reflect above changes

---------

Co-authored-by: Luc Grosheintz <[email protected]>

NMODL Repo SHA: BlueBrain/nmodl@3acc935
* use cibuildwheel for creating redistributable wheels
* simplify CI pipeline
* update packaging docs

---------

Co-authored-by: Nicolas Cornu <[email protected]>
Co-authored-by: Luc Grosheintz <[email protected]>

NMODL Repo SHA: BlueBrain/nmodl@347f786
* Throw error if passing directory instead of file to `parse_file`
* Add test for it

NMODL Repo SHA: BlueBrain/nmodl@f8c8d23
The previous behaviour for

    KINETIC states {
         ~ x + y << 42.0
    }

was to print a warning and continue as if the line didn't exist. The new
behaviour is to throw an error, because it's unclear why ignoring the
line could ever lead to a correct translation of the MOD file.

NMODL Repo SHA: BlueBrain/nmodl@9292c18
* Format Python files with `black`.
* Enable `black` formatter.

NMODL Repo SHA: BlueBrain/nmodl@40aa0bf
When encountering an indexed `COMPARTMENT` block:

    COMPARTMENT i, volume_expr { species }

All state variables are searched if they match they match the pattern
`"{species}[%d]"`. For each matching state variables, we obtain the
value of the array index and substitute the name of the array index with
its value in `volume_expr`. This is then stored in the array of
compartment factors.

Check that the resulting derivatives are divided by the volume
of the compartment.

NMODL Repo SHA: BlueBrain/nmodl@219a3ed
…1234)

This can be reproduced by:

    cmake -B build && cmake --build build --target=generate_references

i.e. call `--target=generate_references` without building it first. It
would complain about missing `bin/nmodl`.

NMODL Repo SHA: BlueBrain/nmodl@735b11d
Store the forward and reverse rates in local variables, e.g.:
```
rate = 42.0
~ a <-> b (1.0*rate, 2.0*rate)
```
is converted to:
```
LOCAL kf0_, kb0_
rate = 42.0
kf0_ = 1.0*rate
kb0_ = 2.0*rate
~ a <-> b (kf0_, kb0_)
```

This solves a bug that assigning to `rate` between two reaction equation statements would mean the first line sees the value meant for the second line.

NMODL Repo SHA: BlueBrain/nmodl@868ce13
In NRN we depend on the target `nmodl` to ensure that when we run `nrnivmodl -coreneuron` the binary `nmodl` exists. We need to also ensure that everything `nmodl` needs to works is present.

This fixes a dependency bug on certain copied files by making the target for copying the files a dependency of `nmodl`.

NMODL Repo SHA: BlueBrain/nmodl@b78c3f3
* Fix array variables.

These are the code generation changes required for:

    #2779

The solution is to fill `nrn_prop_param_size` with the number of
doubles, not number of variables.

NMODL Repo SHA: BlueBrain/nmodl@f6821ce
…dl#1244)

- If `-DNMODL_ENABLE_PYTHON_BINDINGS=OFF` then there is no need
  to generate AST and other wrapper classes for Pybind11.
- Using sympy-based solvers does not require Python bindings to
  be enabled. Avoid confusing warning when using nmodl binary.
- When someone tries to use the nmodl module via Python, the
  warning is still preserved.

```console
$ nmodl mod_examples/sparse.mod
[NMODL] [warning] :: Python bindings are not available with this installation
..

$ nmodl mod_examples/sparse.mod
$ python3.11 -c "import nmodl"
[NMODL] [warning] :: Python bindings are not available with this installation
```

NMODL Repo SHA: BlueBrain/nmodl@8eb88e4
- when neuron is built with nmodl then nmodl related files
  should be installed in <prefix>/ and not in <prefix>/nmodl/.data
- <prefix>/nmodl/.data is used when we build standalone wheels
- add one mod file using sparse solver - nmodl will automatically
  use sympy solver

NMODL Repo SHA: BlueBrain/nmodl@2fb037e
1uc and others added 24 commits October 29, 2024 13:55
Lower all of `process_verbatim_text` and the implementation
of `visit_verbatim` to the CoreNEURON visitor. The NEURON
version will use a different system.

NMODL Repo SHA: BlueBrain/nmodl@d4a43e9
Both the NEURON and CoreNEURON codegen need the same
code for printing all top-level verbatim blocks.

NMODL Repo SHA: BlueBrain/nmodl@41f0815
The race is cause by the fact that two targets exist to copy the Python
files. The first one because of `cpp_cc_build_time_copy` without
`NO_TARGET`. The second because of the:

    add_custom_target(nmodl_copy_python_files ALL DEPENDS ...)

The solution is to not create the per file target by passing
`NO_TARGET` to `cpp_cc_build_time_copy`.

NMODL Repo SHA: BlueBrain/nmodl@1f0c5c1
The code for the code-generator NMODL is itself generated from Jinja
templates. This requires CMake integration to ensure it happens
automatically.

The following has been changed about the integration:

  * The CMake code needs a list of the generated files. This list is
    stored in a CMake file and included. This file must exist before
    running `code_generator.py` to generate the NMODL code, because the
    command to generate the files needs to know which file it will be
    generating. Therefore, we split the script into two phases: one to
    generate the list of generated files, and another to generate the
    files.

  * The list of generated files depends on the build flags. Therefore,
    this file (`code_generator.cmake`) can't be checked into the Git
    repo. The `code_generator.cmake` is created during the configure
    phase, and now lives in the build directory.

  * CMake code was lifted from `src/language/CMakeLists.txt` to the
    top-level `CMakeLists.txt` to avoid any scoping issues for the
    lists of generated files.

  * Removed pattern of yielding tasks while creating a list tasks in
    favour of returning the list of tasks directly.

NMODL Repo SHA: BlueBrain/nmodl@49fdfe6
* Lower `visit_protected_statement`.

The code is lowered into the CoreNEURON side, because
NEURON can't use the same OpenMP system.

* Lower `print_atomic_reduction_pragma`.

This method is specific to the CoreNEURON implementation of
`visit_protected_statement`.

NMODL Repo SHA: BlueBrain/nmodl@ea94748
The previous implementation would apply the transformation to children
of the node in the AST. Hence, if the root was `sp.Derivative` it would
fail.

This commit adds a fix and the tests.

NMODL Repo SHA: BlueBrain/nmodl@16b5f95
* Fix thread_variable conditions.

* Support PROTECT via `_NMODLMUTEX{,UN}LOCK`.

* Add tests.

NMODL Repo SHA: BlueBrain/nmodl@96299cb
@pramodk pramodk marked this pull request as draft December 2, 2024 23:36
Copy link

sonarcloud bot commented Dec 2, 2024

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.07%. Comparing base (5fe797a) to head (ab66e5e).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3265   +/-   ##
=======================================
  Coverage   67.07%   67.07%           
=======================================
  Files         571      571           
  Lines      111095   111095           
=======================================
  Hits        74521    74521           
  Misses      36574    36574           

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

Copy link

✔️ ab66e5e -> Azure artifacts URL

@pramodk pramodk mentioned this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants