Closed charleskawczynski closed 2 months ago
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 IOBuffer
s more regularly might recognize a better way.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 89.46%. Comparing base (
93f06e2
) to head (8105f34
). Report is 4 commits behind head on master.:exclamation: Current head 8105f34 differs from pull request most recent head 0f88de4. Consider uploading reports for the commit 0f88de4 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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 🙂
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 test/ui/test_print.jl
, using this commit as a reference: https://github.com/JuliaLang/julia/commit/a43ca052f2c9c29c812f02701600a9f6533507cb#diff-8d178520d08f2b4258b4bc414729ac2f1f0e65a27c176003f981cbd0b5b72e58? If it's too complicated, just let me know, and I'll take care of it.
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.
Could you please add a test case to test/ui/test_print.jl, using this commit as a reference: https://github.com/JuliaLang/julia/commit/a43ca052f2c9c29c812f02701600a9f6533507cb#diff-8d178520d08f2b4258b4bc414729ac2f1f0e65a27c176003f981cbd0b5b72e58? If it's too complicated, just let me know, and I'll take care of it.
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 maxdepth = 2
. Specifically:
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 maxdepth
to OptAnalyzer
, so that users can pass it in from the jetconfigs
kwargs?
The test case looks nice.
Would you be opposed to adding maxdepth to OptAnalyzer, so that users can pass it in from the jetconfigs kwargs?
No, it would be nice to add the proposed maxdepth
configuration to jetconfigs. Implementing a new configuration might be a bit tricky though, so if it feels too complicated, you can leave things as they are for now. We can always make it configurable in a follow-up.
Great 🙂, I pushed up the test. I also made maxdepth
to maxtypedepth
to avoid confusion with depth
. And then I moved maxtypedepth
to a const
Ref
, so that users can interactively change it if they want to set more or less type information. I think this is ready to go, unless you have any more suggestions.
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!
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 https://github.com/JuliaLang/julia/pull/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.