Send exec status independent of report permissions (#561)

* Send exec status independent of report permissions * Add note regarding exec status being sent * Remove unused variable * Adjust tests to recent changes * add mockfs to test * Mock ReportHandler * Mock watchdog observer * Send reports only if execution is not for testing * Fix linter issue * Simplify logic for not sending test reports. Add test * mock observer for new test * Update cli/medperf/commands/dataset/prepare.py Co-authored-by: Viacheslav Kukushkin <[email protected]> * Update cli/medperf/commands/dataset/prepare.py Co-authored-by: Viacheslav Kukushkin <[email protected]> * Update cli/medperf/tests/commands/dataset/test_prepare.py Co-authored-by: Viacheslav Kukushkin <[email protected]> * Remove unnecessary check for test dataset --------- Co-authored-by: Viacheslav Kukushkin <[email protected]>
mlcommons · Jul 10, 2024 · 19f257d · 19f257d
1 parent fb7c54e
commit 19f257d
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 39 deletions.
diff --git a/cli/medperf/commands/dataset/prepare.py b/cli/medperf/commands/dataset/prepare.py
@@ -165,8 +165,7 @@ def setup_parameters(self):
 
     def run_prepare(self):
         report_sender = ReportSender(self)
-        if self.allow_sending_reports:
-            report_sender.start()
+        report_sender.start()
 
         prepare_params = {
             "data_path": self.raw_data_path,
@@ -190,18 +189,15 @@ def run_prepare(self):
                 )
         except Exception as e:
             # Inform the server that a failure occured
-            if self.allow_sending_reports:
-                report_sender.stop("failed")
+            report_sender.stop("failed")
             raise e
-        except KeyboardInterrupt:
+        except KeyboardInterrupt as e:
             # Inform the server that the process is interrupted
-            if self.allow_sending_reports:
-                report_sender.stop("interrupted")
-            raise
+            report_sender.stop("interrupted")
+            raise e
 
         self.ui.print("> Cube execution complete")
-        if self.allow_sending_reports:
-            report_sender.stop("finished")
+        report_sender.stop("finished")
 
     def run_sanity_check(self):
         sanity_check_timeout = config.sanity_check_timeout
@@ -313,8 +309,12 @@ def prompt_for_report_sending_approval(self):
         dict_pretty_print(example)
 
         msg = (
-            " \nDo you approve the automatic submission of summaries similar to the one above"
-            + " to the MedPerf Server throughout the preparation process?[Y/n]"
+            "\nYou can decide whether or not to send information about your dataset preparation"
+            + "\nProgress. Keep in mind that information about the execution status of the pipeline"
+            + "\nwill be sent regardless (whether the pipeline is running, finished or failed)"
+            + "\nto identify issues with the preparation procedure. Do you approve the automatic"
+            + "\nsubmission of summaries similar to the one above to the MedPerf Server throughout"
+            + "\nthe preparation process?[Y/n]"
         )
 
         self.allow_sending_reports = approval_prompt(msg)
@@ -327,7 +327,12 @@ def send_report(self, report_metadata):
             return self._send_report(report_metadata)
 
     def _send_report(self, report_metadata):
-        report_status_dict = self.__generate_report_dict()
+        if self.dataset.for_test:
+            # Test datasets don't have a registration on the server
+            return
+        report_status_dict = {}
+        if self.allow_sending_reports:
+            report_status_dict = self.__generate_report_dict()
         report = {"progress": report_status_dict, **report_metadata}
         if report == self.dataset.report:
             # Watchdog may trigger an event even if contents didn't change

diff --git a/cli/medperf/tests/commands/dataset/test_prepare.py b/cli/medperf/tests/commands/dataset/test_prepare.py
@@ -8,7 +8,7 @@
 
 from medperf.tests.mocks.dataset import TestDataset
 from medperf.tests.mocks.cube import TestCube
-from medperf.commands.dataset.prepare import DataPreparation
+from medperf.commands.dataset.prepare import DataPreparation, Observer
 
 PATCH_REGISTER = "medperf.commands.dataset.prepare.{}"
 
@@ -54,37 +54,58 @@ def test_get_prep_cube_downloads_cube_file(mocker, data_preparation, cube):
     spy.assert_called_once()
 
 
+@pytest.mark.parametrize("dataset_for_test", [False, True])
+def test_prepare_with_test_data_doesnt_send_reports(
+        mocker, data_preparation, dataset_for_test, cube, comms, fs
+):
+    # Arrange
+    data_preparation.dataset.for_test = dataset_for_test
+    mocker.patch.object(cube, "run")
+    mocker.patch.object(data_preparation.dataset, "write")
+    mocked_obs = mocker.create_autospec(spec=Observer)
+    mocker.patch(PATCH_REGISTER.format("Observer", side_effect=mocked_obs))
+    send_report_spy = mocker.patch.object(comms, "update_dataset")
+
+    # Act
+    data_preparation.run_prepare()
+
+    # Assert
+    if dataset_for_test:
+        send_report_spy.assert_not_called()
+    else:
+        send_report_spy.assert_called()
+
+
 @pytest.mark.parametrize("allow_sending_reports", [False, True])
 def test_prepare_runs_then_stops_report_handler(
-    mocker, data_preparation, allow_sending_reports, cube
+    mocker, data_preparation, allow_sending_reports, cube, comms, fs
 ):
     # Arrange
     data_preparation.allow_sending_reports = allow_sending_reports
     mocker.patch.object(cube, "run")
-    start_spy = mocker.patch(PATCH_REGISTER.format("ReportSender.start"))
-    stop_spy = mocker.patch(PATCH_REGISTER.format("ReportSender.stop"))
+    mocker.patch.object(data_preparation.dataset, "write")
+    mocked_obs = mocker.create_autospec(spec=Observer)
+    mocker.patch(PATCH_REGISTER.format("Observer"), side_effect=mocked_obs)
+    gen_report_spy = mocker.patch(PATCH_REGISTER.format("DataPreparation._DataPreparation__generate_report_dict"))
 
     # Act
     data_preparation.run_prepare()
 
     # Assert
     if allow_sending_reports:
-        start_spy.assert_called_once()
-        stop_spy.assert_called_once_with("finished")
+        gen_report_spy.assert_called()
     else:
-        start_spy.assert_not_called()
-        stop_spy.assert_not_called()
+        gen_report_spy.assert_not_called()
 
 
-@pytest.mark.parametrize("allow_sending_reports", [False, True])
 def test_prepare_runs_then_stops_report_handler_on_failure(
-    mocker, data_preparation, allow_sending_reports, cube
+    mocker, data_preparation, cube
 ):
     # Arrange
     def _failure_run(*args, **kwargs):
         raise Exception()
 
-    data_preparation.allow_sending_reports = allow_sending_reports
+    data_preparation.allow_sending_reports = True
     mocker.patch.object(cube, "run", side_effect=_failure_run)
     start_spy = mocker.patch(PATCH_REGISTER.format("ReportSender.start"))
     stop_spy = mocker.patch(PATCH_REGISTER.format("ReportSender.stop"))
@@ -94,23 +115,18 @@ def _failure_run(*args, **kwargs):
         data_preparation.run_prepare()
 
     # Assert
-    if allow_sending_reports:
-        start_spy.assert_called_once()
-        stop_spy.assert_called_once_with("failed")
-    else:
-        start_spy.assert_not_called()
-        stop_spy.assert_not_called()
+    start_spy.assert_called_once()
+    stop_spy.assert_called_once_with("failed")
 
 
-@pytest.mark.parametrize("allow_sending_reports", [False, True])
 def test_prepare_runs_then_stops_report_handler_on_interrupt(
-    mocker, data_preparation, allow_sending_reports, cube
+    mocker, data_preparation, cube
 ):
     # Arrange
     def _failure_run(*args, **kwargs):
         raise KeyboardInterrupt()
 
-    data_preparation.allow_sending_reports = allow_sending_reports
+    data_preparation.allow_sending_reports = True
     mocker.patch.object(cube, "run", side_effect=_failure_run)
     start_spy = mocker.patch(PATCH_REGISTER.format("ReportSender.start"))
     stop_spy = mocker.patch(PATCH_REGISTER.format("ReportSender.stop"))
@@ -120,12 +136,8 @@ def _failure_run(*args, **kwargs):
         data_preparation.run_prepare()
 
     # Assert
-    if allow_sending_reports:
-        start_spy.assert_called_once()
-        stop_spy.assert_called_once_with("interrupted")
-    else:
-        start_spy.assert_not_called()
-        stop_spy.assert_not_called()
+    start_spy.assert_called_once()
+    stop_spy.assert_called_once_with("interrupted")
 
 
 @pytest.mark.parametrize("report_specified", [False, True])