Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ea17a4d
fix: add FlyteValueException handling and clean up exit_handler calls
vincent0426 Jan 6, 2025
60f4732
fix: update exit_handler calls to return values in decorators
vincent0426 Jan 8, 2025
f1d19c8
fix: add return type annotations to test functions and mock exit_hand…
vincent0426 Jan 8, 2025
722b2da
fix: add return type to t1() in generic_idl_flytetypes.py
vincent0426 Jan 8, 2025
c6d70d7
fix: add return type to t1() in msgpack_idl_flytetypes.py
vincent0426 Jan 8, 2025
0cc4aec
test: add unit tests for task return values and exception handling
vincent0426 Jan 11, 2025
912cf34
fix: lint
vincent0426 Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion flytekit/core/base_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@
FlyteNonRecoverableSystemException,
FlyteUploadDataException,
)
from flytekit.exceptions.user import FlyteUserRuntimeException
from flytekit.exceptions.user import (
FlyteUserRuntimeException,
FlyteValueException,
)
from flytekit.loggers import logger
from flytekit.models import dynamic_job as _dynamic_job
from flytekit.models import interface as _interface_models
Expand Down Expand Up @@ -770,6 +773,15 @@ def dispatch_execute(
):
return native_outputs

if isinstance(native_outputs, VoidPromise):
return _literal_models.LiteralMap(literals={})

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.",
)

try:
with timeit("dispatch execute"):
literals_map, native_outputs_as_map = run_sync(
Expand Down
2 changes: 1 addition & 1 deletion flytekit/interactive/vscode_lib/decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ def execute(self, *args, **kwargs):
# 5. Prepare the launch.json
prepare_launch_json()

return exit_handler(
exit_handler(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

child_process=child_process,
task_function=self.task_function,
args=args,
Expand Down
2 changes: 1 addition & 1 deletion plugins/flytekit-flyteinteractive/flytekitplugins/flyteinteractive/jupyter_lib/decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor Author

@vincent0426 vincent0426 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2025-01-06 at 2 33 33 PM

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

Copy link
Member

@pingsutw pingsutw Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, you can add return type to that function.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screenshot 2025-01-06 at 3 38 48 PM

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

Copy link
Contributor

@Mecoli1219 Mecoli1219 Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

child_process=child_process,
task_function=self.task_function,
args=args,
Expand Down
Loading