MRG: Merge pull request #48 from octue/fix/empty-analysis-attributes

Prevent imaginary/empty attributes being retrieved from Analysis

Author: cortadocodes

octue/resources/analysis.py

@@ -2,7 +2,6 @@
 import logging
 
 from octue.definitions import OUTPUT_STRANDS
-from octue.exceptions import ProtectedAttributeException
 from octue.mixins import Identifiable, Loggable, Serialisable, Taggable
 from octue.resources.manifest import Manifest
 from octue.utils.encoders import OctueJSONEncoder
@@ -62,27 +61,11 @@ def __init__(self, twine, skip_checks=False, **kwargs):
         # Pop any possible strand data sources before init superclasses (and tie them to protected attributes)
         strand_kwargs = ((name, kwargs.pop(name, None)) for name in ALL_STRANDS)
         for strand_name, strand_data in strand_kwargs:
-            self.__setattr__(f"_{strand_name}", strand_data)
+            setattr(self, f"{strand_name}", strand_data)
 
         # Init superclasses
         super().__init__(**kwargs)
 
-    def __setattr__(self, name, value):
-        """ Override setters for protected attributes (the strand contents may change, but the strands themselves
-        shouldn't be changed after instantiation)
-        """
-        if name in ALL_STRANDS:
-            raise ProtectedAttributeException(f"You cannot set {name} on an instantiated Analysis")
-
-        super().__setattr__(name, value)
-
-    def __getattr__(self, name):
-        """ Override public getters to point to protected attributes (the strand contents may change, but the strands
-        themselves shouldn't be changed after instantiation)
-        """
-        if name in ALL_STRANDS:
-            return getattr(self, f"_{name}", None)
-
     def finalise(self, output_dir=None):
         """ Validates and serialises output_values and output_manifest, optionally writing them to files
 

octue/runner.py

@@ -109,8 +109,8 @@ def _update_manifest_path(manifest, pathname):
             if pathname.endswith(".json"):
                 manifest.path = os.path.split(pathname)[0]
 
-            # Otherwise do nothing and rely on manifest having its path variable set already
-            return manifest
+        # Otherwise do nothing and rely on manifest having its path variable set already
+        return manifest
 
     def run(
         self,

octue/templates/template-using-manifests/app.py

@@ -20,7 +20,7 @@ def run(analysis, *args, **kwargs):
     """
 
     # You can use the attached logger to record debug statements, general information, warnings or errors
-    analysis.logger.info(f"Starting clean up of files in {analysis.input_dir}")
+    analysis.logger.info(f"Starting clean up of files in {analysis.input_manifest.absolute_path}")
 
     # Get the configuration value for our time averaging window (or if not present, use the default specified in
     # the twine)

setup.py

@@ -17,7 +17,7 @@
 
 setup(
     name="octue",
-    version="0.1.5",
+    version="0.1.6",
     py_modules=["cli"],
     install_requires=["click>=7.1.2", "twined==0.0.13"],  # Dev note: you also need to bump twined in tox.ini
     url="https://www.github.com/octue/octue-sdk-python",

tests/resources/test_analysis.py

@@ -1,6 +1,3 @@
-import os
-
-from octue import exceptions
 from octue.resources import Analysis
 from twined import Twine
 from ..base import BaseTestCase
@@ -20,23 +17,11 @@ def test_instantiate_analysis_with_twine(self):
         analysis = Analysis(twine=Twine(source="{}"))
         self.assertEqual(analysis.__class__.__name__, "Analysis")
 
-    def test_protected_setter(self):
-        """ Ensures that protected attributes can't be set
+    def test_non_existent_attributes_cannot_be_retrieved(self):
+        """ Ensure attributes that don't exist on Analysis aren't retrieved as None and instead raise an error. See
+        https://github.com/octue/octue-sdk-python/issues/45 for reasoning behind adding this.
         """
-        analysis = Analysis(twine="{}")
-        with self.assertRaises(exceptions.ProtectedAttributeException) as error:
-            analysis.configuration_values = {}
-
-        self.assertIn("You cannot set configuration_values on an instantiated Analysis", error.exception.args[0])
+        analysis = Analysis(twine=Twine(source="{}"))
 
-    def test_protected_getter(self):
-        """ Ensures that protected attributes can't be set
-        """
-        analysis = Analysis(
-            twine=str(os.path.join(self.data_path, "twines", "valid_schema_twine.json")),
-            configuration_values={"n_iterations": 5},
-            input_values={"height": 5},
-            output_values={},
-        )
-        cfg = analysis.configuration_values
-        self.assertIn("n_iterations", cfg.keys())
+        with self.assertRaises(AttributeError):
+            analysis.furry_purry_cat

tests/test_runner.py

@@ -3,6 +3,10 @@
 from .base import BaseTestCase
 
 
+def mock_app(analysis):
+    pass
+
+
 class RunnerTestCase(BaseTestCase):
     def test_instantiate_runner(self):
         """ Ensures that runner whose twine requires configuration can be instantiated
@@ -27,10 +31,7 @@ def test_run_with_configuration_passes(self):
             configuration_values="{}",
         )
 
-        def fcn(analysis):
-            pass
-
-        runner.run(fcn)
+        runner.run(mock_app)
 
     def test_instantiation_without_configuration_fails(self):
         """ Ensures that runner can be instantiated with a string that points to a path
@@ -72,11 +73,8 @@ def test_run_output_values_validation(self):
         )
 
         # Test for failure with an incorrect output
-        def fcn(analysis):
-            pass
-
         with self.assertRaises(twined.exceptions.InvalidValuesContents) as error:
-            runner.run(fcn).finalise()
+            runner.run(mock_app).finalise()
 
         self.assertIn("'n_iterations' is a required property", error.exception.args[0])
 
@@ -112,13 +110,32 @@ def test_exception_raised_when_strand_data_missing(self):
             configuration_values={"n_iterations": 5},
         )
 
-        def fcn(analysis):
-            pass
-
         with self.assertRaises(twined.exceptions.TwineValueException) as error:
-            runner.run(fcn)
+            runner.run(mock_app)
 
         self.assertIn(
             "The 'input_values' strand is defined in the twine, but no data is provided in sources",
             error.exception.args[0],
         )
+
+    def test_output_manifest_is_not_none(self):
+        """ Ensure the output manifest of an analysis is not None if an output manifest is defined in the Twine. """
+        runner = Runner(
+            twine="""
+                {
+                    "output_manifest": [
+                        {
+                            "key": "open_foam_result",
+                            "purpose": "A dataset containing solution fields of an openfoam case."
+                        },
+                        {
+                            "key": "airfoil_cp_values",
+                            "purpose": "A file containing cp values"
+                        }
+                    ]
+                }
+            """
+        )
+
+        analysis = runner.run(mock_app)
+        self.assertIsNotNone(analysis.output_manifest)