-
Notifications
You must be signed in to change notification settings - Fork 31
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
Use depth-limited type printing #601
Use depth-limited type printing #601
Conversation
55b3941
to
7d727d3
Compare
51d31c1
to
2c7263b
Compare
I had local changes that I wanted to commit, and I had already amended them, so I just squashed and pushed up all the fixes. @aviatesk, could you take a second look? Also, I'm not sure how performant this pattern is: buf = IOBuffer()
_print_signature(buf, a, config; kwargs...)
write(io, Base_type_depth_limit(buf; maxdepth=2)); . Maybe it's fine? But someone who uses |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #601 +/- ##
==========================================
+ Coverage 89.42% 89.46% +0.03%
==========================================
Files 10 10
Lines 3008 3018 +10
==========================================
+ Hits 2690 2700 +10
Misses 318 318 ☔ View full report in Codecov by Sentry. |
I'll give another review on this PR next week since I'm quite busy until then. |
Sorry for being late to give a review. I will try to do this weekend or so. |
No problem 🙂 |
Bump 🙂 |
2c7263b
to
8105f34
Compare
Is there anything else needed for this PR? @aviatesk? |
I’m happy to put more work into it, I’m just not sure what else is needed |
I apologize for the delay in reviewing; I haven't had time to work on JET recently. The changes you've made look great. Thanks so much for contributing. |
Could you please add a test case to |
And don't care test failures on 1.11 and nightly-they are known to be broken and we can proceed as far as tests on v1.10 are passing. |
8105f34
to
fccb188
Compare
I'm happy to add tests for this. The first test I have that seems to work fine is simply: using Test
import JET
struct F49231{a,b,c,d,e,f,g}
num::g
end;
bar(x) = rand() > 0.5 ? x : Any[0][1]
mysum(x) = sum(y-> bar(x.num), 1:5; init=0)
@testset "Depth-limited type printing" begin
f = F49231{Float64,Float32,Int,String,AbstractString,6,Float64}(1)
Ftype = Tuple{Vector{typeof(f)}}
result = JET.report_opt(Ftype) do a
mysum(a[1]) # runtime dispatch !
end
buf = IOBuffer()
show(buf, result)
s = String(take!(buf))
@test occursin("F49231{…}", s)
end However, I'm remembering that one thing I was a bit dissatisfied about with this PR was hard-coding function print_signature(io, sig::Signature, config; kwargs...)
for a in sig
- _print_signature(io, a, config; kwargs...)
+ buf = IOBuffer()
+ _print_signature(buf, a, config; kwargs...)
+ # Let's use maxdepth=2, so that unnamed
+ # functions still show types we recognize.
+ write(io, Base_type_depth_limit(buf; maxdepth=2));
end
end
function print_frame_sig(io, frame)
mi = frame.linfo
m = mi.def
if m isa Module
Base.show_mi(io, mi, #=from_stackframe=#true)
else
buf = IOBuffer()
ioc = IOContext(buf, :backtrace=>true, :limit=>true)
Base.StackTraces.show_spec_sig(ioc, m, mi.specTypes)
io = IOContext(io, :backtrace=>true, :limit=>true)
write(io, Base_type_depth_limit(buf; maxdepth=2));
end
end Would you be opposed to adding |
The test case looks nice.
No, it would be nice to add the proposed |
Great 🙂, I pushed up the test. I also made |
Got it. I'll be adding a few follow-up commits myself. |
Thank you @aviatesk! This will be a quantum leap in quality of life / usability for us. We really look forward to the next release! |
src/ui/print.jl
Outdated
print_inference_success::Bool = true, | ||
fullpath::Bool = false, | ||
fullpath::Bool = false, | ||
stacktrace_types_limited::Bool = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stacktrace_types_limited::Bool = true, | |
stacktrace_types_limit::Int = 2, |
What if instead we made this an Int, and then users can customize how deep the type printing goes? We could document that stacktrace_types_limit = -1
or Inf
prints all the type information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I found 2 to be pretty reasonable, but perhaps that's not best for everyone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked out how type_depth_limit
is implemented, and it looks like when maxdepth
is nothing
, the depth is adjusted dynamically based on the display size. I think this flexible depth might work better in most cases, but what do you think? It might be reasonable to make stacktrace_types_limited::Union{Nothing,Int}=nothing
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me, so long it's toggle-able in some way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, could you make that change if you have time? Otherwise I will work on it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, thanks!
- recover printing color - define `stacktrace_types_limited` configuration
49bbb4f
to
6c815d6
Compare
I've made a few updates. Please feel free to add any adjustments. I believe we're nearly ready to merge. |
Alright, I pushed the changes, thanks again @aviatesk! |
Thanks a lot @charleskawczynski. This features is planned to be included in the next release. |
Thank you so much @aviatesk! |
Add depth-limited type printing.
This PR takes advantage of JuliaLang/julia#49795 by truncating the printing of types.
I'm sure that not all of these changes are needed, and we probably want to change or expose changing some parameters, but this reproducer does work for me:
Full reproducer
Results, main:
Results, this branch:
Closes #616.