-
Notifications
You must be signed in to change notification settings - Fork 287
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.
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
Deprecate scopes #2671
Deprecate scopes #2671
Conversation
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@@ -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]}",) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will directly modify the error message instead of raising another exception.
Signed-off-by: Kevin Su <[email protected]>
😱 this is awesome |
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do we remove deprecated items? flytekit 2.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want to wait until 2.0, maybe 6 months after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a github issue to track this deprecation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flytekit/bin/entrypoint.py
Outdated
tb_str = "\n ".join([""] + lines) | ||
format_str = "Traceback (most recent call last):\n" "{traceback}\n" "\n" "Message:\n" "\n" " {message}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a newline in the beginning and joining, which creates another list. Does this work?
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}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We should have a more specific error message there. Something like "flytekit runtime error. Original error message: {e}".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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())
flytekit/bin/entrypoint.py
Outdated
@@ -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__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added a comment here. we should only use e.__cause__.__traceback__
when the error is FlyteUserRuntimeException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We should have a more specific error message there. Something like "flytekit runtime error. Original error message: {e}".
flytekit/exceptions/user.py
Outdated
FlyteUserRuntimeException is thrown when a user code raises an exception. | ||
@param exc_value: The exception that was raised from user code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated it, thanks
@@ -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}!!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a User-scoped error. What should we change?
flytekit/bin/entrypoint.py
Outdated
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)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is task_def.id.name
being dropped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a github issue to track this deprecation?
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
This reverts commit 3c031c7.
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
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]}",) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean flytekit raises an exception when using AssertionError
in the user code because there is no message in the args
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, we get IndexError: tuple index out of range
when a test case throws an empty AssertionError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blaketastic2 do you want to open a PR to fix it? otherwise, I'll open one later this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take care of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty straight forward - #2831
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?
FlyteUserRuntimeException
. it is thrown when a user code raises an exception.system_entry_point
anduser_entry_point
in the codeHow was this patch tested?
unit test / remote
Setup process
Screenshots
Before:
Related PRs
NA
Docs link
NA