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.

[Silmathoron]

Maybe we should write this to be discussed at the next VC? (@terhorstd)
Some different perspectives might lead to new ideas on how to tackle these issues in a way that would please everyone...

[stinebuu]

@jd41 @Silmathoron We will discuss this issue during the open NEST Developer VC today, and it would be great if you join so we get your inputs.:)

[jd41]

@stinebuu Thanks for putting it on the agenda, I'll be there :)

[Silmathoron]

Since I was thinking about this during the VC, I came up with an additional possible implementation, so here is a post to summarize what might be possible:

1. Keep everything as is but add error messages in Prepare/Cleanup scope (for SetStatus...)
2. Remove Prepare and Run, make Cleanup optional, in favor of wrappers (proposal by @jd41)
   • preparation call is called when needed, Cleanup is called at exit or manually
   • there is not need for Run anymore since it would behave as Simulate
   • error messages make sure that no incorrect calls are performed before Cleanup
3. Remove Prepare, Cleanup, and Run, make all functions call the required low-level stuff (my new proposal)
   • We place wrapper around modifier functions so that they all cleanup if necessary (SetStatus...), no exceptions necessary anymore
   • wrapper around Simulate to call prepare if necessary
   • automatic cleanup on exit

To me:

1. Keeps things explicit and does not break anything but does not solve many users' issues (see post from @jd41)
2. Solves most issues but is a bit convoluted, it would break code using Prepare/Run/Cleanup
3. Hopefully solves everything while making it as easy as possible for users, it also breaks Prepare/Run/Cleanup, the only issue I see is that people need to know that some ways of organizing the code may be faster than others

[jougs]

The original reason for splitting Simulate into Prepare, Run and Cleanup was to support faster simulations when running incremental experiment protocols (for instance to explore or prototype plasticity mechanisms from the user interface level). For the performance of some recording backends (e.g. the one for SIONlib) it is cricial to prevent excessive I/O operations of the types that are now often run in Prepare and Cleanup (e.g. opening and closing of files).

I would be cautious to automatically call Prepare and Cleanup from SetStatus and such, as that defeats the idea of being explicit on what is called when and potentially reduces the users' awareness of the situation described above. My take on this would rather be that all dangerous functions should error out in case they can't be run in a Prepared context.

In summary, I completely agree with @stinebuu's proposal to fix the documentation and throw errors when dangerous operations are summoned by users. The real work here is to identify, which operations are OK and which are not.

[jd41]

Hey @Silmathoron actually the initial message in this thread did include a change to SetStatus (in the bullet points), but I forgot to mention it explicitly in the Update: part.^1

I think the option of manual cleanup is needed even in your variant 3, people must be able to sync/close files manually in the middle of the simulation if they want to access recordings on file before a script ends. A part of the issue @stinebuu's raises "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." is that with just a Run, they may not even know there is a problem (do you have another example of when people may need to call Cleanup manually @stinebuu?).

I'd also like to add variant 4, which means variant 3 (+manual cleanup)+adding warnings/information messages as here.^2 To my mind, this would mostly solve @stinebuu and @jougs worry that users would not be aware of what is happening.

A concrete scenario I see how something can go wrong if the Prepare/Cleanup is not done automatically (besides the added time of understanding, adding these functions and getting them to run at all, and general inherent risk of complexity) is if people have included control logic in their scripts where they use SetStatus in the middle with some, but not all, parameter settings, and something goes wrong with that. Without automatically doing the right thing, this might then cause errors and add to the misery of whatever poor soul tries to rerun the code from a paper...

There have been several misunderstandings and apparently lots of typing time in this thread so far, if you are interested in furthering this discussion, I think it should be finalized by voice, ideally among less than 5 people. I'll also ask my "NEST user focus group" (i.e. the group meeting) tomorrow to understand better how often these potential problems actually arise.

_{^1 As this was unclear, I should have taken my own advice to be concise, bullet-pointed, and non-redundant in that message...
^2 If an expert is annoyed by these warning messages, they could suppress them with manual Cleanups before each SetStatus.}

[jd41]

Brief recap from the "focus group" (9 NEST users including me):

• noone was aware of code (that they themselves or others wrote) which needed to make sure that files are synced and closed at some point within the simulation (which was @stinebuu's concern here if I understand correctly).

• One user told me that they had once used code which saved some parameters in between the simulation Runs, but this would be handled automatically by the proposal above and it is a rare occurence in their experience.

[jougs]

It might be that syncing and closing mid-flight is not needed often, but everyone who wants to analyze their data while the simulation is still running needs to be able to do this. Especially in long-running experiments involving plasticity this is crucial to not waste allocated compute time (and thus energy) on a possibly dead network.

And my other point still stands: If SetStatus automatically calls Cleanup and Prepare, users will not (and cannot) be aware of the fact that these might be (extremely) expensive operations in certain situations (like for instance a full supercomputer simulation opening a million data files). Moreover, this proposal does not consider any of the streaming input and output backends that are currently under development and for which preparation might involve setting up networking and authentication towards remote resources that might not even work a second time...

If anything, I'd be for removing Simulate altogether in favor of just having Prepare Run and Cleanup, proper documentation for how simulations are supposed to be structured and proper error messages for functions that are called in the wrong context.

[jd41]

@jougs

It might be that syncing and closing mid-flight is not needed often, but everyone who wants to analyze their data while the simulation is still running needs to be able to do this.

1. This would still be possible by adding manual Cleanups, if the user wishes to during development of the network. None of the variants I suggested remove functionality. The question in my mind is just how much it is a problem that users may not be aware of that if the tutorial starts saying "use Run" + a Sphinx .. warning :: role talking about how Cleanup is needed if you want to ensure syncing.

2. Is it actually the case that data never gets saved if Cleanup is not called? I would naively guess that the OS still does this fairly regularly, but I don't know the details.

And my other point still stands: If SetStatus automatically calls Cleanup and Prepare, users will not (and cannot) be aware of the fact that these might be (extremely) expensive operations in certain situations

3. To what extent do you think variant 4 here would solve this problem?
4. Minor quibble: My proposal doesn't include SetStatus calling Prepare, this would be done automatically by the next Run.

If anything, I'd be for removing Simulate altogether in favor of just having Prepare Run and Cleanup

I'm not so enthusiastic about that right now tbh but may have a different opinion on Monday, what do you think @stinebuu @Silmathoron?

[jd41]

Sorry, had to correct some formatting in my last comment

[jougs]

There still seems to be a formatting issue (or I don't get what the "Sphinx .. warning .. :: role" part is meant to mean). I also changed your numbering above, so it is easier to quote.

1. ...

Yes, but I'd rather have the user always do this explicitly, as this might require moving files away or setting new names to prevent errors or overridden files

2. ...

Most likely, some destructor will write the data when the process ends.

3. ...

Not sure at the moment.

4. ...

This is exactly my point. I want users to be aware that SetStatus might just not be possible in certain situations if performance is of any concern. If things just happen behind the scenes, they have no chance of realizing and acting accordingly.

---

I think what my scepticism against changing the current functions boils down to, is the following:
The split was implemented for good reasons and I'd be very careful to replace/combine functions without a very thorough analysis of all the pros, cons and corner cases. IMO this issue got way too long and convoluted for anyone to really follow along and get what the current proposal actually is, what problem it tries to solve (and for whom, i.e. laptop/supercomputer users) and what new problems it might create.

[jd41]

IMO this issue got way too long and convoluted for anyone to really follow along and get what the current proposal actually is, what problem it tries to solve (and for whom, i.e. laptop/supercomputer users) and what new problems it might create.

There have been several misunderstandings and apparently lots of typing time in this thread so far, if you are interested in furthering this discussion, I think it should be finalized by voice, ideally among less than 5 people.

I think you and I are saying similar things here =)

[apeyser]

This whole thing was built for the small minority of users who would need to poke around with internals -- specifically, collect data during a simulation phase, as per NestIO. I'd avoid doing any complicated work here -- it would defeat the purpose -- beyond simply throwing an exception on empirically defined "bad behavior", since everything is undefined in terms of these calls.

Or rewrite NEST from scratch?

As I recall, the original issues discussed this already, and decided already that without a significant cleanup of the NEST internals, the only choices left are: a) There Be Dragons Here; or b) don't expose these calls at all.

[heplesser]

I would like to take up the discussion here again, because we still do not raise exceptions when any network-modifying commands ( SetStatus, Create, Connect, SetKernelStatus, any other?) are attempted between Prepare and Cleanup.

Following Python's explicit is better than implicit maxime, I would suggest the following approach:

• Run() can only be used inside a RunManager() context
• Prepare() and Cleanup() become private functions
• Protection against network-modifying functions is implemented at the C++ level
• Consider renaming RunManager, e.g. to SimulationManager og SimulationContext
• Consider to drop Run and just have the Simulate function but implemented such that it behaves like the old Run when used inside the simulation context.

[tfardet]

any other?)

would Disconnect be safe?

I rather like your proposal about limiting available functions to Run within a Runmanager context

[stinebuu]

@heplesser If Prepare() and Cleanup() are private functions, do you mean that they are private on PyNEST level?
When I run benchmarks it is useful to have Prepare() because it is during the preparation stage that the connections are sorted. It is a bit more intuitive to have benchmarks with the Prepare(), Run(), Cleanup() phases separate in my opinion.

[tfardet]

@stinebuu but are there cases where you could not replace these calls to Prepare and Cleanup by a RunManager context?

[heplesser]

@stinebuu They should be private on the PyNEST level, so you could still use them if you wanted ;). With the built-in timers provided by #1778, timing from the Python level becomes maybe also less important. And finally, I guess you could measure prepare/cleanup times even with a context by smart placement of timers:

prepare_start = time.time()
with nest.RunManager():
    sim_start = time.time()
    nest.Run(1000)
    sim_stop = time.time()
cleanup_stop = time.time()

t_prep = sim_start - prepare_start
t_sim = sim_stop - sim_start
t_cleanup = cleanup_stop - sim_stop

[stinebuu]

Yes, this would work for me :)

[stinebuu]

We looked at this again during the NEST 3.0 Finalton (08/06/2020). The issue with calling SetStatus between run will be fixed with #2060, it should then be possible to call SetStatus. As there are no RunManager for Sli, we keep Prepare, run, Cleanup and Simulate as is for now.

I am thus closing this issue, as it will be fixed with #2060.