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

Cleanup remote tracebacks #10354

Merged
merged 16 commits into from Jun 20, 2023

Conversation

j-bennet
Copy link
Contributor

@j-bennet j-bennet commented Jun 13, 2023

Adds a utility function shorten_traceback, and wraps sys.excepthook to apply it to unhandled exceptions.

The behavior of the function is controlled by several settings:

  • admin.traceback.shorten.when
  • admin.traceback.shorten.what

If both are non-empty, and any of the stack frames match a pattern listed in admin.traceback.shorten.when, stack trace is shortened. Frames that match a pattern listed in admin.traceback.shorten.what are omitted. The method always preserves the first and last stack frame though.

For IPython, instead of sys.excepthook, a custom exception handler is provided.

The second part - adding worker info to the remote exception - is implemented in dask/distributed#7847.

Issue: dask/distributed#4880.

  • Tests added / passed
  • Passes pre-commit run --all-files

As discussed with @crusaderky.

@j-bennet j-bennet force-pushed the j-bennet/4880-cleanup-remote-tracebacks branch from b4ac95e to 3fac9a1 Compare June 13, 2023 23:24
@j-bennet j-bennet force-pushed the j-bennet/4880-cleanup-remote-tracebacks branch from 65e4aa1 to fb32bd6 Compare June 14, 2023 23:23
@j-bennet j-bennet force-pushed the j-bennet/4880-cleanup-remote-tracebacks branch from b191357 to a9cb145 Compare June 15, 2023 23:10
@j-bennet j-bennet marked this pull request as ready for review June 16, 2023 01:33
@j-bennet
Copy link
Contributor Author

@crusaderky Finally got the test passing. Lesson learned: never expect ipython to behave the same on different envs. This is now ready for review.

One more thing I was thinking about adding to this PR: exposing a function to install the excepthook, instead of modifying it without the user participation. It might be an overkill. What do you think?

@crusaderky crusaderky force-pushed the j-bennet/4880-cleanup-remote-tracebacks branch from 287d57b to bd6bca9 Compare June 19, 2023 11:06
Copy link
Collaborator

@crusaderky crusaderky left a comment

Choose a reason for hiding this comment

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

@j-bennet see my review commit
Merging as soon as you confirm you're happy with my changes

@crusaderky crusaderky force-pushed the j-bennet/4880-cleanup-remote-tracebacks branch from bd6bca9 to f90a842 Compare June 19, 2023 11:14
@crusaderky
Copy link
Collaborator

exposing a function to install the excepthook, instead of modifying it without the user participation. It might be an overkill. What do you think?

I don't think I follow. What function are you referring to?

@j-bennet
Copy link
Contributor Author

j-bennet commented Jun 19, 2023

@crusaderky

@j-bennet see my review commit Merging as soon as you confirm you're happy with my changes

when and what sounds fine.

I don't think I follow. What function are you referring to?

In the PR at the current state, as soon as the user imports something from dask, sys.excepthook is wrapped, even if the wrapper does nothing when admin.traceback.shorten is false.

Alternatively, wrapping excepthook could be something that's triggered by the user, by calling a method, let's say, dask.instal_hooks.

@crusaderky
Copy link
Collaborator

In the PR at the current state, as soon as the user imports something from dask, sys.excepthook is wrapped, even if the wrapper does nothing when admin.traceback.shorten is false.

Alternatively, wrapping excepthook could be something that's triggered by the user, by calling a method, let's say, dask.instal_hooks.

I see your reasoning, but I feel that if we added a completely different API it would make things worse for the UX rather than better.
This (and quite a few other situations) could potentially resolved by being able to install callbacks in dask.config.set. Definitely out of scope though.

@crusaderky crusaderky merged commit d601131 into dask:main Jun 20, 2023
24 checks passed
@crusaderky
Copy link
Collaborator

Thank you!

@j-bennet j-bennet deleted the j-bennet/4880-cleanup-remote-tracebacks branch June 20, 2023 15:53
@sk1p sk1p mentioned this pull request Jun 29, 2023
2 tasks
maartenbreddels added a commit to widgetti/solara that referenced this pull request Jul 11, 2023
In dask/dask#10354 dask
calls set_custom_exc to set a custom exception handler.

This breaks solara with dask because we have a custom/fake
IPython implementation.

Fixes #191
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.

None yet

2 participants