Repository: fitbenchmarking/fitbenchmarking

Issue:
Corrupt checkpoint files after crash
If FitBenchmarking crashes during results processing, the checkpoint file is missing a final `}` we should add a try catch to finalise the file before reraising the exception.

Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/66855443/badge)](https://coveralls.io/builds/66855443)

coverage: 90.89% (+0.002%) from 90.888%
when pulling **8afd97aa4ce42b1c40458f1c6032f33d9f8056bf on 1265-checkpoint_crash**
into **10d510730f3f25e8d304a9d2728c51b46ea5932b on master**.

AndrewLister-STFC: I can probably add a test that checks the checkpoint file is valid even after a crash post data generation.
github-actions[bot]: ## Unit & system test results
  3 files  ±0    3 suites  ±0   11m 10s :stopwatch: +35s
562 tests +1  552 :white_check_mark: +1  10 :zzz: ±0  0 :x: ±0 
567 runs  +1  552 :white_check_mark: +1  15 :zzz: ±0  0 :x: ±0 

Results for commit 7ba975af. ± Comparison against base commit a97826bb.

[test-results]:data:application/gzip;base64,H4sIAHVtxGYC/1XMTQ6DIBCG4asY1l2ICJRexsAACalKw8/K9O7FipYu32cy34asm01Ej47cOhSzS1foHGRyfi3JeF+gnNJ+pGw4a4oZYCfa0NO9CuHrZbLSzUV+YELwoUrI6zHKa/xvHlIn6QnN4rfbQfDL4lIJxJUUnEorjAJxN+MAHATVgpMRUyVIL4kGpjF6fwCxWo50BwEAAA==


Patch:
diff --git a/fitbenchmarking/cli/main.py b/fitbenchmarking/cli/main.py
index 1ae7d531..04aabe33 100755
--- a/fitbenchmarking/cli/main.py
+++ b/fitbenchmarking/cli/main.py
@@ -383,76 +383,85 @@ def run(problem_sets, additional_options=None, options_file="", debug=False):
     pp_dfs_all_prob_sets = {}
     cp = Checkpoint(options=options)
 
-    for sub_dir in problem_sets:
-        # Create full path for the directory that holds a group of
-        # problem definition files
-        data_dir = os.path.join(current_path, sub_dir)
-
-        test_data = glob.glob(data_dir + "/*.*")
-
-        if not test_data:
-            LOGGER.warning("Problem set %s not found", data_dir)
-            continue
-
-        # generate group label/name used for problem set
-        try:
-            with open(
-                os.path.join(data_dir, "META.txt"), encoding="utf-8"
-            ) as f:
-                label = f.readline().strip("\n")
-        except OSError:
-            label = sub_dir.replace("/", "_")
-
-        LOGGER.info("Running the benchmarking on the %s problem set", label)
-        fit = Fit(
-            options=options, data_dir=data_dir, label=label, checkpointer=cp
-        )
-        results, failed_problems, unselected_minimizers = fit.benchmark()
-
-        # If a result has error flag 4 then the result contains dummy values,
-        # if this is the case for all results then output should not be
-        # produced as results tables won't show meaningful values.
-        all_dummy_results_flag = True
-        for result in results:
-            if result.error_flag != 4:
-                all_dummy_results_flag = False
-                break
-
-        # If the results are an empty list then this means that all minimizers
-        # raise an exception and the tables will produce errors if they run
-        # for that problem set.
-        if not results or all_dummy_results_flag:
-            message = (
-                "\nWARNING: \nThe user chosen options and/or problem "
-                " setup resulted in all minimizers and/or parsers "
-                "raising an exception. Because of this, results for "
-                f"the {label} problem set will not be displayed. "
-                "Please see the logs for more detail on why this is "
-                "the case."
+    try:
+        for sub_dir in problem_sets:
+            # Create full path for the directory that holds a group of
+            # problem definition files
+            data_dir = os.path.join(current_path, sub_dir)
+
+            test_data = glob.glob(data_dir + "/*.*")
+
+            if not test_data:
+                LOGGER.warning("Problem set %s not found", data_dir)
+                continue
+
+            # generate group label/name used for problem set
+            try:
+                with open(
+                    os.path.join(data_dir, "META.txt"), encoding="utf-8"
+                ) as f:
+                    label = f.readline().strip("\n")
+            except OSError:
+                label = sub_dir.replace("/", "_")
+
+            LOGGER.info(
+                "Running the benchmarking on the %s problem set", label
             )
-            LOGGER.warning(message)
-        else:
-            LOGGER.info("Producing output for the %s problem set", label)
-            # Display the runtime and accuracy results in a table
-            group_results_dir, pp_dfs = save_results(
-                group_name=label,
-                results=results,
+            fit = Fit(
                 options=options,
-                failed_problems=failed_problems,
-                unselected_minimizers=unselected_minimizers,
-                config=cp.config,
+                data_dir=data_dir,
+                label=label,
+                checkpointer=cp,
             )
-
-            pp_dfs_all_prob_sets[label] = pp_dfs
-
-            LOGGER.info("Completed benchmarking for %s problem set", sub_dir)
-            group_results_dir = os.path.relpath(
-                path=group_results_dir, start=options.results_dir
-            )
-            result_dir.append(group_results_dir)
-            group_labels.append(label)
-
-    cp.finalise()
+            results, failed_problems, unselected_minimizers = fit.benchmark()
+
+            # If a result has error flag 4 then the result contains dummy
+            # values, if this is the case for all results then output should
+            # not be produced as results tables won't show meaningful values.
+            all_dummy_results_flag = True
+            for result in results:
+                if result.error_flag != 4:
+                    all_dummy_results_flag = False
+                    break
+
+            # If the results are an empty list then this means that all
+            # minimizers raise an exception and the tables will produce
+            # errors if they run for that problem set.
+            if not results or all_dummy_results_flag:
+                message = (
+                    "\nWARNING: \nThe user chosen options and/or problem "
+                    " setup resulted in all minimizers and/or parsers "
+                    "raising an exception. Because of this, results for "
+                    f"the {label} problem set will not be displayed. "
+                    "Please see the logs for more detail on why this is "
+                    "the case."
+                )
+                LOGGER.warning(message)
+            else:
+                LOGGER.info("Producing output for the %s problem set", label)
+                # Display the runtime and accuracy results in a table
+                group_results_dir, pp_dfs = save_results(
+                    group_name=label,
+                    results=results,
+                    options=options,
+                    failed_problems=failed_problems,
+                    unselected_minimizers=unselected_minimizers,
+                    config=cp.config,
+                )
+
+                pp_dfs_all_prob_sets[label] = pp_dfs
+
+                LOGGER.info(
+                    "Completed benchmarking for %s problem set", sub_dir
+                )
+                group_results_dir = os.path.relpath(
+                    path=group_results_dir, start=options.results_dir
+                )
+                result_dir.append(group_results_dir)
+                group_labels.append(label)
+
+    finally:
+        cp.finalise()
 
     # Check result_dir is non empty before producing output
     if not result_dir:


Test patch:
diff --git a/fitbenchmarking/cli/tests/test_main.py b/fitbenchmarking/cli/tests/test_main.py
index d538ad9d..e7d32e74 100644
--- a/fitbenchmarking/cli/tests/test_main.py
+++ b/fitbenchmarking/cli/tests/test_main.py
@@ -4,7 +4,9 @@ This file contains unit tests for the main CLI script
 
 import inspect
 import os
+from json import load
 from pathlib import Path
+from tempfile import TemporaryDirectory
 from unittest import TestCase
 from unittest.mock import patch
 
@@ -14,6 +16,7 @@ from fitbenchmarking.controllers.scipy_controller import ScipyController
 from fitbenchmarking.cost_func.nlls_cost_func import NLLSCostFunc
 from fitbenchmarking.parsing.parser_factory import parse_problem_file
 from fitbenchmarking.utils import exceptions, fitbm_result
+from fitbenchmarking.utils.misc import get_problem_files
 from fitbenchmarking.utils.options import Options
 
 
@@ -67,11 +70,7 @@ class TestMain(TestCase):
         benchmark.return_value = ([], [], {})
 
         with self.assertRaises(exceptions.NoResultsError):
-            main.run(
-                ["examples/benchmark_problems/simple_tests"],
-                os.path.dirname(__file__),
-                debug=True,
-            )
+            main.run(["examples/benchmark_problems/simple_tests"], debug=True)
 
     @patch("fitbenchmarking.cli.main.Fit.benchmark")
     def test_all_dummy_results_produced(self, benchmark):
@@ -98,3 +97,34 @@ class TestMain(TestCase):
         with self.assertRaises(SystemExit) as exp:
             main.main()
         self.assertEqual(exp.exception.code, 1)
+
+    @patch("fitbenchmarking.cli.main.save_results")
+    @patch("fitbenchmarking.utils.misc.get_problem_files")
+    def test_checkpoint_file_on_fail(self, get_problems, save_results):
+        """
+        Checks that the checkpoint file is valid json if there's a crash.
+        """
+        get_problems.side_effect = lambda path: [get_problem_files(path)[0]]
+        save_results.side_effect = RuntimeError(
+            "Exception raised during save..."
+        )
+
+        with TemporaryDirectory() as results_dir:
+            with self.assertRaises(RuntimeError):
+                main.run(
+                    ["examples/benchmark_problems/NIST/low_difficulty"],
+                    additional_options={
+                        "scipy_ls": ["lm-scipy"],
+                        "software": ["scipy_ls"],
+                        "num_runs": 1,
+                        "results_dir": results_dir,
+                    },
+                    debug=True,
+                )
+
+            with open(f"{results_dir}/checkpoint.json", encoding="utf8") as f:
+                # This will fail if the json is invalid
+                contents = load(f)
+
+        # Check that it's not empty
+        self.assertTrue(contents)