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

Distinguish between local and remote exceptions when stopping ephemeral app #2071

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

mwaskom
Copy link
Contributor

@mwaskom mwaskom commented Aug 5, 2024

Currently, we re-raise remote exceptions inside the local client process, which causes us to report the app stop event as being caused by a "local exception". That's confusing and sets users off on the wrong debugging path. This inspects the traceback to determine whether the stack contains a hop out to a remote modal function call and then attributes the error accordingly.

Needs a server-side change too.

  • Closes MOD-3082

@mwaskom mwaskom force-pushed the michael/improve-ephemeral-remote-error-exit branch from f417784 to db4c1df Compare August 5, 2024 14:12
@mwaskom mwaskom requested a review from erikbern August 5, 2024 14:48
@mwaskom
Copy link
Contributor Author

mwaskom commented Aug 6, 2024

@prbot approve

Copy link

@modal-pr-review-automation modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

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

Approved 👍. @erikbern will follow-up review this.

@@ -94,6 +97,15 @@ def reduce_traceback_to_user_code(tb: TracebackType, user_source: str) -> Traceb
return tb


def traceback_contains_remote_call(tb: Optional[TracebackType]) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's cleaner to set a magic attribute on the exception or something? we create this exception in code somewhere, i think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into that but I'm not convinced it's simpler. We construct the traceback with these special task-id-containing filenames server-side as a TracebackDict when then gets deserialized into an actual traceback object client-side. For ephemeral apps, all we have is the run_app context manager that yields out to a script contexts; but if we catch an exception there we don't know whether it was triggered locally or after a remote call. Then there's already some special exception type wrapping / unwrapping in synchronicity too when we go through Function.remote...

@mwaskom mwaskom force-pushed the michael/improve-ephemeral-remote-error-exit branch from db4c1df to 0d087a4 Compare August 6, 2024 12:34
@mwaskom mwaskom merged commit cb6b242 into main Aug 6, 2024
22 checks passed
@mwaskom mwaskom deleted the michael/improve-ephemeral-remote-error-exit branch August 6, 2024 14:04
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.

2 participants