From c1d31a6ce4e7ca47325b46637624f166a3dd04d8 Mon Sep 17 00:00:00 2001 From: Fabio Graetz Date: Thu, 14 Dec 2023 14:41:14 +0000 Subject: [PATCH 1/8] Improve error message in workflow compliation when output binding fails Signed-off-by: Fabio Graetz --- flytekit/core/workflow.py | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/flytekit/core/workflow.py b/flytekit/core/workflow.py index 41e421fe69..93d6f93317 100644 --- a/flytekit/core/workflow.py +++ b/flytekit/core/workflow.py @@ -746,14 +746,17 @@ def compile(self, **kwargs): ) workflow_outputs = workflow_outputs[0] t = self.python_interface.outputs[output_names[0]] - b, _ = binding_from_python_std( - ctx, - output_names[0], - self.interface.outputs[output_names[0]].type, - workflow_outputs, - t, - ) - bindings.append(b) + try: + b, _ = binding_from_python_std( + ctx, + output_names[0], + self.interface.outputs[output_names[0]].type, + workflow_outputs, + t, + ) + bindings.append(b) + except Exception as e: + raise AssertionError(f"Failed to bind output {output_names[0]} for function {self.name}.") from e elif len(output_names) > 1: if not isinstance(workflow_outputs, tuple): raise AssertionError("The Workflow specification indicates multiple return values, received only one") @@ -763,14 +766,17 @@ def compile(self, **kwargs): if isinstance(workflow_outputs[i], ConditionalSection): raise AssertionError("A Conditional block (if-else) should always end with an `else_()` clause") t = self.python_interface.outputs[out] - b, _ = binding_from_python_std( - ctx, - out, - self.interface.outputs[out].type, - workflow_outputs[i], - t, - ) - bindings.append(b) + try: + b, _ = binding_from_python_std( + ctx, + out, + self.interface.outputs[out].type, + workflow_outputs[i], + t, + ) + bindings.append(b) + except Exception as e: + raise AssertionError(f"Failed to bind output {out} for function {self.name}.") from e # Save all the things necessary to create an WorkflowTemplate, except for the missing project and domain self._nodes = all_nodes From c559e2649f9ed0de02e9fe0151d491bb7dde664e Mon Sep 17 00:00:00 2001 From: Fabio Graetz Date: Thu, 14 Dec 2023 14:48:35 +0000 Subject: [PATCH 2/8] Include previous exception in error message Signed-off-by: Fabio Graetz --- flytekit/core/workflow.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/flytekit/core/workflow.py b/flytekit/core/workflow.py index 93d6f93317..ca952a8e0c 100644 --- a/flytekit/core/workflow.py +++ b/flytekit/core/workflow.py @@ -756,7 +756,7 @@ def compile(self, **kwargs): ) bindings.append(b) except Exception as e: - raise AssertionError(f"Failed to bind output {output_names[0]} for function {self.name}.") from e + raise AssertionError(f"Failed to bind output {output_names[0]} for function {self.name}: {e}") from e elif len(output_names) > 1: if not isinstance(workflow_outputs, tuple): raise AssertionError("The Workflow specification indicates multiple return values, received only one") @@ -776,7 +776,7 @@ def compile(self, **kwargs): ) bindings.append(b) except Exception as e: - raise AssertionError(f"Failed to bind output {out} for function {self.name}.") from e + raise AssertionError(f"Failed to bind output {out} for function {self.name}: {e}") from e # Save all the things necessary to create an WorkflowTemplate, except for the missing project and domain self._nodes = all_nodes From ae6207eb1d1089973855e2c9abf1bba8586ac6ce Mon Sep 17 00:00:00 2001 From: Fabio Graetz Date: Thu, 14 Dec 2023 15:40:45 +0000 Subject: [PATCH 3/8] Adapt tests Signed-off-by: Fabio Graetz --- tests/flytekit/unit/core/test_type_hints.py | 2 +- tests/flytekit/unit/experimental/test_eager_workflows.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/flytekit/unit/core/test_type_hints.py b/tests/flytekit/unit/core/test_type_hints.py index 9e01b4a619..a68e69b897 100644 --- a/tests/flytekit/unit/core/test_type_hints.py +++ b/tests/flytekit/unit/core/test_type_hints.py @@ -794,7 +794,7 @@ def t1(a: int) -> typing.NamedTuple("OutputsBC", t1_int_output=int, c=str): def t2(a: str) -> str: return a - with pytest.raises(TypeError): + with pytest.raises(AssertionError): @workflow def my_wf(a: int, b: str) -> (int, str): diff --git a/tests/flytekit/unit/experimental/test_eager_workflows.py b/tests/flytekit/unit/experimental/test_eager_workflows.py index 50ed29063c..86a48c677b 100644 --- a/tests/flytekit/unit/experimental/test_eager_workflows.py +++ b/tests/flytekit/unit/experimental/test_eager_workflows.py @@ -9,7 +9,6 @@ from hypothesis import given, settings from flytekit import dynamic, task, workflow -from flytekit.core.type_engine import TypeTransformerFailedError from flytekit.experimental import EagerException, eager from flytekit.types.directory import FlyteDirectory from flytekit.types.file import FlyteFile @@ -213,7 +212,7 @@ async def eager_wf(x: int) -> int: out = await local_wf(x=x) return await double(x=out) - with pytest.raises(TypeTransformerFailedError): + with pytest.raises(AssertionError): asyncio.run(eager_wf(x=x_input)) From 061301fe3f5e1901394e564a3881f6a7788a2db8 Mon Sep 17 00:00:00 2001 From: Fabio Graetz Date: Thu, 14 Dec 2023 15:46:58 +0000 Subject: [PATCH 4/8] Add test that would have caught issue Signed-off-by: Fabio Graetz --- tests/flytekit/unit/core/test_type_hints.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/flytekit/unit/core/test_type_hints.py b/tests/flytekit/unit/core/test_type_hints.py index a68e69b897..1c9579a667 100644 --- a/tests/flytekit/unit/core/test_type_hints.py +++ b/tests/flytekit/unit/core/test_type_hints.py @@ -133,6 +133,15 @@ def my_task() -> str: assert context_manager.FlyteContextManager.size() == 1 +def test_missing_output(): + @workflow + def wf() -> str: + return None + + with pytest.raises(AssertionError, match="Failed to bind output"): + wf.compile() + + def test_engine_file_output(): basic_blob_type = _core_types.BlobType( format="", From c42f2155682bf83b8b8b61d76fee49a00cbc3422 Mon Sep 17 00:00:00 2001 From: Fabio Graetz Date: Thu, 14 Dec 2023 20:22:54 +0000 Subject: [PATCH 5/8] Silence expected mypy warning in test Signed-off-by: Fabio Graetz --- tests/flytekit/unit/core/test_type_hints.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/flytekit/unit/core/test_type_hints.py b/tests/flytekit/unit/core/test_type_hints.py index 1c9579a667..01cf0aebb4 100644 --- a/tests/flytekit/unit/core/test_type_hints.py +++ b/tests/flytekit/unit/core/test_type_hints.py @@ -136,7 +136,7 @@ def my_task() -> str: def test_missing_output(): @workflow def wf() -> str: - return None + return None # type: ignore with pytest.raises(AssertionError, match="Failed to bind output"): wf.compile() From 50ac42fbde0f56cf55aa84251db1caf45b348ae5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabio=20Gr=C3=A4tz?= Date: Tue, 30 Jan 2024 23:00:32 +0100 Subject: [PATCH 6/8] Use FlyteValidationException instead of AssertionError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fabio Grätz --- flytekit/core/workflow.py | 4 ++-- tests/flytekit/unit/core/test_type_hints.py | 3 ++- tests/flytekit/unit/experimental/test_eager_workflows.py | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/flytekit/core/workflow.py b/flytekit/core/workflow.py index ca952a8e0c..0f69fe3321 100644 --- a/flytekit/core/workflow.py +++ b/flytekit/core/workflow.py @@ -756,7 +756,7 @@ def compile(self, **kwargs): ) bindings.append(b) except Exception as e: - raise AssertionError(f"Failed to bind output {output_names[0]} for function {self.name}: {e}") from e + raise FlyteValidationException(f"Failed to bind output {output_names[0]} for function {self.name}: {e}") from e elif len(output_names) > 1: if not isinstance(workflow_outputs, tuple): raise AssertionError("The Workflow specification indicates multiple return values, received only one") @@ -776,7 +776,7 @@ def compile(self, **kwargs): ) bindings.append(b) except Exception as e: - raise AssertionError(f"Failed to bind output {out} for function {self.name}: {e}") from e + raise FlyteValidationException(f"Failed to bind output {out} for function {self.name}: {e}") from e # Save all the things necessary to create an WorkflowTemplate, except for the missing project and domain self._nodes = all_nodes diff --git a/tests/flytekit/unit/core/test_type_hints.py b/tests/flytekit/unit/core/test_type_hints.py index 01cf0aebb4..24fc42ab3b 100644 --- a/tests/flytekit/unit/core/test_type_hints.py +++ b/tests/flytekit/unit/core/test_type_hints.py @@ -34,6 +34,7 @@ from flytekit.core.testing import patch, task_mock from flytekit.core.type_engine import RestrictedTypeError, SimpleTransformer, TypeEngine from flytekit.core.workflow import workflow +from flytekit.exceptions.user import FlyteValidationException from flytekit.models import literals as _literal_models from flytekit.models.core import types as _core_types from flytekit.models.interface import Parameter @@ -138,7 +139,7 @@ def test_missing_output(): def wf() -> str: return None # type: ignore - with pytest.raises(AssertionError, match="Failed to bind output"): + with pytest.raises(FlyteValidationException, match="Failed to bind output"): wf.compile() diff --git a/tests/flytekit/unit/experimental/test_eager_workflows.py b/tests/flytekit/unit/experimental/test_eager_workflows.py index 86a48c677b..6ceef8af32 100644 --- a/tests/flytekit/unit/experimental/test_eager_workflows.py +++ b/tests/flytekit/unit/experimental/test_eager_workflows.py @@ -9,6 +9,7 @@ from hypothesis import given, settings from flytekit import dynamic, task, workflow +from flytekit.exceptions.user import FlyteValidationException from flytekit.experimental import EagerException, eager from flytekit.types.directory import FlyteDirectory from flytekit.types.file import FlyteFile @@ -212,7 +213,7 @@ async def eager_wf(x: int) -> int: out = await local_wf(x=x) return await double(x=out) - with pytest.raises(AssertionError): + with pytest.raises(FlyteValidationException): asyncio.run(eager_wf(x=x_input)) From 8519a374b78ddc5f1b22b60c129c9eeadd7c328a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabio=20Gr=C3=A4tz?= Date: Tue, 30 Jan 2024 23:01:55 +0100 Subject: [PATCH 7/8] Use FlyteValidationException instead of AssertionError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fabio Grätz --- tests/flytekit/unit/core/test_type_hints.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/flytekit/unit/core/test_type_hints.py b/tests/flytekit/unit/core/test_type_hints.py index 24fc42ab3b..8253fc82be 100644 --- a/tests/flytekit/unit/core/test_type_hints.py +++ b/tests/flytekit/unit/core/test_type_hints.py @@ -804,7 +804,7 @@ def t1(a: int) -> typing.NamedTuple("OutputsBC", t1_int_output=int, c=str): def t2(a: str) -> str: return a - with pytest.raises(AssertionError): + with pytest.raises(FlyteValidationException): @workflow def my_wf(a: int, b: str) -> (int, str): From ea8196276678399766135767f828210595804768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabio=20Gr=C3=A4tz?= Date: Wed, 31 Jan 2024 09:53:41 +0100 Subject: [PATCH 8/8] Lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fabio Grätz --- flytekit/core/workflow.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/flytekit/core/workflow.py b/flytekit/core/workflow.py index 0f69fe3321..9d8ce8cdc4 100644 --- a/flytekit/core/workflow.py +++ b/flytekit/core/workflow.py @@ -756,7 +756,9 @@ def compile(self, **kwargs): ) bindings.append(b) except Exception as e: - raise FlyteValidationException(f"Failed to bind output {output_names[0]} for function {self.name}: {e}") from e + raise FlyteValidationException( + f"Failed to bind output {output_names[0]} for function {self.name}: {e}" + ) from e elif len(output_names) > 1: if not isinstance(workflow_outputs, tuple): raise AssertionError("The Workflow specification indicates multiple return values, received only one")