Avoid calling cleanup from destructor (#168)

## Context
Neurodamus used to ensure `cleanup` was called by calling it from the
destructor of the `Neurodamus` class.

However, depending on the python version, this could lead to crashes as
some objects from `Node` eventually were already destroyed.

To workaround this problem, at some point we introduced the
`cleanup_atexit` parameter which would skip over this procedure, and
notably multiscale_run was using it. However this was not a solution as
cleanup was not being called.

## Scope
This PR fixes this long standing issue. Cleanup is now called directly
from the top-level `run()` function. It can still be avoided if the user
really wants to keep the data, with the parameter `cleanup=False`
 
## Testing
CI is happy.

## Review
* [x] PR description is complete
* [x] Coding style (imports, function length, New functions, classes or
files) are good

neurodamus/commands.py

@@ -11,6 +11,8 @@
 from os.path import abspath
 from pathlib import Path
 
+from neurodamus.utils.timeit import TimerManager
+
 from . import Neurodamus
 from .core import MPI, OtherRankError
 from .core.configuration import ConfigurationError, LogLevel, EXCEPTION_NODE_FILENAME
@@ -81,6 +83,7 @@ def neurodamus(args=None):
 
     try:
         Neurodamus(config_file, True, logging_level=log_level, **options).run()
+        TimerManager.timeit_show_stats()
     except ConfigurationError as e:  # Common, only show error in Rank 0
         if MPI._rank == 0:           # Use _rank so that we avoid init
             logging.error(str(e))

neurodamus/node.py

@@ -1523,9 +1523,6 @@ def dump_circuit_config(self, suffix="nrn_python"):
         for gid in gids:
             self._pc.prcellstate(gid, suffix)
 
-    # ---------------------------------------------------------------------------
-    # Note: This method may be called automatically from Neurodamus.__del__
-    #     and therefore it must stay as simple as possible as exceptions are ignored
     def cleanup(self):
         """Have the compute nodes wrap up tasks before exiting.
         """
@@ -1537,30 +1534,29 @@ def cleanup(self):
             self.clear_model(avoid_creating_objs=True)
 
         if SimConfig.delete_corenrn_data and not SimConfig.dry_run:
-            data_folder = SimConfig.coreneuron_datadir
-            logging.info("Deleting intermediate data in %s", data_folder)
-
             with timeit(name="Delete corenrn data"):
-                if MPI.rank == 0:
-                    if ospath.islink(data_folder):
-                        # in restore, coreneuron data is a symbolic link
-                        os.unlink(data_folder)
-                    else:
-                        subprocess.call(['/bin/rm', '-rf', data_folder])
-                    os.remove(ospath.join(SimConfig.output_root, "sim.conf"))
-                    if self._run_conf["EnableReports"]:
-                        os.remove(ospath.join(SimConfig.output_root, "report.conf"))
-
-                # Delete the SHM folder if it was used
-                if self._shm_enabled:
-                    data_folder_shm = SHMUtil.get_datadir_shm(data_folder)
-                    logging.info("Deleting intermediate SHM data in %s", data_folder_shm)
-                    subprocess.call(['/bin/rm', '-rf', data_folder_shm])
+                self._delete_corenrn_data()
+                MPI.barrier()
 
-            MPI.barrier()
+    @run_only_rank0
+    def _delete_corenrn_data(self):
+        data_folder = SimConfig.coreneuron_datadir
+        logging.info("Deleting intermediate data in %s", data_folder)
+
+        if ospath.islink(data_folder):
+            # in restore, coreneuron data is a symbolic link
+            os.unlink(data_folder)
+        else:
+            subprocess.call(['/bin/rm', '-rf', data_folder])
+        os.remove(ospath.join(SimConfig.output_root, "sim.conf"))
+        if self._run_conf["EnableReports"]:
+            os.remove(ospath.join(SimConfig.output_root, "report.conf"))
 
-        logging.info("Finished")
-        TimerManager.timeit_show_stats()
+        # Delete the SHM folder if it was used
+        if self._shm_enabled:
+            data_folder_shm = SHMUtil.get_datadir_shm(data_folder)
+            logging.info("Deleting intermediate SHM data in %s", data_folder_shm)
+            subprocess.call(['/bin/rm', '-rf', data_folder_shm])
 
 
 # Helper class
@@ -1572,7 +1568,6 @@ class Neurodamus(Node):
     def __init__(
         self, config_file,
         auto_init=True,
-        cleanup_atexit=True,
         logging_level=None,
         **user_opts
     ):
@@ -1592,12 +1587,8 @@ def __init__(
                 1 - Info messages (default)
                 2 - Verbose
                 3 - Debug messages
-            cleanup_atexit: (bool) Call cleanup in the destructor
-                [for more see: https://bbpteam.epfl.ch/project/issues/browse/BBPBGLIB-976]
             user_opts: Options to Neurodamus overriding the simulation config file
         """
-        self._init_ok = False
-        self.cleanup_atexit = cleanup_atexit
         enable_reports = not user_opts.pop("disable_reports", False)
 
         if logging_level is not None:
@@ -1612,17 +1603,13 @@ def __init__(
             self.load_targets()
             self.create_cells()
             self.create_synapses()
-            self._init_ok = True
             return
 
         if SimConfig.restore_coreneuron:
             self._coreneuron_restore()
         elif SimConfig.build_model:
             self._instantiate_simulation()
 
-        # In case an exception occurs we must prevent the destructor from cleaning
-        self._init_ok = True
-
         # Remove .SUCCESS file if exists
         self._success_file = SimConfig.config_file + ".SUCCESS"
         self._remove_file(self._success_file)
@@ -1782,9 +1769,14 @@ def _instantiate_simulation(self):
 
     # -
     @timeit(name="finished Run")
-    def run(self):
+    def run(self, cleanup=True):
         """Prepares and launches the simulation according to the loaded config.
         If '--only-build-model' option is set, simulation is skipped.
+
+        Args:
+            cleanup (bool): Free up the model and intermediate files [default: true]
+                Rationale is: the high-level run() method it's typically for a
+                one shot simulation so we should cleanup. If not it can be set to False
         """
         if SimConfig.dry_run:
             log_stage("============= DRY RUN (SKIP SIMULATION) =============")
@@ -1803,12 +1795,12 @@ def run(self):
         else:
             log_stage("======================= SIMULATION =======================")
             self.run_all()
+
         # Create SUCCESS file if the simulation finishes successfully
         self._touch_file(self._success_file)
-        logging.info("Creating .SUCCESS file: '%s'", self._success_file)
+        logging.info("Finished! Creating .SUCCESS file: '%s'", self._success_file)
 
-    def __del__(self):
-        if self._init_ok and self.cleanup_atexit:
+        if cleanup:
             self.cleanup()
 
     @run_only_rank0