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.

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

Add FlyteNonRecoverableSystemException #2700

Merged
merged 45 commits into from
Sep 25, 2024
Merged

Add FlyteNonRecoverableSystemException #2700

merged 45 commits into from
Sep 25, 2024

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Aug 21, 2024

Tracking issue

NA

Why are the changes needed?

Flyte returns the task when it fails to convert the Python output to a literal or vice versa.

What changes were proposed in this pull request?

Raise FlyteNonRecoverableSystemException if flytekit fails to serialize the output. (e.g. failed to convert python.int to Literal.simple.string).
However, if the error comes from data persistence, we won't change the error type to FlyteNonRecoverableSystemException

How was this patch tested?

remote cluster

pyflyte run --remote flyte-example/improve_error/mismatching_output.py wf

Setup process

from flytekit import task, workflow, ImageSpec

new_flytekit = "git+https://github.com/flyteorg/flytekit@3ef9b11cba76a237bb010c8a764ecdc2c2093089"

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


@task(container_image=image_spec)
def t1() -> int:
    return "hello"


@workflow()
def wf():
    t1()

Screenshots

Before:
Screenshot 2024-08-21 at 4 54 55 PM

After:
Screenshot 2024-08-21 at 4 54 47 PM

Check all the applicable boxes

  • 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 <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
This reverts commit 3c031c7.
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
wild-endeavor and others added 14 commits September 3, 2024 16:13
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw pingsutw marked this pull request as ready for review September 5, 2024 23:17
@pingsutw pingsutw changed the title [WIP] Add FlyteNonRecoverableSystemException Add FlyteNonRecoverableSystemException Sep 5, 2024
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@wild-endeavor
Copy link
Contributor

tried to merge in master, didn't do it correctly though, definitely missing some things, like the double Exception catching that's now in entrypoint.

Regardless I think we should think more about this - I'm not sure it really addresses the issue raised in slack (and commented in this issue here). This issue is on the propeller side.

@wild-endeavor
Copy link
Contributor

forgot to hit submit on that comment. that was from before kevin fixed the merge

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw pingsutw merged commit e60c152 into master Sep 25, 2024
104 checks passed
otarabai pushed a commit to otarabai/flytekit that referenced this pull request Oct 15, 2024
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Co-authored-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
kumare3 pushed a commit that referenced this pull request Nov 8, 2024
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Co-authored-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants