From 74ec64201e8df57af5e189eab17a50ddf5e617b0 Mon Sep 17 00:00:00 2001 From: Jared O'Connell Date: Mon, 24 Jun 2024 12:15:16 -0400 Subject: [PATCH 1/7] Detect invalid step return --- src/arcaflow_plugin_sdk/schema.py | 5 ++++ src/arcaflow_plugin_sdk/test_plugin.py | 33 +++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/arcaflow_plugin_sdk/schema.py b/src/arcaflow_plugin_sdk/schema.py index e6e2831..0a26299 100644 --- a/src/arcaflow_plugin_sdk/schema.py +++ b/src/arcaflow_plugin_sdk/schema.py @@ -5989,6 +5989,11 @@ def __call__( input.validate(params, tuple(["input"])) # Run the step result = self._handler(step_local_data.initialized_object, params) + if not isinstance(result, tuple): + raise BadArgumentException( + "The step returned type {};".format(type(result)) + + " expected a tuple with two values: output ID string and a step-specific value." + ) if len(result) != 2: raise BadArgumentException( "The step returned {} results instead of 2. Did your step" diff --git a/src/arcaflow_plugin_sdk/test_plugin.py b/src/arcaflow_plugin_sdk/test_plugin.py index 3ca1896..d73a755 100644 --- a/src/arcaflow_plugin_sdk/test_plugin.py +++ b/src/arcaflow_plugin_sdk/test_plugin.py @@ -5,15 +5,16 @@ import unittest from arcaflow_plugin_sdk import plugin +from arcaflow_plugin_sdk import schema @dataclasses.dataclass -class StdoutTestInput: +class EmptyTestInput: pass @dataclasses.dataclass -class StdoutTestOutput: +class EmptyTestOutput: pass @@ -21,13 +22,13 @@ class StdoutTestOutput: "stdout-test", "Stdout test", "A test for writing to stdout.", - {"success": StdoutTestOutput}, + {"success": EmptyTestOutput}, ) def stdout_test_step( - input: StdoutTestInput, -) -> typing.Tuple[str, StdoutTestOutput]: + _: EmptyTestInput, +) -> typing.Tuple[str, EmptyTestOutput]: print("Hello world!") - return "success", StdoutTestOutput() + return "success", EmptyTestOutput() class StdoutTest(unittest.TestCase): @@ -52,5 +53,25 @@ def cleanup(): self.assertEqual("Hello world!\n", e.getvalue()) +@plugin.step( + "incorrect-return", + "Incorrect Return", + "A test that doesn't include the output ID.", + {"success": EmptyTestOutput}, +) +def incorrect_return_step( + _: EmptyTestInput, +): # Skip return type, since we're purposefully not doing it right. + return EmptyTestOutput() + + +class CallStepTest(unittest.TestCase): + def test_incorrect_return_args_count(self): + s = plugin.build_schema(incorrect_return_step) + + with self.assertRaises(schema.BadArgumentException): + s.call_step(self.id(), "incorrect-return", EmptyTestInput()) + + if __name__ == "__main__": unittest.main() From 05395d76f9c16cdc95f630fdebf3517021250d79 Mon Sep 17 00:00:00 2001 From: Jared O'Connell Date: Mon, 24 Jun 2024 12:23:16 -0400 Subject: [PATCH 2/7] Change import format --- src/arcaflow_plugin_sdk/test_plugin.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/arcaflow_plugin_sdk/test_plugin.py b/src/arcaflow_plugin_sdk/test_plugin.py index d73a755..2180a07 100644 --- a/src/arcaflow_plugin_sdk/test_plugin.py +++ b/src/arcaflow_plugin_sdk/test_plugin.py @@ -4,8 +4,7 @@ import typing import unittest -from arcaflow_plugin_sdk import plugin -from arcaflow_plugin_sdk import schema +from arcaflow_plugin_sdk import plugin, schema @dataclasses.dataclass From bf8fdef214bb8bc5f1d688a716f573a43d4ee40f Mon Sep 17 00:00:00 2001 From: Jared O'Connell Date: Mon, 24 Jun 2024 12:44:34 -0400 Subject: [PATCH 3/7] Fix linting error --- src/arcaflow_plugin_sdk/schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/arcaflow_plugin_sdk/schema.py b/src/arcaflow_plugin_sdk/schema.py index 0a26299..464c6b0 100644 --- a/src/arcaflow_plugin_sdk/schema.py +++ b/src/arcaflow_plugin_sdk/schema.py @@ -5991,8 +5991,8 @@ def __call__( result = self._handler(step_local_data.initialized_object, params) if not isinstance(result, tuple): raise BadArgumentException( - "The step returned type {};".format(type(result)) - + " expected a tuple with two values: output ID string and a step-specific value." + "The step returned type {}; expected a tuple with".format(type(result)) + + " two values: output ID string and a step-specific value." ) if len(result) != 2: raise BadArgumentException( From a00bbf111610880c3bed46c5764f334c4a6b681f Mon Sep 17 00:00:00 2001 From: Jared O'Connell Date: Mon, 24 Jun 2024 12:53:53 -0400 Subject: [PATCH 4/7] Fix linting error --- src/arcaflow_plugin_sdk/schema.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/arcaflow_plugin_sdk/schema.py b/src/arcaflow_plugin_sdk/schema.py index 464c6b0..e807e0f 100644 --- a/src/arcaflow_plugin_sdk/schema.py +++ b/src/arcaflow_plugin_sdk/schema.py @@ -5991,8 +5991,9 @@ def __call__( result = self._handler(step_local_data.initialized_object, params) if not isinstance(result, tuple): raise BadArgumentException( - "The step returned type {}; expected a tuple with".format(type(result)) - + " two values: output ID string and a step-specific value." + "The step returned type {};".format(type(result)) + + " expected a tuple with two values: output ID string" + " and a step-specific value." ) if len(result) != 2: raise BadArgumentException( From fd422ae4ea1306aa53c909f664e1fc1a66bb8abe Mon Sep 17 00:00:00 2001 From: Jared O'Connell Date: Mon, 24 Jun 2024 15:03:01 -0400 Subject: [PATCH 5/7] Addressed review comments --- src/arcaflow_plugin_sdk/schema.py | 11 ++++++----- src/arcaflow_plugin_sdk/test_plugin.py | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/arcaflow_plugin_sdk/schema.py b/src/arcaflow_plugin_sdk/schema.py index e807e0f..0126643 100644 --- a/src/arcaflow_plugin_sdk/schema.py +++ b/src/arcaflow_plugin_sdk/schema.py @@ -5991,14 +5991,15 @@ def __call__( result = self._handler(step_local_data.initialized_object, params) if not isinstance(result, tuple): raise BadArgumentException( - "The step returned type {};".format(type(result)) - + " expected a tuple with two values: output ID string" - " and a step-specific value." + f"The implementation of step {run_id}/{self.id} returned" + f" type {type(result)}; expected a tuple with two" + " values: output ID string and a step-specific value." ) if len(result) != 2: raise BadArgumentException( - "The step returned {} results instead of 2. Did your step" - " return the correct results?".format(len(result)) + f"The implementation of step {run_id}/{self.id} returned" + f"{len(result)} results instead of 2. Did your step" + " return the correct results?" ) output_id, output_data = result if output_id not in self.outputs: diff --git a/src/arcaflow_plugin_sdk/test_plugin.py b/src/arcaflow_plugin_sdk/test_plugin.py index 2180a07..e6622eb 100644 --- a/src/arcaflow_plugin_sdk/test_plugin.py +++ b/src/arcaflow_plugin_sdk/test_plugin.py @@ -55,7 +55,7 @@ def cleanup(): @plugin.step( "incorrect-return", "Incorrect Return", - "A test that doesn't include the output ID.", + "A step that returns a bad output which omits the output ID.", {"success": EmptyTestOutput}, ) def incorrect_return_step( From 86f5bed7eaaf72b5d02edcc1d921121c4e7c8dd6 Mon Sep 17 00:00:00 2001 From: Jared O'Connell Date: Mon, 24 Jun 2024 16:21:00 -0400 Subject: [PATCH 6/7] Addressed review comments --- src/arcaflow_plugin_sdk/schema.py | 3 +-- src/arcaflow_plugin_sdk/test_plugin.py | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/arcaflow_plugin_sdk/schema.py b/src/arcaflow_plugin_sdk/schema.py index 0126643..ab9ac7c 100644 --- a/src/arcaflow_plugin_sdk/schema.py +++ b/src/arcaflow_plugin_sdk/schema.py @@ -5998,8 +5998,7 @@ def __call__( if len(result) != 2: raise BadArgumentException( f"The implementation of step {run_id}/{self.id} returned" - f"{len(result)} results instead of 2. Did your step" - " return the correct results?" + f"{len(result)} results instead of 2. Got {result}." ) output_id, output_data = result if output_id not in self.outputs: diff --git a/src/arcaflow_plugin_sdk/test_plugin.py b/src/arcaflow_plugin_sdk/test_plugin.py index e6622eb..bb6f7d2 100644 --- a/src/arcaflow_plugin_sdk/test_plugin.py +++ b/src/arcaflow_plugin_sdk/test_plugin.py @@ -60,7 +60,8 @@ def cleanup(): ) def incorrect_return_step( _: EmptyTestInput, -): # Skip return type, since we're purposefully not doing it right. +) -> typing.Tuple[str, EmptyTestOutput]: + # noinspection PyTypeChecker return EmptyTestOutput() From c390fdff6362517cf6b59e0f8432142db7448aa8 Mon Sep 17 00:00:00 2001 From: Jared O'Connell Date: Mon, 24 Jun 2024 16:47:50 -0400 Subject: [PATCH 7/7] Addressed review comments --- src/arcaflow_plugin_sdk/schema.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/arcaflow_plugin_sdk/schema.py b/src/arcaflow_plugin_sdk/schema.py index ab9ac7c..c12382b 100644 --- a/src/arcaflow_plugin_sdk/schema.py +++ b/src/arcaflow_plugin_sdk/schema.py @@ -5994,6 +5994,7 @@ def __call__( f"The implementation of step {run_id}/{self.id} returned" f" type {type(result)}; expected a tuple with two" " values: output ID string and a step-specific value." + f"\nValue returned: {result}" ) if len(result) != 2: raise BadArgumentException(