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 Exception Summaries #8

Merged
merged 13 commits into from
Mar 1, 2023
Merged

Add Exception Summaries #8

merged 13 commits into from
Mar 1, 2023

Conversation

msagarpatel
Copy link
Collaborator

@msagarpatel msagarpatel commented Dec 29, 2022

This PR adds the ability to summarize the current exception. This is especially helpful in cases where there are large exceptions with multiple TaskFailedExceptions and CompositeExceptions wrapping multiple exceptions.

The interface is simple:

    summarize_current_exceptions(io::IO=Base.stderr)

Examples:

julia> begin
           try
               try
                   @sync begin
                       Threads.@spawn try
                           @assert false
                       catch
                           @assert 1 != 1
                       end
                       Threads.@spawn @assert 2 != 2
                   end
               catch
                   @assert 3 != 3
               end
           catch
               summarize_current_exceptions()
           end
       end
=== EXCEPTION SUMMARY ===

CompositeException (2 tasks):
 1. AssertionError: false
     [1] macro expansion
       @ ./REPL[14]:6 [inlined]

    which caused:
    AssertionError: 1 != 1
     [1] macro expansion
       @ ./REPL[14]:8 [inlined]
 --
 2. AssertionError: 2 != 2
     [1] (::var"#10#12")()
       @ Main ./threadingconstructs.jl:258

which caused:
AssertionError: 3 != 3
 [1] top-level scope
   @ REPL[14]:13

TODOs

  • Test this in a larger codebase that can generate truly HUGE exceptions
  • Unit tests (WIP already working on this)
  • Seen set, for deduplication
  • Colorize the exception, not the "caused by"
  • Add examples to this description
  • Add ability to pass arbitrary exception rather than only allow current exception

src/exception_summary.jl Outdated Show resolved Hide resolved
src/exception_summary.jl Outdated Show resolved Hide resolved
Filter the stacktrace _before_ processing it all, which can improve perf
quite a bit.

Before:
```julia
julia> try
           @Assert false
       catch
           @time summarize_current_exceptions()
       end
=== EXCEPTION SUMMARY ===
AssertionError: false
 [1] top-level scope
  @ ./REPL[95]:2
  0.004641 seconds (484 allocations: 27.102 KiB)
```

After:
```julia
julia> try
           @Assert false
       catch
           @time summarize_current_exceptions()
       end
=== EXCEPTION SUMMARY ===
AssertionError: false
 [1] top-level scope
  @ ./REPL[95]:2
  0.001720 seconds (228 allocations: 12.766 KiB)
```
test/exception_summary.jl Outdated Show resolved Hide resolved
- Number the items in a CompositeException when printing.
- Support multiline exception strings, still indented properly on each
  of their multiple lines.
- Add a test for these situations.

For example:
```julia
=== EXCEPTION SUMMARY ===

CompositeException (length 2):
 1. MultiLineException(
        0
    )
     [1] throw_multiline(x::Int64)
       @ Main foo.jl:23

    which caused:
    MultiLineException(
        1
    )
     [1] throw_multiline(x::Int64)
       @ Main foo.jl:23

 2. MultiLineException(
        2
    )
     [1] throw_multiline(x::Int64)
       @ Main foo.jl:23

which caused:
MultiLineException(
    3
)
 [1] throw_multiline(x::Int64)
   @ Main foo.jl:23
```
This hopefully makes it clearer what's going on, for a reader who
doesn't already know what a CompositeException is for.
now that the items are numbered
@NHDaly
Copy link
Member

NHDaly commented Jan 29, 2023

@msagarpatel:
Regarding the TODOs:

  • Test this in a larger codebase that can generate truly HUGE exceptions
    • Good idea, but we could probably merge this PR as-is, and then iterate!
    • No need to let perfection be the enemy of the good.
  • Unit tests (WIP already working on this)
    • I think the tests we have now seem decent! What else would you want to add?
  • Seen set, for deduplication
    • I think we can do this in a follow-up. No need for it now.
  • Colorize the exception, not the "caused by"
    • Meh.. I don't think this is easy, and i don't think it's worth doing.
    • Base doesn't color the exception names, and we're reusing their printing, so i don't think this is feasible, nor important.
  • Add examples to this description
    • sure, sounds nice. I can copy in one of the examples I added as a test. Is that enough?
  • Add ability to pass arbitrary exception rather than only allow current exception
    • Fine to leave this as a follow-up, or never do it. I don't think this really makes
      sense considering that the whole goal here is to print a whole exception stack.
    • With just a single exception, you could just call Base.display_error().

@NHDaly NHDaly marked this pull request as ready for review January 29, 2023 23:08
Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

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

@msagarpatel: I think this PR LGTM!

I'll leave it open in case there's anything else you want to address, but this looks really great. :)

I made a few more improvements today, and I think this is ready to use! We should plug it in to RAICode, since it's not helping anyone just sitting here. 😊 We can always make more changes later, but i think this would already be really, really helpful.

Co-authored-by: Nathan Daly <NHDaly@gmail.com>
@msagarpatel
Copy link
Collaborator Author

Thanks @NHDaly! I'm still going through your comments and changes. Gonna check out for the night, but I'm aiming to wrap this up tomorrow.

@NHDaly NHDaly merged commit e099a4e into master Mar 1, 2023
@NHDaly NHDaly deleted the sp-summary branch March 1, 2023 15:59
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