Skip to content

Only print a single LoadError: prefix for nested LoadErrors during showerror #38725

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 8, 2021
Merged

Only print a single LoadError: prefix for nested LoadErrors during showerror #38725

merged 2 commits into from
Apr 8, 2021

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Dec 5, 2020 •
edited
Loading

Edit: Updated to

ERROR: LoadError: ArgumentError: Package CompilerSupportLibraries_jll [e66e0078-7015-5450-92f7-15fbd957f2ae] is required but does not seem to be installed:
 - Run `Pkg.instantiate()` to install all recorded dependencies.
... (backtrace)
in expression starting at /Users/ian/.julia/packages/OpenSpecFun_jll/Xw8XK/src/wrappers/x86_64-apple-darwin-libgfortran5.jl:4
in expression starting at /Users/ian/.julia/packages/OpenSpecFun_jll/Xw8XK/src/OpenSpecFun_jll.jl:2
in expression starting at /Users/ian/.julia/packages/SpecialFunctions/24S26/src/SpecialFunctions.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/forward/number.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/forward/Forward.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/Zygote.jl:1
in expression starting at /Users/ian/.julia/packages/Flux/q3zeA/src/Flux.jl:1

(Original suggestion)

Folds repeated LoadError: prints into LoadError (xN): where N is the depth. Prints normally if depth is 1

Before:

ERROR: LoadError: LoadError: LoadError: LoadError: LoadError: LoadError: LoadError: ArgumentError: Package CompilerSupportLibraries_jll [e66e0078-7015-5450-92f7-15fbd957f2ae] is required but does not seem to be installed:
 - Run `Pkg.instantiate()` to install all recorded dependencies.
... (backtrace)
in expression starting at /Users/ian/.julia/packages/OpenSpecFun_jll/Xw8XK/src/wrappers/x86_64-apple-darwin-libgfortran5.jl:4
in expression starting at /Users/ian/.julia/packages/OpenSpecFun_jll/Xw8XK/src/OpenSpecFun_jll.jl:2
in expression starting at /Users/ian/.julia/packages/SpecialFunctions/24S26/src/SpecialFunctions.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/forward/number.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/forward/Forward.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/Zygote.jl:1
in expression starting at /Users/ian/.julia/packages/Flux/q3zeA/src/Flux.jl:1

This PR:

ERROR: LoadError (x7): ArgumentError: Package CompilerSupportLibraries_jll [e66e0078-7015-5450-92f7-15fbd957f2ae] is required but does not seem to be installed:
 - Run `Pkg.instantiate()` to install all recorded dependencies.
... (backtrace)
in expression starting at /Users/ian/.julia/packages/OpenSpecFun_jll/Xw8XK/src/wrappers/x86_64-apple-darwin-libgfortran5.jl:4
in expression starting at /Users/ian/.julia/packages/OpenSpecFun_jll/Xw8XK/src/OpenSpecFun_jll.jl:2
in expression starting at /Users/ian/.julia/packages/SpecialFunctions/24S26/src/SpecialFunctions.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/forward/number.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/forward/Forward.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/Zygote.jl:1
in expression starting at /Users/ian/.julia/packages/Flux/q3zeA/src/Flux.jl:1

@IanButterworth IanButterworth changed the title Fold repeated LoadError : prints during showerror Fold repeated LoadError: prints during showerror Dec 5, 2020
@goretkin
Copy link
Contributor

goretkin commented Dec 5, 2020

From very cursory experience with other reporting systems, I expect LoadError (xN) to mean N consecutive LoadErrors (at the top level), and not N nested LoadErrors.

One example that comes to mind is javascript consoles:

image

What about something like LoadError (nested N times) to be very explicit?

@IanButterworth
Copy link
Member Author

Yeah, I see that. How about LoadError (N modules deep)?

@goretkin
Copy link
Contributor

goretkin commented Dec 5, 2020 •
edited
Loading

Yeah, I see that. How about LoadError (N modules deep)?

They're technically not module boundaries, but instead file boundaries (include), right?

in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/forward/number.jl:1
in expression starting at /Users/ian/.julia/packages/Zygote/nK6sg/src/forward/Forward.jl:1

https://github.com/FluxML/Zygote.jl/blob/f8b038c552eb092f7c7af682e8d2acb38c733889/src/forward/number.jl

If it's not imprecise, then LoadError (N files deep). Or perhaps LoadError (N include statements deep), though that's getting long.

@DilumAluthge DilumAluthge added the error messages Better, more actionable error messages label Dec 6, 2020
@IanButterworth
Copy link
Member Author

Good point. How about just (7 loads deep)?

@IanButterworth
Copy link
Member Author

Or how about we just always show it as a single LoadError:, no matter how many times it recurs?

@StefanKarpinski
Copy link
Member

I think just one LoadError: prefix is sufficient.

@IanButterworth
Copy link
Member Author

Yeah, I like that

@IanButterworth IanButterworth changed the title Fold repeated LoadError: prints during showerror Only print a single LoadError: prefix for nested LoadErrors during showerror Dec 15, 2020
@StefanKarpinski
Copy link
Member

Requesting review from @JeffBezanson, primarily on the idea of just printing one LoadError: prefix here.

@IanButterworth
Copy link
Member Author

Low priority bump

@IanButterworth
Copy link
Member Author

Bump @JeffBezanson

vtjnash
vtjnash reviewed Apr 6, 2021
View reviewed changes
base/errorshow.jl

function showerror(io::IO, ex::LoadError, bt; backtrace=true)
print(io, "LoadError: ")
!isa(ex.error, LoadError) && print(io, "LoadError: ")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
!isa(ex.error, LoadError) && print(io, "LoadError: ")

I'm not even entirely sure that this part of the text is useful at all?

Copy link
Member Author

@IanButterworth IanButterworth Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

True. I'd go with removing LoadError: from the error show entirely if people agree. Or would that be considered to be a breaking change?

@IanButterworth
Copy link
Member Author

Given the approval I'm going to go with a LoadError: print, and come back to the idea of skipping LoadError: entirely if that idea gets popular

@IanButterworth IanButterworth merged commit 35a186b into JuliaLang:master Apr 8, 2021
@IanButterworth IanButterworth deleted the ib/loaderror_condensed_print branch April 8, 2021 01:07
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
@IanButterworth @ElOceanografo
Only print a single LoadError: prefix for nested LoadErrors during …
44e8b86 
…showerror (JuliaLang#38725)

* fold repeated `LoadError :` prints during showerror

* Only print a single LoadError: prefix
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
@IanButterworth @antoine-levitt
Only print a single LoadError: prefix for nested LoadErrors during …
38bee9e 
…showerror (JuliaLang#38725)

* fold repeated `LoadError :` prints during showerror

* Only print a single LoadError: prefix
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
@IanButterworth @johanmon
Only print a single LoadError: prefix for nested LoadErrors during …
2991a4b 
…showerror (JuliaLang#38725)

* fold repeated `LoadError :` prints during showerror

* Only print a single LoadError: prefix
@devmotion devmotion mentioned this pull request Dec 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

@vtjnash vtjnash vtjnash approved these changes

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

Assignees
No one assigned
Labels
error messages Better, more actionable error messages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@IanButterworth @goretkin @StefanKarpinski @vtjnash @DilumAluthge