Deprecate scopes #2671

pingsutw · 2024-08-09T23:34:14Z

Tracking issue

NA

Why are the changes needed?

Error messages are not clear in both remote and local execution.

What changes were proposed in this pull request?

Add FlyteUserRuntimeException. it is thrown when a user code raises an exception.
Non-FlyteUserRuntimeException errros are system errors
remove system_entry_point and user_entry_point in the code
deprecate FlyteScopedException

How was this patch tested?

unit test / remote

Setup process

from flytekit import task, workflow, ImageSpec

new_flytekit = "git+https://github.com/flyteorg/flytekit@14ab38ca17d3b9a5bb70e5ae9a61955b01f573e4"

sd_finetuning_image = ImageSpec(
    registry="pingsutw",
    apt_packages=["git"],
    packages=[
        new_flytekit,
    ],
    builder="envd",
)


@task(enable_deck=True, container_image=sd_finetuning_image)
def t1(a: int) -> int:
    raise ValueError("error")
    return a + 1


@workflow
def wf() -> int:
    return t1(a=3)

Screenshots

Before:

User error

System error

Local error

User error

System error

Local error

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

Related PRs

NA

Docs link

NA

Signed-off-by: Kevin Su <[email protected]>

codecov · 2024-08-10T07:49:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.00%. Comparing base (2452c74) to head (f253efe).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2671      +/-   ##
==========================================
+ Coverage   83.55%   93.00%   +9.45%     
==========================================
  Files           3       26      +23     
  Lines         152     1230    +1078     
==========================================
+ Hits          127     1144    +1017     
- Misses         25       86      +61

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

Signed-off-by: Kevin Su <[email protected]>

pingsutw · 2024-08-10T09:56:14Z

flytekit/extend/backend/base_agent.py

@@ -272,7 +272,8 @@ async def _do(
                agent.do, task_template=template, inputs=literal_map, output_prefix=output_prefix
            )
        except Exception as e:
-            raise FlyteUserException(f"Failed to run the task {self.name} with error: {e}") from None
+            e.args = (f"Failed to run the task {self.name} with error: {e.args[0]}",)


It will directly modify the error message instead of raising another exception.

Signed-off-by: Kevin Su <[email protected]>

kumare3 · 2024-08-11T05:28:48Z

😱 this is awesome

flytekit/exceptions/user.py

thomasjpfan · 2024-08-12T15:44:51Z

flytekit/exceptions/scopes.py

@@ -15,6 +16,7 @@

 class FlyteScopedException(Exception):
    def __init__(self, context, exc_type, exc_value, exc_tb, top_trim=0, bottom_trim=0, kind=None):
+        warn(f"{self.__class__.__name__} is deprecated.", DeprecationWarning, stacklevel=2)


When do we remove deprecated items? flytekit 2.0?

Don't want to wait until 2.0, maybe 6 months after

Can you create a github issue to track this deprecation?

flyteorg/flyte#5668

flytekit/bin/entrypoint.py

thomasjpfan · 2024-08-12T15:57:06Z

flytekit/bin/entrypoint.py

+    tb_str = "\n    ".join([""] + lines)
+    format_str = "Traceback (most recent call last):\n" "{traceback}\n" "\n" "Message:\n" "\n" "    {message}"


Instead of adding a newline in the beginning and joining, which creates another list. Does this work?

Suggested change

tb_str = "\n ".join([""] + lines)

format_str = "Traceback (most recent call last):\n" "{traceback}\n" "\n" "Message:\n" "\n" " {message}"

tb_str = "\n ".join(lines)

format_str = "Traceback (most recent call last):\n" "\n {traceback}\n" "\n" "Message:\n" "\n" " {message}"

updated it. new error

Signed-off-by: Kevin Su <[email protected]>

wild-endeavor

thanks!

wild-endeavor · 2024-08-13T02:14:31Z

flytekit/core/base_task.py

+                        # If the task is being executed locally, we want to raise the original exception
+                        e.args = (f"Error encountered while executing '{self.name}':\n  {e.args[0]}",)
+                        raise
+                    raise FlyteUserRuntimeException(e) from e


is this a bit odd? to raise a new type using e but also from e. Is this what you're supposed to do? I guess the printing strips out the top layer and only looks at the __cause__ right?

Agreed. We should have a more specific error message there. Something like "flytekit runtime error. Original error message: {e}".

I thought it was a common use case.

I did this, so I can get the user stack trace from e.__cause__.__traceback.
Otherwise, I have to save it in the FlyteUserRuntimeException. like

raise FlyteUserRuntimeException(*_exc_info())

wild-endeavor · 2024-08-13T02:17:52Z

flytekit/bin/entrypoint.py

@@ -185,6 +180,17 @@ def _dispatch_execute(
        exit(1)


+def get_traceback_str(e: Exception) -> str:
+    tb = e.__cause__.__traceback__ if e.__cause__ else e.__traceback__


should we make this recursive?

Are you happy with the way these look? should we try to format them better? This is only used in actual runs right?

I just added a comment here. we should only use e.__cause__.__traceback__ when the error is FlyteUserRuntimeException

eapolinario

This is pretty cool!

Worth calling out that the exception handling code is no longer re-entrant. For example, an exception coming from user code called by system code will no longer be considered a system exception, since now we'll only consider system errors any exception that's not a FlyteUserRuntimeException.

flytekit/bin/entrypoint.py

flytekit/core/base_task.py

eapolinario · 2024-08-14T22:35:14Z

flytekit/core/base_task.py

+                        # If the task is being executed locally, we want to raise the original exception
+                        e.args = (f"Error encountered while executing '{self.name}':\n  {e.args[0]}",)
+                        raise
+                    raise FlyteUserRuntimeException(e) from e


Agreed. We should have a more specific error message there. Something like "flytekit runtime error. Original error message: {e}".

flytekit/exceptions/scopes.py

eapolinario · 2024-08-14T22:40:35Z

flytekit/exceptions/user.py

+        FlyteUserRuntimeException is thrown when a user code raises an exception.
+        @param exc_value: The exception that was raised from user code.


can you fix the docstring to follow the rest of the codebase? For example

flytekit/flytekit/core/reference.py

Lines 22 to 41 in 03d2301

See the documentation for :py:class:`flytekit.reference_task` and :py:class:`flytekit.reference_workflow` as well.

This function is the general form of the two aforementioned functions. It's better for programmatic usage, as

the interface is passed in as arguments instead of analyzed from type annotations.

.. literalinclude:: ../../../tests/flytekit/unit/core/test_references.py

:start-after: # docs_ref_start

:end-before: # docs_ref_end

:language: python

:dedent: 4

:param resource_type: This is the type of entity it is. Must be one of

:py:class:`flytekit.models.core.identifier.ResourceType`

:param project: The project the entity you're looking for has been registered in.

:param domain: The domain the entity you're looking for has been registered in.

:param name: The name of the registered entity

:param version: The version the entity you're looking for has been registered with.

:param inputs: An ordered dictionary of input names as strings to their Python types.

:param outputs: An ordered dictionary of output names as strings to their Python types.

:return:

.

updated it, thanks

eapolinario · 2024-08-14T22:48:02Z

flytekit/bin/entrypoint.py

@@ -122,48 +120,45 @@ def _dispatch_execute(
            )

    # Handle user-scoped errors
-    except _scoped_exceptions.FlyteScopedUserException as e:
+    except FlyteUserRuntimeException as e:
+        # Step3b
        if isinstance(e.value, IgnoreOutputs):
            logger.warning(f"User-scoped IgnoreOutputs received! Outputs.pb will not be uploaded. reason {e}!!")


Update comment.

It is a User-scoped error. What should we change?

flytekit/bin/entrypoint.py

eapolinario · 2024-08-14T23:02:26Z

flytekit/bin/entrypoint.py

                exc_str,
                _error_models.ContainerError.Kind.RECOVERABLE,
                _execution_models.ExecutionError.ErrorKind.SYSTEM,
            )
        )
-        logger.error(f"Exception when executing task {task_def.name or task_def.id.name}, reason {str(e)}")
-        logger.error("!! Begin Unknown System Error Captured by Flyte !!")
+        logger.error(f"Exception when executing task {task_def.name}, reason {str(e)}")


why is task_def.id.name being dropped?

task_def doesn't have id attribute

flytekit/bin/entrypoint.py

eapolinario · 2024-08-15T01:22:10Z

flytekit/exceptions/scopes.py

@@ -15,6 +16,7 @@

 class FlyteScopedException(Exception):
    def __init__(self, context, exc_type, exc_value, exc_tb, top_trim=0, bottom_trim=0, kind=None):
+        warn(f"{self.__class__.__name__} is deprecated.", DeprecationWarning, stacklevel=2)


Can you create a github issue to track this deprecation?

flytekit/core/base_task.py

Signed-off-by: Kevin Su <[email protected]>

This reverts commit 3c031c7.

Signed-off-by: Kevin Su <[email protected]>

blaketastic2 · 2024-10-17T15:35:59Z

flytekit/core/base_task.py

-            msg = f"Failed to convert inputs of task '{self.name}':\n  {exc}"
-            logger.error(msg)
-            raise TypeError(msg) from None
+            exc.args = (f"Failed to convert inputs of task '{self.name}':\n  {exc.args[0]}",)


This actually starts breaking behavior for things like AssertionError where someone might just do assert val > 0 without specifying a message like assert val > 0, "val must be greater than 0

Does it mean flytekit raises an exception when using AssertionError in the user code because there is no message in the args?

Yea, we get IndexError: tuple index out of range when a test case throws an empty AssertionError.

To be more specific, I mentioned test case only because that's how I caught this bug when migrating from 1.12.x to 1.13.x. The task is what is actually doing the assert val > 0 check.

@blaketastic2 do you want to open a PR to fix it? otherwise, I'll open one later this week.

I'll take care of it.

Thank you in advance!

Seems pretty straight forward - #2831

Deprecate scopes

4a97da5

Signed-off-by: Kevin Su <[email protected]>

pingsutw requested review from fg91, wild-endeavor, kumare3, eapolinario, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners August 9, 2024 23:34

pingsutw marked this pull request as draft August 9, 2024 23:34

pingsutw added 4 commits August 9, 2024 16:43

test

c8b2a2b

Signed-off-by: Kevin Su <[email protected]>

test

aecd76f

Signed-off-by: Kevin Su <[email protected]>

test

24048e7

Signed-off-by: Kevin Su <[email protected]>

test

14ab38c

Signed-off-by: Kevin Su <[email protected]>

pingsutw added 6 commits August 10, 2024 01:30

fix tests

d6bdfc2

Signed-off-by: Kevin Su <[email protected]>

fix tests

b5eff43

Signed-off-by: Kevin Su <[email protected]>

format error

a710985

Signed-off-by: Kevin Su <[email protected]>

test

010bff5

Signed-off-by: Kevin Su <[email protected]>

fix test

50e15bb

Signed-off-by: Kevin Su <[email protected]>

fmt

97c0648

Signed-off-by: Kevin Su <[email protected]>

pingsutw commented Aug 10, 2024

View reviewed changes

fix tests

33b53dd

Signed-off-by: Kevin Su <[email protected]>

pingsutw changed the title ~~[WIP] Deprecate scopes~~ Deprecate scopes Aug 11, 2024

pingsutw self-assigned this Aug 11, 2024

pingsutw marked this pull request as ready for review August 11, 2024 05:03

thomasjpfan reviewed Aug 12, 2024

View reviewed changes

pingsutw added 2 commits August 12, 2024 14:04

address comments

89e61cc

Signed-off-by: Kevin Su <[email protected]>

nit

d9561cd

Signed-off-by: Kevin Su <[email protected]>

wild-endeavor previously approved these changes Aug 13, 2024

View reviewed changes

eapolinario reviewed Aug 15, 2024

View reviewed changes

wild-endeavor reviewed Aug 15, 2024

View reviewed changes

flytekit/core/base_task.py Outdated Show resolved Hide resolved

pingsutw mentioned this pull request Aug 18, 2024

Remove FlyteScopedException from flytekit flyteorg/flyte#5668

Open

2 tasks

Address comment

9e64e93

Signed-off-by: Kevin Su <[email protected]>

pingsutw dismissed wild-endeavor’s stale review via 9e64e93 August 18, 2024 05:56

pingsutw added 10 commits August 17, 2024 23:14

lint

a08a43a

Signed-off-by: Kevin Su <[email protected]>

merged master

da9690d

Signed-off-by: Kevin Su <[email protected]>

conftest

58b2afa

Signed-off-by: Kevin Su <[email protected]>

nit

0542f76

Signed-off-by: Kevin Su <[email protected]>

test

3c031c7

Signed-off-by: Kevin Su <[email protected]>

Revert "test"

470c707

This reverts commit 3c031c7.

merged master

1e0f971

Signed-off-by: Kevin Su <[email protected]>

lint

264d182

Signed-off-by: Kevin Su <[email protected]>

fix tests

732375e

Signed-off-by: Kevin Su <[email protected]>

fix tests

f253efe

Signed-off-by: Kevin Su <[email protected]>

wild-endeavor approved these changes Aug 28, 2024

View reviewed changes

pingsutw merged commit cf2f492 into master Aug 28, 2024
101 of 103 checks passed

blaketastic2 reviewed Oct 17, 2024

View reviewed changes

blaketastic2 mentioned this pull request Oct 18, 2024

handle case where error may not have args #2831

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate scopes #2671

Deprecate scopes #2671

pingsutw commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 10, 2024 •

edited

Loading

pingsutw Aug 10, 2024

kumare3 commented Aug 11, 2024

thomasjpfan Aug 12, 2024

pingsutw Aug 12, 2024

eapolinario Aug 15, 2024

pingsutw Aug 18, 2024

thomasjpfan Aug 12, 2024

pingsutw Aug 12, 2024

wild-endeavor left a comment

wild-endeavor Aug 13, 2024 •

edited

Loading

eapolinario Aug 14, 2024

pingsutw Aug 18, 2024

pingsutw Aug 18, 2024

wild-endeavor Aug 13, 2024

pingsutw Aug 18, 2024

eapolinario left a comment

eapolinario Aug 14, 2024

eapolinario Aug 14, 2024

pingsutw Aug 18, 2024

eapolinario Aug 14, 2024

pingsutw Aug 18, 2024

eapolinario Aug 14, 2024

pingsutw Aug 18, 2024

eapolinario Aug 15, 2024

blaketastic2 Oct 17, 2024

pingsutw Oct 17, 2024

blaketastic2 Oct 17, 2024

blaketastic2 Oct 17, 2024

pingsutw Oct 17, 2024

blaketastic2 Oct 17, 2024

pingsutw Oct 17, 2024

blaketastic2 Oct 18, 2024

		tb_str = "\n ".join([""] + lines)
		format_str = "Traceback (most recent call last):\n" "{traceback}\n" "\n" "Message:\n" "\n" " {message}"

		FlyteUserRuntimeException is thrown when a user code raises an exception.
		@param exc_value: The exception that was raised from user code.

	See the documentation for :py:class:`flytekit.reference_task` and :py:class:`flytekit.reference_workflow` as well.

	This function is the general form of the two aforementioned functions. It's better for programmatic usage, as
	the interface is passed in as arguments instead of analyzed from type annotations.

	.. literalinclude:: ../../../tests/flytekit/unit/core/test_references.py
	:start-after: # docs_ref_start
	:end-before: # docs_ref_end
	:language: python
	:dedent: 4

	:param resource_type: This is the type of entity it is. Must be one of
	:py:class:`flytekit.models.core.identifier.ResourceType`
	:param project: The project the entity you're looking for has been registered in.
	:param domain: The domain the entity you're looking for has been registered in.
	:param name: The name of the registered entity
	:param version: The version the entity you're looking for has been registered with.
	:param inputs: An ordered dictionary of input names as strings to their Python types.
	:param outputs: An ordered dictionary of output names as strings to their Python types.
	:return:

Deprecate scopes #2671

Deprecate scopes #2671

Conversation

pingsutw commented Aug 9, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Related PRs

Docs link

codecov bot commented Aug 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

kumare3 commented Aug 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wild-endeavor left a comment

Choose a reason for hiding this comment

wild-endeavor Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapolinario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pingsutw commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 10, 2024 •

edited

Loading

wild-endeavor Aug 13, 2024 •

edited

Loading