Don't include solver headers verbatim. #1344

matz-e · 2024-07-15T09:44:10Z

This first version will create a solver directory to contain the same newton.hpp and crout.hpp as present in the NMODL sources.

By default, NMODL will create the solver headers in the output directory. There are two new arguments to the NMODL invocation:

--dump-solvers <solver_names> will dump the corresponding solver/*.hpp files into the specified output directory.
--no-dump-solvers will keep a regular NMODL from creating the solver headers.

CI_BRANCHES:NEURON_BRANCH=matz-e/better-solver-dumper

codecov · 2024-07-16T15:38:30Z

Codecov Report

Attention: Patch coverage is 10.52632% with 17 lines in your changes missing coverage. Please review.

Project coverage is 86.72%. Comparing base (b91c0ea) to head (9d4696a).

Files	Patch %	Lines
src/main.cpp	6.25%	15 Missing ⚠️
src/codegen/codegen_coreneuron_cpp_visitor.cpp	50.00%	1 Missing ⚠️
src/codegen/codegen_neuron_cpp_visitor.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1344      +/-   ##
==========================================
- Coverage   86.82%   86.72%   -0.10%     
==========================================
  Files         179      179              
  Lines       13684    13700      +16     
==========================================
+ Hits        11881    11882       +1     
- Misses       1803     1818      +15

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

1uc

Could we please avoid the word "dump". It reliably reminds be of brown and smelly stuff.

1uc · 2024-07-18T06:05:59Z

src/main.cpp

+    return true;
+}
+
+void dump_solvers(const std::string& directory,


Is there a nicer word than dump. It just always triggers brown and smelly associations in me.

1uc · 2024-07-18T06:22:07Z

src/main.cpp

@@ -302,6 +349,18 @@ int main(int argc, const char* argv[]) {
        }
    };

+    if (skip_dump_solvers) {


Why does NMODL need to assert the existence of the header files? I'd have expected two modes:

Convert foo.mod to foo.cpp.

Create shared header files that are a prerequites for compiling the translated MOD files.

Note that Step 1 is meaningful whether or not the headers are created. It's perfectly fine for translating MOD files and creating the shared headers to be concurrent operations. The dependency only exists when compiling the translated MOD file, at that point we need the .cpp and all shared headers.

The hope is that we can make do with a single flag, --{create,write}-{shared-headers,prerequisites}.

It also feels a little to specific. When calling nmodl from nrnivmodl I don't particularly want to know exactly which files need to be created. I semantically just want to enable nmodl to create whatever shared header files or other prerequisites it needs. We want it as a separate, manual step, because it makes it easier to avoid race conditions by setting up the correct dependency chains.

To enable adding/removing shared headers from NMODL we'd probably want to agree on a particular file to be generated last, which can act as a target, e.g. "nmodl.hpp". Its existence guarantees that all shared headers have been created.

I'll go with --write-shared-headers, seem pretty versatile.

If we rely on a singular "nmodl.hpp" being there, that still does not guard against the present headers being outdated / in need of an update. So within NEURON, I would unconditionally write the headers.

This first version will create a `solver` directory to contain the same structure of `newton/newton.hpp` and `crout/crout.hpp` as present in the NMODL sources. *By default, NMODL will create the solver headers in the output directory.* There are two new arguments to the NMODL invocation: * `--dump-solvers <directory>` will dump the `solver/*/*.hpp` files into the specified directory. * `--no-dump-solvers` will keep a regular NMODL from creating the solver headers.

bbpbuildbot · 2024-07-23T08:21:02Z

Logfiles from GitLab pipeline #223168 (:white_check_mark:) have been uploaded here!

Status and direct links: