fix: add FlyteValueException handling and clean up exit_handler calls #3036

vincent0426 · 2025-01-06T05:11:03Z

Tracking issue

Why are the changes needed?

Currently, if a task returns a value but does not specify a return type, the return value will be empty instead of raising an error.

It is expected to raise an error if a function returns a value but no return type is specified.

This should throw an error, but it doesn't

pyflyte run example.py greeting_length --greeting="hello"

@task()
def greeting_length(greeting: str):
    """A task the counts the length of a greeting."""
    return len(greeting)

What changes were proposed in this pull request?

Add conditions check to throw an error if native_outputs have value but no output interface is specified

How was this patch tested?

Setup process

pyflyte run example.py greeting_length --greeting="hello"

@task()
def greeting_length(greeting: str):
    """A task the counts the length of a greeting."""
    return len(greeting)

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

Summary by Bito

Implementation of enhanced error handling for tasks with undefined/unspecified return types, introducing FlyteValueException for validation of native outputs. Added comprehensive test cases covering regular returns, optional types, and error scenarios. Enhanced test coverage for task return type handling mechanisms and improved validation in test files to prevent silent failures. Optimized exit_handler calls in VSCode and Jupyter environments for better type safety.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 1

Signed-off-by: Vincent <0426vincent@gmail.com>

flyte-bot · 2025-01-06T05:11:16Z

Code Review Agent Run #a5368c

Actionable Suggestions - 1

flytekit/interactive/vscode_lib/decorator.py - 1
- Consider handling exit_handler return value · Line 459-459

Review Details

Files reviewed - 3 · Commit Range: ea17a4d..ea17a4d
- flytekit/core/base_task.py
- flytekit/interactive/vscode_lib/decorator.py
- plugins/flytekit-flyteinteractive/flytekitplugins/flyteinteractive/jupyter_lib/decorator.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

vincent0426 · 2025-01-06T05:11:28Z

plugins/flytekit-flyteinteractive/flytekitplugins/flyteinteractive/jupyter_lib/decorator.py

@@ -168,7 +168,7 @@ def execute(self, *args, **kwargs):

        write_example_notebook(task_function=self.task_function, notebook_dir=self.notebook_dir)

-        return exit_handler(
+        exit_handler(


Remove the return for both Jupyter and VS Code. I'm not sure if this will cause any side effects. If not removed, native_output will receive the mock function return from execute , which will then throw an error as the output interface is empty.

We cannot remove return here. Where did you see the error?

Hi @pingsutw

flytekit/flytekit/core/base_task.py

Lines 779 to 783 in ea17a4d

if native_outputs is not None and len(list(self._outputs_interface.keys())) == 0:

raise FlyteValueException(

native_outputs,

f"Interface has {len(self.python_interface.outputs)} outputs.",

)

the following tests will fail as they would return the mock handler but the output interface expects to have 0 outputs

pytest -sv tests/flytekit/unit/interactive/test_flyteinteractive_vscode.py::test_vscode_remote_execution

flytekit/tests/flytekit/unit/interactive/test_flyteinteractive_vscode.py

Lines 139 to 142 in 9c2caa1

@task

@vscode

def t():

return

this one should fail, too

flytekit/plugins/flytekit-flyteinteractive/tests/test_flyteinteractive_jupyter.py

Lines 38 to 41 in 9c2caa1

@task

@jupyter

def t():

return

ah, you can add return type to that function.

@task @vscode def t() -> str: return "hello"

Adding -> str might require modifying the return value of exit_handler() as t() may be overridden by the decorator?

mock.patch( "flytekit.interactive.vscode_lib.decorator.exit_handler", return_value="Hello, world!",

However, this will further lead to other errors, which I don't think is solvable as @VScode may not always return exit_handler(). Thanks.

full error log

vscode_patches = (, , <MagicMock...10240'>, , , ...)
mock_local_execution =

def test_vscode_local_execution(vscode_patches, mock_local_execution): ( mock_process, mock_prepare_interactive_python, mock_exit_handler, mock_download_vscode, mock_signal, mock_prepare_resume_task_python, mock_prepare_launch_json, ) = vscode_patches @task @vscode def t() -> str: return @workflow def wf(): t()

wf()

tests/flytekit/unit/interactive/test_flyteinteractive_vscode.py:149:

flytekit/core/workflow.py:308: in call
raise exc
flytekit/core/workflow.py:301: in call
return flyte_entity_call_handler(self, *args, **input_kwargs)
flytekit/core/promise.py:1526: in flyte_entity_call_handler
return cast(LocallyExecutable, entity).local_execute(ctx, **kwargs)
flytekit/core/workflow.py:327: in local_execute
function_outputs = self.execute(**kwargs_literals)
flytekit/core/workflow.py:838: in execute
return self._workflow_function(**kwargs)
tests/flytekit/unit/interactive/test_flyteinteractive_vscode.py:147: in wf
t()
flytekit/core/base_task.py:365: in call
return flyte_entity_call_handler(self, *args, **kwargs) # type: ignore
flytekit/core/promise.py:1526: in flyte_entity_call_handler
return cast(LocallyExecutable, entity).local_execute(ctx, **kwargs)
flytekit/core/base_task.py:345: in local_execute
outputs_literal_map = self.sandbox_execute(ctx, input_literal_map)
flytekit/core/base_task.py:422: in sandbox_execute
return self.dispatch_execute(ctx, input_literal_map)
flytekit/core/base_task.py:787: in dispatch_execute
literals_map, native_outputs_as_map = run_sync(
flytekit/utils/asyn.py:106: in run_sync
return self._runner_map[name].run(coro)
flytekit/utils/asyn.py:85: in run
res = fut.result(None)
/opt/homebrew/Cellar/python@3.12/3.12.8/Frameworks/Python.framework/Versions/3.12/lib/python3.12/concurrent/futures/_base.py:456: in result
return self.__get_result()
/opt/homebrew/Cellar/python@3.12/3.12.8/Frameworks/Python.framework/Versions/3.12/lib/python3.12/concurrent/futures/_base.py:401: in __get_result
raise self._exception
flytekit/core/base_task.py:656: in _output_to_literal_map
raise e
flytekit/core/type_engine.py:1389: in async_to_literal
cls.to_literal_checks(python_val, python_type, expected)

cls = <class 'flytekit.core.type_engine.TypeEngine'>, python_val = None, python_type = <class 'str'>
expected = Flyte Serialized object (LiteralType):
simple: 3

@classmethod def to_literal_checks(cls, python_val: typing.Any, python_type: Type[T], expected: LiteralType): from flytekit.core.promise import VoidPromise if isinstance(python_val, VoidPromise): raise AssertionError( f"Outputs of a non-output producing task {python_val.task_name} cannot be passed to another task." ) if isinstance(python_val, tuple): raise AssertionError( "Tuples are not a supported type for individual values in Flyte - got a tuple -" f" {python_val}. If using named tuple in an inner task, please, de-reference the" "actual attribute that you want to use. For example, in NamedTuple('OP', x=int) then" "return v.x, instead of v, even if this has a single element" ) if ( (python_val is None and python_type != type(None)) and expected and expected.union_type is None and python_type is not Any ):

raise TypeTransformerFailedError(f"Python value cannot be None, expected {python_type}/{expected}")

E flytekit.core.type_engine.TypeTransformerFailedError: Failed to convert outputs of task 'channelexec.test_flyteinteractive_vscode.t' at position 0.
E Failed to convert type <class 'NoneType'> to type <class 'str'>.
E Error Message: Python value cannot be None, expected <class 'str'>/Flyte Serialized object (LiteralType):
E simple: 3.

flytekit/core/type_engine.py:1330: TypeTransformerFailedError

Why not just mock the return value to None in the unit test? (vscode_patches function of test_flyteinteractive_vscode.py)

flyte-bot · 2025-01-06T05:14:30Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Bug Fix - Enhanced Error Handling for Task Return Types	- `base_task.py` - Added FlyteValueException handling for tasks with undefined return types - `test_task_return.py` - Added new test cases for task return type validation
Feature Improvement - Type Safety Enhancements	- `test_elastic_task.py` - Added return type annotation for test_task - `test_plugin.py` - Added complete return type annotation for DataFrame function - `generic_idl_flytetypes.py` - Added return type annotation for DC type - `msgpack_idl_flytetypes.py` - Added return type annotation for DC type
Other Improvements - Exit Handler Optimization	- `test_flyteinteractive_jupyter.py` - Added return_value parameter to exit_handler mock - `test_flyteinteractive_vscode.py` - Added return_value parameter to exit_handler mock - `test_flyteinteractive_vscode.py` - Added return_value parameter to exit_handler mock

flyte-bot · 2025-01-06T05:14:32Z

flytekit/interactive/vscode_lib/decorator.py

@@ -456,7 +456,7 @@ def execute(self, *args, **kwargs):
        # 5. Prepare the launch.json
        prepare_launch_json()

-        return exit_handler(
+        exit_handler(


Consider handling exit_handler return value

The exit_handler function returns a value but the return value is not being captured. Consider capturing the return value or handling it appropriately. A similar issue was also found in plugins/flytekit-flyteinteractive/flytekitplugins/flyteinteractive/jupyter_lib/decorator.py (line 171).

Code suggestion

Check the AI-generated fix before applying

Suggested change

exit_handler(

return exit_handler(

Code Review Run #a5368c

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

Signed-off-by: Vincent <0426vincent@gmail.com>

codecov · 2025-01-08T01:02:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.54%. Comparing base (0ad84f3) to head (912cf34).
Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3036      +/-   ##
==========================================
- Coverage   82.79%   79.54%   -3.26%     
==========================================
  Files           3      202     +199     
  Lines         186    21359   +21173     
  Branches        0     2746    +2746     
==========================================
+ Hits          154    16989   +16835     
- Misses         32     3602    +3570     
- Partials        0      768     +768

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flyte-bot · 2025-01-08T01:56:19Z

Code Review Agent Run #0101b5

Actionable Suggestions - 0

Additional Suggestions - 1

plugins/flytekit-flyteinteractive/tests/test_flyteinteractive_jupyter.py - 1
- Consider necessity of explicit None return · Line 31-31

Review Details

Files reviewed - 4 · Commit Range: ea17a4d..60f4732
- flytekit/interactive/vscode_lib/decorator.py
- plugins/flytekit-flyteinteractive/flytekitplugins/flyteinteractive/jupyter_lib/decorator.py
- plugins/flytekit-flyteinteractive/tests/test_flyteinteractive_jupyter.py
- tests/flytekit/unit/interactive/test_flyteinteractive_vscode.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

…ler return value Signed-off-by: Vincent <0426vincent@gmail.com>

flyte-bot · 2025-01-08T03:46:18Z

Code Review Agent Run #f8b810

Actionable Suggestions - 0

Review Details

Files reviewed - 3 · Commit Range: 60f4732..f1d19c8
- plugins/flytekit-flyteinteractive/tests/test_flyteinteractive_vscode.py
- plugins/flytekit-kf-pytorch/tests/test_elastic_task.py
- plugins/flytekit-pandera/tests/test_plugin.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

Signed-off-by: Vincent <0426vincent@gmail.com>

flyte-bot · 2025-01-08T05:56:19Z

Code Review Agent Run #d37b87

Actionable Suggestions - 0

Review Details

Files reviewed - 2 · Commit Range: f1d19c8..c6d70d7
- tests/flytekit/integration/remote/workflows/basic/generic_idl_flytetypes.py
- tests/flytekit/integration/remote/workflows/basic/msgpack_idl_flytetypes.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

Mecoli1219 · 2025-01-10T18:20:09Z

@vincent0426 Can you add some unit tests to make sure running the task/workflow without specifying output while returning a non-None value would throw an error? Also, running with --remote flag should also throw an error, but I think you can fix it in another PR.

Signed-off-by: Vincent <0426vincent@gmail.com>

flyte-bot · 2025-01-11T02:46:38Z

Code Review Agent Run #dd9258

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: c6d70d7..912cf34
- tests/flytekit/unit/core/test_task_return.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

fix: add FlyteValueException handling and clean up exit_handler calls

ea17a4d

Signed-off-by: Vincent <0426vincent@gmail.com>

vincent0426 requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners January 6, 2025 05:11

vincent0426 commented Jan 6, 2025

View reviewed changes

flyte-bot reviewed Jan 6, 2025

View reviewed changes

fix: update exit_handler calls to return values in decorators

60f4732

Signed-off-by: Vincent <0426vincent@gmail.com>

fix: add return type annotations to test functions and mock exit_hand…

f1d19c8

…ler return value Signed-off-by: Vincent <0426vincent@gmail.com>

vincent0426 requested a review from fg91 as a code owner January 8, 2025 02:37

vincent0426 added 2 commits January 7, 2025 20:47

fix: add return type to t1() in generic_idl_flytetypes.py

722b2da

Signed-off-by: Vincent <0426vincent@gmail.com>

fix: add return type to t1() in msgpack_idl_flytetypes.py

c6d70d7

Signed-off-by: Vincent <0426vincent@gmail.com>

vincent0426 added 2 commits January 10, 2025 17:44

test: add unit tests for task return values and exception handling

0cc4aec

Signed-off-by: Vincent <0426vincent@gmail.com>

fix: lint

912cf34

Signed-off-by: Vincent <0426vincent@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add FlyteValueException handling and clean up exit_handler calls #3036

fix: add FlyteValueException handling and clean up exit_handler calls #3036

vincent0426 commented Jan 6, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 6, 2025 •

edited

Loading

Code Review Agent Run #a5368c

vincent0426 Jan 6, 2025 •

edited

Loading

pingsutw Jan 6, 2025

vincent0426 Jan 6, 2025

pingsutw Jan 6, 2025 •

edited

Loading

vincent0426 Jan 6, 2025

Mecoli1219 Jan 8, 2025 •

edited

Loading

flyte-bot commented Jan 6, 2025 •

edited

Loading

Changelist by Bito

flyte-bot Jan 6, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

flyte-bot commented Jan 8, 2025 •

edited

Loading

Code Review Agent Run #0101b5

flyte-bot commented Jan 8, 2025 •

edited

Loading

Code Review Agent Run #f8b810

flyte-bot commented Jan 8, 2025 •

edited

Loading

Code Review Agent Run #d37b87

Mecoli1219 commented Jan 10, 2025

flyte-bot commented Jan 11, 2025 •

edited

Loading

Code Review Agent Run #dd9258

	if native_outputs is not None and len(list(self._outputs_interface.keys())) == 0:
	raise FlyteValueException(
	native_outputs,
	f"Interface has {len(self.python_interface.outputs)} outputs.",
	)

fix: add FlyteValueException handling and clean up exit_handler calls #3036

Are you sure you want to change the base?

fix: add FlyteValueException handling and clean up exit_handler calls #3036

Conversation

vincent0426 commented Jan 6, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

Summary by Bito

flyte-bot commented Jan 6, 2025 • edited Loading

Code Review Agent Run #a5368c

vincent0426 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

pingsutw Jan 6, 2025

Choose a reason for hiding this comment

vincent0426 Jan 6, 2025

Choose a reason for hiding this comment

pingsutw Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

vincent0426 Jan 6, 2025

Choose a reason for hiding this comment

full error log

Mecoli1219 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

flyte-bot commented Jan 6, 2025 • edited Loading

Changelist by Bito

flyte-bot Jan 6, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

flyte-bot commented Jan 8, 2025 • edited Loading

Code Review Agent Run #0101b5

flyte-bot commented Jan 8, 2025 • edited Loading

Code Review Agent Run #f8b810

flyte-bot commented Jan 8, 2025 • edited Loading

Code Review Agent Run #d37b87

Mecoli1219 commented Jan 10, 2025

flyte-bot commented Jan 11, 2025 • edited Loading

Code Review Agent Run #dd9258

vincent0426 commented Jan 6, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 6, 2025 •

edited

Loading

vincent0426 Jan 6, 2025 •

edited

Loading

pingsutw Jan 6, 2025 •

edited

Loading

Mecoli1219 Jan 8, 2025 •

edited

Loading

flyte-bot commented Jan 6, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading

flyte-bot commented Jan 8, 2025 •

edited

Loading

flyte-bot commented Jan 8, 2025 •

edited

Loading

flyte-bot commented Jan 8, 2025 •

edited

Loading

flyte-bot commented Jan 11, 2025 •

edited

Loading