Can we replace nest.Prepare/nest.Run/nest.Simulate/nest.Cleanup by a single or two wrapper command(s)?

[jd41]

See also #1772.

Update: The basic idea of this change is to keep track of whether the simulation is currently in a prepared state (e.g. nest.Prepare() has been called, but nest.Cleanup() hasn't), and put nest.Prepare and nest.Cleanup into wrappers which only invoke the expensive kernel functions if they are necessary/appropriate according to that status. The current motivation for exposing Prepare/Run/Cleanup rather than just one simulation command to the PyNEST user is that the user can save time by not cleaning up between runs (see the Running simulations guide). With the wrappers, the overhead of calling nest.Prepare twice becomes negligible, and we can remove much of that complexity from the API and documentation (see below for details and precisifications).

Is your feature request related to a problem? Please describe.
It is related to the problem I described in #1772, which is related to the fact that the user has to learn about 4 commands (Update: actually 5, if you include the RunManager)+at least 1 pitfall (see #1772 for the pitfall) for running simulations once vs. in stages. It seems to me that, by introducing some wrappers on the PyNEST level, the pitfall would vanish, only 2 functions would be necessary to document/understand (and the others could be deprecated/removed), and almost all users would only need to learn about 1 of them. I write here to understand whether I am missing something and it's more complicated in reality.

Describe the solution you'd like

• The PyNEST API module keeps track of a global Boolean variable "prepared". It is initialized to False.
• nest.Prepare() is replaced by a wrapper that only calls the original nest.Prepare() if prepared == False, and sets prepared = True afterwards.
• nest.Run() is replaced by a wrapper that checks if prepared == True, and calls nest.Prepare() before the original nest.Run() if it was false. UPDATE/Clarification: If it finds that prepared == True already, it skips the Prepare() call. Another UPDATE: Actually, because the new Prepare() checks whether prepared == True already and doesn't invoke the NEST kernel if it is, it wouldn't hurt performance too much to skip the check in the new Run() function.
• nest.Simulate() is replaced by a wrapper that calls the new nest.Run() (which in turn will call nest.Prepare() if the simulation wasn't prepared), followed by nest.Cleanup().
• nest.SetStatus() is replaced by a wrapper that checks if prepared == False, and calls nest.Cleanup() before the original nest.SetStatus() otherwise. UPDATE: We also could skip the if check without much performance cost, as the new nest.Cleanup() will also check whether prepared == True and not do anything expensive if it is already/still False.
• model.set() changed in the same way as nest.SetStatus().
• nest.Cleanup() is replaced by a wrapper that only calls the original nest.Cleanup() if prepared == True, and sets prepared = False afterwards.
• include atexit.register(Cleanup) from the atexit module in the PyNEST initialisation code so that nest.Cleanup() is called on program exit automatically (would this work in the way I understand it works?).
• nest.RunManager() deprecated/removed.

Alternatively, one could implement these wrapper functions in the NEST kernel.

If I understand correctly, these changes by themselves would be completely backwards-compatible and not change the behaviour of currently correct PyNEST simulation code (as well as introducing minimal overhead). However, they would make life easier for the user, who now can usually call nest.Run() instead of understanding and distinguishing Prepare/Cleanup/Simulate/Run. Only in the presumably rare case where they want to close/sync files in the middle of the simulation script, they would also need to know about and call nest.Cleanup(). The other functions (nest.Simulate() and nest.Prepare()) could be deprecated/removed from the documentation and user-visible API in the next step. The SetStatus-after-Prepare pitfall (#1772) would vanish.

Describe alternatives you've considered
I'm somewhat on the fence on whether to call the new nest.Run() function "nest.Run()" or "nest.Simulate()", because:

In the variant described above, the remaining difference between the new nest.Run() and the new nest.Simulate() would be that, as is the case currently, nest.Simulate() will be guaranteed to exit with the kernel in a cleaned-up state/files closed/whatever.

If we instead call the function that becomes the new nest.Run() above "nest.Simulate()", I see two advantages and a disadvantage:

• Advantages: I guess that most simulation code out there is fine with an unclean kernel after Simulate, and the cleanup being done automatically once on script termination (with the atexit handler). So just calling the new nest.Run() nest.Simulate() will be fine, and scripts will continue to work without users having to learn about a new command/changing their scripts/understanding one more entry in the NEST-X-to-X+1 porting guide. Scripts that didn't care about using Prepare/Run/Simulate before will can become faster.

• Disadvantage: However, if a script depends on the kernel being cleaned up after a Simulate() call in the middle of the python code, and the Simulate suddenly stops doing that, it may cause a lot of grief to whatever student tries to port that script to the next NEST version and doesn't understand why it doesn't work anymore.

Am I missing something here, and is it more complicated to do? If my understanding is correct, this seems like a relatively straightforward change in the PyNEST module to me, which I'd be happy to implement and write some unit tests for.

[Silmathoron]

I think Prepare is actually doing stuff we do not want to see done every time run is called (unnecessary overhead), so replacing it with a wrapper is not desirable.
To me, the main issue here is the pitfall: some functions must not be called within a (Prepare/Cleanup) scope but nothing prevents the user from doing it.
Therefore, I think we should implement a boolean variable (prepared above) that tells whether we are within this scope, and we should raise an error whenever a user tries to call a forbidden function inside that scope.
The main question for me is: does that bool already exist at the C++ level and if not, should it be introduced at the C++ or Python level?

[jd41]

Oh, my proposal would check whether prepared == True, and only call Prepare if it's false...

So as I understand, the only overhead would be that if check.

[jd41]

I realized overnight that the way I described this issue, describing every change to the python functions I propose, may obscure the rather straightforward basic idea. I prepended the message with a comment accordingly.

[stinebuu]

@Silmathoron @jd41 There is a bool that checks for prepare, simulating and simulated on C++ level already, see for instance

nest-simulator/nestkernel/simulation_manager.h

Lines 214 to 217 in aa894c1

	SimulationManager::has_been_prepared() const
	{
	return prepared_;
	}

This check should also be added to set/set_status and all functions that cannot be called within the prepare/run/cleanup scope.

I don't really agree about having a wrapper around prepare/run/cleanup, as this is basically what Simulate is. Though, there should be an error message if functions are called that should not be called.

[jd41]

@stinebuu The difference between the current Simulate and the proposed Run is that the current Simulate always includes a Cleanup, while the proposed Run doesn't. So calling the current Simulate twice always incurs the overhead of superfluous cleanup and prepare, while calling the proposed Run twice wouldn't (and still wouldn't fail if it was called without a preceding Prepare).

In the proposed solution, Cleanup would be called automatically on program exit with the atexit handler, and could also be called manually if the user wishes so.

[jd41]

Thanks for the answer, by the way! Then whatever is changed seems to be easier to change in C++ indeed.

[Silmathoron]

Given @stinebuu 's answer, the proposed changes seem pretty straightforward.
I'm definitely in favor of a wrapper checking the state and raising errors for forbidden functions.

Regarding the removal of Prepare and Cleanup I also have mixed feelings... in a way, I think that the addition of the error and a slightly improved doc would solve the issue encountered previously.
From my point of view the advantage of Prepare and Cleanup is that they make the scope explicit, while it would become implicit if we follow the whole proposal from @jd41

[jd41]

As I understand currently, I didn't get anything wrong with my original proposal, and it would work out that way. It would also be in the same order of straightforwardness-to-implement as @Silmathoron's proposal.

I also still believe that it would be an improvement. In short, while emitting the errors proposed by @Silmathoron would clearly improve the situation, hitting upon, understanding and fixing the source of an error message (running the script on a cluster, which we can assume is clogged until tomorrow, with the error occurring after 3 hours of runtime...) and noticing/reading/understanding/judging the relevance of/acting upon some documentation (the appropriate part of the Running Simulations guide) always has costs as well, and interface complexity has an expectation value of costs in general. In contrast, if the stuff is done correctly automatically, the cost to the user is 0 (except when they spend time reading about how not using GetStatus between Runs improves performance), and the benefit may still accrue (either if they just don't use SetStatus, or rewrite their code to use it less often).

Having the complexity of people managing Prepare/Cleanup in hundreds (I guess?) of carelessly-written simulation scripts is also a much richer source of errors, trouble, and confusion than managing it once in the NEST kernel. Even if we couldn't concretely think of something else that can go wrong here, or how someone could struggle to understand that code, that would likely say something about our imaginations rather than reality!

The two people I talked with about using Prepare/Run/Cleanup instead of Simulate before (both finished their PhDs) hadn't known about these functions at all, so the benefit to them was 0. If the right thing had been done automatically by the proposed Run command, they might have had a performance benefit from time to time in their own works. They - and anyone else - would also have been free to learn at their leisure that SetStatus between two Runs incurs a performance penalty, and acted upon that if they felt it was useful.

In my case, it turned out not to have had a benefit (because I need SetStatus between every Run). So in this concrete case, the utility of that functionality and documentation existing and me having noticed it must always have been negative.

• In the current situation, the utility was ~-1 week for me, and maybe -some hours for other people (people helping me finding the error, ~10 * 6 minutes in the group meeting... not counting the current discussion, which concerns NEST as a whole rather than just my project)

• with a perfect error message that makes the problem in my case exactly clear, but unchanged documentation, I think the utility would have been on the order of magnitude of -4 to -7 hours for me: Reading the doc, implementing the change that will turn out not to work, submitting cluster job with a new modified SLURM file, waiting for it to run, seeing the error, understanding it, making a typo when replacing Run by Simulate again, submitting the job again, waiting until the cluster is unclogged... I may also underestimate this because my current understanding is much better than it probably was when I first would have encountered this error.

• with a perfectly appropriate error message and perfectly warning documentation that got the message across to me, it may have been -20 minutes or so reading it (although I have a hard time judging that right now). I (and everyone else) would also need to scroll over and filter out the PyNEST commands in the API reference from time to time (a small cost paid often).

You can multiply the third cost by everyone ever learning about NEST and hitting upon Prepare/Run.. in the PyNEST API or the Running Simulations guide. Or wanting to get an overview of the whole documentation...^1

My experience with NEST documentation and error messages is that they typically don't immediately make clear what the problem is and how to fix it. So if an error message is tailor-made for my issue #1772, I think it's at least possible that it won't be as helpful to the next person as we may at first believe. Again, not thinking of the possible trouble may just mean something about our imaginations...

And this cost accounting just concerns the concrete problem described in #1772. Is there some other pitfall that could occur? A bug in PyNEST? Someone triggering a hard-to-catch error that doesn't occur every time by using SetStatus, or mixing up the words Run and Simulate, in some conditional block that the authors forgot to test before publishing the paper? I can't imagine that so well...

^1 I do realize that a more experienced person will have a more efficient workflow, that there are some other optimization potentials for me to reduce that timesink etc, that learning something has benefits beyond one-shot... but right now, I was only talking about the concrete consequences of that doc+code to me as of two weeks ago.

[jd41]

I reformatted my comment and would also like to suggest another solution, a combination of the two previous proposals in a way. We implement the original proposal in the issue, and in addition, whenever SetStatus is called in a prepared state, we emit a warning ("Cleaning up NEST kernel before using SetStatus. A performance penalty will be incurred in the next Run. See https://nest-simulator.readthedocs.org/guide-xyz" for details" or similar)... This would seem to me to have all the advantages of making the user understand what happens as in @Silmathoron's variant, and little of the complexity disadvantages, except for someone who knows what they are doing and is annoyed by such a warning.

[jd41]

I asked around in the last group meeting. Of ~5 NEST users present (not counting me), one person had read the "Running Simulations" guide at some point, and if I understood correctly, this person said that they don't really remember what was written in there either.

I also searched on GitHub for how often "nest.Prepare()" is currently used in Python code. I am not sure how exactly they are counted, and I get a few false positives - but I find 4400 code results for Python code that uses "nest.Create" (a proxy for code using NEST in general) and ~20 results for Python code that uses "nest.Prepare()" (a proxy for code using the current Prepare-Run-Cleanup mechanism).

[stinebuu]

@jd41 sorry for the delay in responding, things suddenly got in the way.

When we benchmark NEST, we are (often) interested in the individual Prepare, Run and Cleanup times, because in Prepare we for instance sort the connections, and this is time we are not interested in when benchmarking simulation time, as it is strictly speaking not relevant for what goes on when simulating (it has more to do with connecting). When benchmarking simulation time, we might for instance be more interested in how efficient the spike delivery is, and we want to time only the Run call. It is therefore very useful to have Prepare, Run and Cleanup as individual functions. If Prepare was part of Run this would not be possible anymore.

I am also a bit skeptical about removing Simulate and just have Run and Cleanup. One reason for this is that Simulate has been around much longer than the individual Prepare, Run and Cleanup (as you have noticed yourself not all know about the individual functions), so almost all user scripts will be broken. The users will also have to learn about Cleanup, as this is something they are not used to call. As you say, we might add it to an atexit handler, but we don't know how the scripts of users are. A lot of the time it might be very important for them to call Cleanup and then they will not, because they do not know that they have to.

I am also skeptical because as I said previously, Simulate is already a wrapper around Run, Prepare and Cleanup, and I don't really see why we should replace one with the other (though I know they are slightly different), especially because some actually use both the individual functions and the Simulate function, but for different use cases. If you look at NEST examples, a lot of them use Simulate, because they don't need to use the individual functions, and then Simulate is definitely easiest.

Don't get me wrong, though, it is definitely a problem that users have problems understanding the documentation, and/or that they are calling functions they shouldn't in between the scope and an error does not occur. This should definitively be fixed!! I think we should update both the documentation, and throw exceptions when things are wrong.