MRG: Merge pull request #468 from octue/enhancement/improve-service-compatibility-checks

Ensure outputs of analyses are always validated against the twine

Author: cortadocodes

octue/cli.py

@@ -152,7 +152,7 @@ def run(service_config, input_dir, output_file, output_manifest_file, monitor_me
         handle_monitor_message=monitor_message_handler,
     )
 
-    click.echo(analysis.output_values)
+    click.echo(json.dumps(analysis.output_values))
 
     if analysis.output_values and output_file:
         if not os.path.exists(os.path.dirname(output_file)):

octue/resources/analysis.py

@@ -85,8 +85,18 @@ def __init__(self, twine, handle_monitor_message=None, **kwargs):
         self.output_location = kwargs.pop("output_location", None)
 
         self._calculate_strand_hashes(strands=strand_kwargs)
+        self._finalised = False
         super().__init__(**kwargs)
 
+    @property
+    def finalised(self):
+        """Check whether the analysis has been finalised (i.e. whether its outputs have been validated and, if an output
+        manifest is produced, its datasets uploaded).
+
+        :return bool:
+        """
+        return self._finalised
+
     def send_monitor_message(self, data):
         """Send a monitor message to the parent that requested the analysis.
 
@@ -120,6 +130,7 @@ def finalise(self, upload_output_datasets_to=None):
             serialised_strands["output_manifest"] = self.output_manifest.to_primitive()
 
         self.twine.validate(**serialised_strands)
+        self._finalised = True
         logger.info("Validated output values and output manifest against the twine.")
 
         if not (upload_output_datasets_to and hasattr(self, "output_manifest")):

octue/runner.py

@@ -189,6 +189,9 @@ def run(
                 logger.error(str(e))
                 raise e
 
+            if not analysis.finalised:
+                analysis.finalise()
+
             return analysis
 
     def _populate_environment_with_google_cloud_secrets(self):

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "octue"
-version = "0.29.5"
+version = "0.29.6"
 description = "A package providing template applications for data services, and a python SDK to the Octue API."
 readme = "README.md"
 authors = ["Marcus Lugg <[email protected]>", "Thomas Clark <[email protected]>"]

tests/cloud/pub_sub/test_service.py

@@ -47,7 +47,8 @@ def mock_app(analysis):
             "input_values_schema": {
                 "type": "object",
                 "required": []
-            }
+            },
+            "output_values_schema": {}
         }
     """
 

tests/test_app_modules/app_class/app.py

@@ -1,5 +1,5 @@
 class App:
-    """A mock app.
+    """A mock app that doesn't call `self.analysis.finalise`.
 
     :param octue.resources.analysis.Analysis analysis:
     :return None:

tests/test_app_modules/app_module/app.py

@@ -4,4 +4,4 @@ def run(analysis):
     :param octue.resources.analysis.Analysis analysis:
     :return None:
     """
-    analysis.output_values = [1, 2, 3, 4]
+    analysis.output_values = {"width": 3}

tests/test_app_modules/app_module_with_output_manifest/app.py

@@ -7,5 +7,5 @@ def run(analysis):
     :param octue.resources.analysis.Analysis analysis:
     :return None:
     """
-    analysis.output_values = [1, 2, 3, 4]
+    analysis.output_values = {"width": 3}
     analysis.output_manifest = Manifest()

tests/test_app_modules/app_with_monitor_message/app.py

@@ -5,4 +5,4 @@ def run(analysis):
     :return None:
     """
     analysis.send_monitor_message({"status": "hello"})
-    analysis.output_values = [1, 2, 3, 4]
+    analysis.output_values = {"width": 3}

tests/test_cli.py

@@ -22,15 +22,15 @@ class TestCLI(BaseTestCase):
     def test_version(self):
         """Ensure the version command works in the CLI."""
         result = CliRunner().invoke(octue_cli, ["--version"])
-        assert "version" in result.output
+        self.assertIn("version", result.output)
 
     def test_help(self):
         """Ensure the help commands works in the CLI."""
         help_result = CliRunner().invoke(octue_cli, ["--help"])
-        assert help_result.output.startswith("Usage")
+        self.assertTrue(help_result.output.startswith("Usage"))
 
         h_result = CliRunner().invoke(octue_cli, ["-h"])
-        assert help_result.output == h_result.output
+        self.assertEqual(help_result.output, h_result.output)
 
 
 class TestRunCommand(BaseTestCase):
@@ -55,7 +55,7 @@ def test_run(self):
                 ],
             )
 
-        assert json.dumps([1, 2, 3, 4]) in result.output
+        self.assertIn(json.dumps({"width": 3}), result.output)
 
     def test_run_with_output_values_file(self):
         """Test that the `run` CLI command runs the given service and stores the output values in a file if the `-o`
@@ -74,20 +74,19 @@ def test_run_with_output_values_file(self):
                 )
 
             with open(temporary_file.name) as f:
-                self.assertEqual(json.load(f), [1, 2, 3, 4])
+                self.assertEqual(json.load(f), {"width": 3})
 
-        assert json.dumps([1, 2, 3, 4]) in result.output
+        self.assertIn(json.dumps({"width": 3}), result.output)
 
     def test_run_with_output_manifest(self):
         """Test that the `run` CLI command runs the given service and stores the output manifest in a file."""
         mock_configurations = (
             ServiceConfiguration(
                 name="test-app",
                 app_source_path=os.path.join(TESTS_DIR, "test_app_modules", "app_module_with_output_manifest"),
-                twine_path=TWINE_FILE_PATH,
-                app_configuration_path="blah.json",
+                twine_path={"input_values_schema": {}, "output_manifest": {"datasets": {}}, "output_values_schema": {}},
             ),
-            AppConfiguration(configuration_values={"n_iterations": 5}),
+            AppConfiguration(),
         )
 
         with tempfile.NamedTemporaryFile(delete=False) as temporary_file:
@@ -104,7 +103,7 @@ def test_run_with_output_manifest(self):
             with open(temporary_file.name) as f:
                 self.assertIn("datasets", json.load(f))
 
-        assert json.dumps([1, 2, 3, 4]) in result.output
+        self.assertIn(json.dumps({"width": 3}), result.output)
 
     def test_run_with_monitor_messages_sent_to_file(self):
         """Test that, when the `--monitor-messages-file` is provided, any monitor messages are written to it."""
@@ -132,7 +131,7 @@ def test_run_with_monitor_messages_sent_to_file(self):
             with open(monitor_messages_file.name) as f:
                 self.assertEqual(json.load(f), [{"status": "hello"}])
 
-        assert json.dumps([1, 2, 3, 4]) in result.output
+        self.assertIn(json.dumps({"width": 3}), result.output)
 
     def test_remote_logger_uri_can_be_set(self):
         """Test that remote logger URI can be set via the CLI and that this is logged locally."""

tests/test_runner.py

@@ -225,18 +225,45 @@ def test_invalid_app_directory(self):
 
     def test_app_can_be_provided_as_a_class(self):
         """Test that apps can be written and provided as a class."""
-        analysis = Runner(app_src=App, twine="{}").run()
+        analysis = Runner(app_src=App, twine={"output_values_schema": {}}).run()
         self.assertEqual(analysis.output_values, "App as a class works!")
 
     def test_app_can_be_provided_as_path_to_module_containing_class_named_app(self):
         """Test that apps can be provided as a path to a module containing a class named "App"."""
-        analysis = Runner(app_src=os.path.join(TESTS_DIR, "test_app_modules", "app_class"), twine="{}").run()
+        analysis = Runner(
+            app_src=os.path.join(TESTS_DIR, "test_app_modules", "app_class"),
+            twine={"output_values_schema": {}},
+        ).run()
+
         self.assertEqual(analysis.output_values, "App as a class works!")
 
     def test_app_can_be_provided_as_a_module_containing_function_named_run(self):
         """Test that apps can be provided as a module containing a function named "run"."""
-        analysis = Runner(app_src=app, twine="{}").run()
-        self.assertEqual(analysis.output_values, [1, 2, 3, 4])
+        analysis = Runner(app_src=app, twine={"output_values_schema": {}}).run()
+        self.assertEqual(analysis.output_values, {"width": 3})
+
+    def test_analysis_finalised_by_runner_if_not_finalised_in_app(self):
+        """Test that analyses are finalised automatically if they're not finalised within their app."""
+        analysis = Runner(app_src=App, twine={"output_values_schema": {}}).run()
+        self.assertTrue(analysis.finalised)
+
+    def test_analysis_not_re_finalised_by_runner_if_finalised_in_app(self):
+        """Test that the `Analysis.finalise` method is not called again if an analysis has already been finalised."""
+
+        def app(analysis):
+            analysis.output_values = {"hello": "world"}
+
+            self.assertFalse(analysis.finalised)
+
+            # Simulate the analysis being finalised while still being able to mock `Analysis.finalise` to count how
+            # many times it's been called.
+            analysis._finalised = True
+
+        with patch("octue.resources.analysis.Analysis.finalise") as mock_finalise:
+            analysis = Runner(app_src=app, twine={"output_values_schema": {}}).run()
+
+        self.assertEqual(mock_finalise.call_count, 0)
+        self.assertTrue(analysis.finalised)
 
 
 class TestRunnerWithRequiredDatasetFileTags(BaseTestCase):
@@ -272,7 +299,8 @@ class TestRunnerWithRequiredDatasetFileTags(BaseTestCase):
                         },
                     }
                 }
-            }
+            },
+            "output_values_schema": {},
         }
     )
 
@@ -355,7 +383,8 @@ def test_validate_input_manifest_with_required_tags_for_remote_tag_template_sche
                         "file_tags_template": {"$ref": schema_url},
                     }
                 }
-            }
+            },
+            "output_values_schema": {},
         }
 
         remote_schema = {
@@ -412,7 +441,8 @@ def test_validate_input_manifest_with_required_tags_in_several_datasets(self):
                         }
                     },
                 }
-            }
+            },
+            "output_values_schema": {},
         }
 
         with tempfile.TemporaryDirectory() as temporary_directory: