Add NEST 2 stage to CI #720

clinssen · 2022-01-17T08:01:59Z

Combines to_nest() and install_nest() NESTML API functions into a single generate_target(), thereby fixing #503.

Introduces the concept of an abstract base class, Builder, analogous to CodeGenerator.

Adds NEST 2.20.2 integration test to CI.

med-ayssar

Since the installation path of nest is constant, wouldn't better to set the default path without having the user explicitly calling those nest functions to extract the path?

pynestml/codegeneration/builder.py

med-ayssar · 2022-01-17T10:04:21Z

pynestml/codegeneration/builder.py

+            from pynestml.codegeneration.nest_builder import NESTBuilder
+            return NESTBuilder(target_name, options)
+
+        return None   # no builder requested or available


Won't be better to throw an error, so that we make sure if we have builder or not.

It's alright if there is a code generator (like for .rst documents) that doesn't have a build stage.

med-ayssar · 2022-01-17T10:20:43Z

pynestml/codegeneration/code_generator.py

+            if k in self.__class__._default_options:
+                self._options[k] = options[k]
+            else:
+                print("Warning: Option \"" + str(k) + "\" does not exist in code generator")

    def get_option(self, k):
        return self._options[k]


I think the following would be better: self._options.get(k, None), in case if the dictionary doesn't contain the key.

I'd rather it throw an exception. What if there exists a known key but with value None?

tfardet · 2022-01-17T12:49:28Z

I'm fairly suprised with this change of to_nest to generate_target that's going to break all existing code.
Is there a plan for NESTML to support other platforms than NEST?
If not I'd really advise to reconsider this: repeatedly breaking backward compatibility tends to discourage users.
Given that it does not say anywhere that NESTML API is unstable and that we've been advertising NESTML for years now, people would (understandably) expect it to be stable.

clinssen · 2022-01-18T11:58:52Z

Since the installation path of nest is constant, wouldn't better to set the default path without having the user explicitly calling those nest functions to extract the path?

I like this idea, because it means that we don't have to explicitly pass nest_path in the code generation opts of every single call to generate_target(). Because we should avoid for NESTML to import nest anywhere outside of the tests, I'll implement this as a single external Python script that just does import nest; print(nest.ll_api.sli_func("statusdict/prefix ::")), then call this script in nest_codegenerator.py while capturing stdout.

I'm fairly suprised with this change of to_nest to generate_target that's going to break all existing code. Is there a plan for _NEST_ML to support other platforms than NEST? If not I'd really advise to reconsider this: repeatedly breaking backward compatibility tends to discourage users. Given that it does not say anywhere that NESTML API is unstable and that we've been advertising NESTML for years now, people would (understandably) expect it to be stable.

I agree completely that API changes should be kept to a minimum, but we have a growing number of code generation targets: due to evolution of the NEST API, we now even have two NEST code generators (for major versions 2 and 3); there is also "audodoc" for generating model documentation, "spinnaker" is hopefully coming soon, and I am secretly planning a minimal self-contained Python code generation option for debugging purposes.

These changes are still very flexible and up for discussion, for example, the following may or may not be true:

Generating code without building it is not useful, so we don't need separate calls for that, just one "generate_and_build": generate_target()
All code generator-specific options should go into the codegen_opts dictionary. For instance, SpiNNaker (or other non-NEST code generators) might not have any notion of a "module", so perhaps the parameter module_name should also go into codegen_opts.

I will create an issue to create a transition guide for NESTML 4 → 5, which will document the changes and how to adapt scripts.

tfardet · 2022-01-18T16:19:53Z

I agree completely that API changes should be kept to a minimum, but we have a growing number of code generation targets: due to evolution of the NEST API, we now even have two NEST code generators (for major versions 2 and 3); there is also "audodoc" for generating model documentation, "spinnaker" is hopefully coming soon, and I am secretly planning a minimal self-contained Python code generation option for debugging purposes.

This sound like very different targets and use cases, I'd rather have separate functions rather than an argument. It should also make it easier for new people to find them and understand their logic.

These changes are still very flexible and up for discussion, for example, the following may or may not be true:
* Generating code without building it is not useful, so we don't need separate calls for that, just one "generate_and_build": `generate_target()`

It can probably be useful when making new models, for debugging, and generally for user to understand what the NESTML code does, but probably minor, yes.

* All code generator-specific options should go into the `codegen_opts` dictionary. For instance, SpiNNaker (or other non-NEST code generators) might not have any notion of a "module", so perhaps the parameter `module_name` should also go into `codegen_opts`.

Again, I really think this screams "separate functions".

pnbabu · 2022-01-20T07:02:48Z

pynestml/codegeneration/resources_nest/point_neuron/common/NeuronClass.jinja2

@@ -481,11 +481,7 @@ void {{neuronName}}::handle(nest::DataLoggingRequest& e)
 {% if has_spike_input %}
 void {{neuronName}}::handle(nest::SpikeEvent &e)
 {
-#if NEST2_COMPAT


Why is this removed?

This was changed some time around NEST 2.18. @jougs: which NEST2 version(s) do we want to support here, does it make sense to retain support for older versions of NEST 2 (that is, older than 2.20.2)?

doc/running.rst

pynestml/codegeneration/builder.py

pynestml/codegeneration/nest2_code_generator.py

doc/running.rst

pynestml/codegeneration/nest_code_generator.py

med-ayssar · 2022-02-08T12:34:44Z

pynestml/codegeneration/resources_nest/point_neuron/SynapseHeader.h.jinja2

-} // namespace
-
-#endif /* #ifndef {{synapseName.upper()}}_H */
+{%- include "common/SynapseHeader.h.jinja2" %}


If it doesn't include new changes, why is it being moved to a different place

This is to support NEST2 compatibility, so the template in common can be included from the NEST3 templates directory, or from the NEST2 templates directory.

pynestml/frontend/pynestml_frontend.py

med-ayssar · 2022-02-08T12:46:46Z

pynestml/utils/with_options.py

+            if k in self.__class__._default_options:
+                self._options[k] = options[k]
+            else:
+                print("Option \"" + str(k) + "\" does not exist in builder")


Is it ok to just print that one of the given keys doesn't exist in the _default_options without throwing an error?

doc/running.rst

pynestml/codegeneration/nest_code_generator.py

pynestml/codegeneration/nest_reference_converter.py

pynestml/frontend/pynestml_frontend.py

tests/nest_tests/resources/nestml_allmodels_codegen_opts.json

tests/vector_code_generator_test.py

.github/workflows/nestml-build.yml

med-ayssar · 2022-02-08T13:20:45Z

pynestml/frontend/pynestml_frontend.py

+    """
+    generate_target(input_path, target_path, target_platform="NEST", logging_level=logging_level,
+                    module_name=module_name, store_log=store_log, suffix=suffix, install_path=install_path,
+                    dev=dev, codegen_opts=codegen_opts)


Isn't the call to add_libraries_to_sli missing here?

It's in the NESTBuilder class.

tests/nest_tests/nest_install_module_in_different_location_test.py

med-ayssar · 2022-02-08T13:28:31Z

pynestml/codegeneration/nest_builder.py

+        target_path = FrontendConfiguration.get_target_path()
+        install_path = FrontendConfiguration.get_install_path()
+        if install_path is not None:
+            add_libraries_to_sli(install_path)


Doesn't this cause Circular dependency

Why is that? It seems to work OK.

med-ayssar · 2022-02-08T13:35:10Z

pynestml/codegeneration/nest_builder.py

+            subprocess.check_call(make_all_cmd, stderr=subprocess.STDOUT, shell=shell,
+                                  cwd=str(os.path.join(target_path)))
+        except subprocess.CalledProcessError as e:
+            raise GeneratedCodeBuildException('Error occurred during \'make all\'! More detailed error messages can be found in stdout.')


I thought the PR containing the add_libraries_to_sli has modified the exceptions:

Suggested change

raise GeneratedCodeBuildException('Error occurred during \'make all\'! More detailed error messages can be found in stdout.')

msg = "Error during 'make all'. More detailed error messages can be found in stdout."

raise GeneratedCodeBuildException(msg)

pnbabu

Looks good to me!

clinssen · 2022-02-11T16:40:53Z

Thank you for the comments and reviews!

add NEST 2.20 stage to CI

7ace307

clinssen added enhancement refactoring CI labels Jan 17, 2022

clinssen added this to the NESTML 5.0 milestone Jan 17, 2022

clinssen requested a review from med-ayssar January 17, 2022 08:02

clinssen linked an issue Jan 17, 2022 that may be closed by this pull request

Merge of to_nest() and nest_install() #503

Closed

med-ayssar reviewed Jan 17, 2022

View reviewed changes

med-ayssar mentioned this pull request Jan 18, 2022

Install Module from outside NEST lib directory #691

Merged

clinssen mentioned this pull request Jan 19, 2022

Move pre, post and mod input port qualifiers from codegen_opts to synapse models #723

Closed

pnbabu reviewed Jan 20, 2022

View reviewed changes

C.A.P. Linssen added 12 commits January 31, 2022 15:09

_default_options is always present in Builder and CodeGenerator

9b51cad

let code generator and builder automatically grab nest_path

32a0eb1

update running NESTML documentation

c21027a

move code generator factory to frontend

295f0f2

Merge remote-tracking branch 'upstream/master' into nest2_ci

c4bef4b

fix unit tests

593644f

fix unit tests

ddd4b78

fix namespace decorator

6f3cc0e

Merge remote-tracking branch 'upstream/master' into nest2_ci

ff4cd8a

fix install_path parameter

318556f

revert to to_nest() API

d8e5fbd

fix install_path parameter

a39f62a

clinssen force-pushed the nest2_ci branch from 42c3026 to a39f62a Compare February 2, 2022 21:21

C.A.P. Linssen added 2 commits February 2, 2022 22:53

fix install_path parameter

510ab2e

fix install_path parameter

643510f

med-ayssar reviewed Feb 8, 2022

View reviewed changes

pnbabu reviewed Feb 8, 2022

View reviewed changes

med-ayssar reviewed Feb 8, 2022

View reviewed changes

C.A.P. Linssen added 2 commits February 10, 2022 15:45

change parameter order in generate_target()

dc6c219

rename to_nest() to generate_nest_target()

d64a35c

pnbabu approved these changes Feb 11, 2022

View reviewed changes

clinssen modified the milestones: NESTML 5.0, NESTML 5.0.0-rc2 Feb 11, 2022

clinssen merged commit 868fe66 into nest:master Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NEST 2 stage to CI #720

Add NEST 2 stage to CI #720

clinssen commented Jan 17, 2022

med-ayssar left a comment

med-ayssar Jan 17, 2022

clinssen Jan 31, 2022

med-ayssar Jan 17, 2022

clinssen Jan 31, 2022

tfardet commented Jan 17, 2022

clinssen commented Jan 18, 2022

tfardet commented Jan 18, 2022 •

edited

Loading

pnbabu Jan 20, 2022

clinssen Jan 31, 2022

med-ayssar Feb 8, 2022

clinssen Feb 10, 2022

med-ayssar Feb 8, 2022

med-ayssar Feb 8, 2022

clinssen Feb 10, 2022

med-ayssar Feb 8, 2022

clinssen Feb 10, 2022

med-ayssar Feb 8, 2022

pnbabu left a comment

clinssen commented Feb 11, 2022

	raise GeneratedCodeBuildException('Error occurred during \'make all\'! More detailed error messages can be found in stdout.')
	msg = "Error during 'make all'. More detailed error messages can be found in stdout."
	raise GeneratedCodeBuildException(msg)

Add NEST 2 stage to CI #720

Add NEST 2 stage to CI #720

Conversation

clinssen commented Jan 17, 2022

med-ayssar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfardet commented Jan 17, 2022

clinssen commented Jan 18, 2022

tfardet commented Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnbabu left a comment

Choose a reason for hiding this comment

clinssen commented Feb 11, 2022

tfardet commented Jan 18, 2022 •

edited

Loading