Conversation
Codecov Report
@@ Coverage Diff @@
## master #212 +/- ##
==========================================
+ Coverage 81.48% 82.11% +0.63%
==========================================
Files 9 9
Lines 1118 1124 +6
==========================================
+ Hits 911 923 +12
+ Misses 207 201 -6
Continue to review full report at Codecov.
|
timholy
force-pushed
the
teh/various_fixes
branch
from
4b54d97 to
fb4c0aa
Compare
| The total width of the flamegraph is set from the `ROOT` node. | ||
| """ | ||
| function FlameGraphs.flamegraph(tinf::InferenceTimingNode; tmin = 0.0, excluded_modules=Set([Main::Module]), mode=nothing) | ||
| isROOT(tinf) && isempty(tinf.children) && error("root node has no children") |
There was a problem hiding this comment.
@timholy I just hit this error in usage of this package..
It seems to me that it's much better to just return an otherwise empty profile, with just the single ROOT node.
From what i can tell, without this check, it does return that empty profile correctly.
Can we remove this line? I'm curious if you remember the motivation to add it.
There was a problem hiding this comment.
I don't. I guess I was thinking the error explained what was going on, whereas a blank ProfileView window might just be confusing. For me the usual invocation is ProfileView.view(flamegraph(tinf)).
There was a problem hiding this comment.
Very happy to have you change this though!
There was a problem hiding this comment.
Yeah, makes sense. I think i was thinking that if it's empty, then it's empty! 😅 But you're right that it's not as nice as if it said "hey this is empty."
Maybe we can print a warning instead of throwing an error?
Our use case is that we're creating this via an HTTP endpoint on prod servers now (!!), and i don't want that to throw an error and return a 500:
JuliaPerf/ProfileEndpoints.jl#16
K, thanks, i'll send a quick PR
2 participants
These are things I fixed while using this in practice. Unfortunately I don't have tests for them (bad, bad), I guess when I encountered them in practice I felt it was important to keep moving.
Better to have them than not, I think.