-
Notifications
You must be signed in to change notification settings - Fork 49
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 report_invalidations
to tabulate invalidations summary
#303
Add report_invalidations
to tabulate invalidations summary
#303
Conversation
Codecov ReportBase: 84.08% // Head: 83.70% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #303 +/- ##
==========================================
- Coverage 84.08% 83.70% -0.39%
==========================================
Files 17 18 +1
Lines 2162 2221 +59
==========================================
+ Hits 1818 1859 +41
- Misses 344 362 +18
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
bump 🙂 |
Bump! 🙂 |
I think we could have this, but I'd rather it be loaded under a
as a "stub" in the main package, and then in function __init__()
# other stuff
@require PettyTables = sha include("tabulated.jl")
end and put the |
f4eab32
to
3082d33
Compare
Ok, this now works I think as expected, here's a mwe (assuming some packages are loadable): import SnoopCompileCore
invalidations = SnoopCompileCore.@snoopr begin
using ForwardDiff
end;
import SnoopCompile
using PrettyTables # needed for report_invalidations to be defined
SnoopCompile.report_invalidations(;
invalidations,
process_filename = x -> last(split(x, ".julia/packages/"))
) Which produces
|
c26a6e3
to
6bfd41d
Compare
6bfd41d
to
a2f6e12
Compare
Based on JuliaDiff/ForwardDiff.jl#620 (comment) and JuliaDiff/ForwardDiff.jl#620 (comment), should we update how |
report_invalidations
to tabulate invalidations summary
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.
This looks nearly merge-worthy. I don't think there are big changes needed in how you collect the invalidations, because:
- You suggest collecting them with SnoopCompileCore rather than SnoopCompile ✔️
- Nothing here uses a workload. Of course this summary will include many invalidations that may not affect the users' code.
filtermod
andprecompile_blockers
can restrict them to ones that affect particular modules or workloads. Are you wondering if you should support them?
src/invalidations.jl
Outdated
|
||
Print a tabular summary of invalidations given: | ||
|
||
- `job_name::String` the name of the job |
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.
What's the role of the job_name? Does it have any functional significance? (Can I choose a name that's wrong or broken?)
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.
You know, I think we can get rid of it. It's not really doing anything. I added it in case I'd run several invalidation jobs, so that the tables could be differentiated, but 1) that can be printed before the call to report_invalidations
and 2) I think they'd need to be separate sessions anyway. I'll remove it
src/invalidations.jl
Outdated
- `job_name::String` the name of the job | ||
|
||
An `@info` message also prints some information, for example | ||
if the table has been truncated (upon request). |
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.
This might be interpreted as saying "you have to request the @info
message" whereas I think you mean "it was truncated by request," which in turn really means "it was truncated by specifying nrows
."
But even this is confusing since nrows
has a default value of 10, so you don't actually have to request it.
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.
Very good feedback. I'm thinking we can skip this comment and just do a better job documenting n_rows
. I'll update to something that I think is clearer/simpler
Co-authored-by: Tim Holy <[email protected]>
I think one last thing that we could do, that would really put this over the top in terms of ease of use, would be if we followed through with CliMA/ReportMetrics.jl#7 (comment), and made the file / line number hypertext links through PrettyTables (which I think is supported here). I think this can be a followup feature, though. Thumbs up to open an issue 🙂 |
Thanks! This is a nice addition. Release of SnoopCompile 2.10.0 is underway. |
Hello 👋🏻! There has been a lot of activity around hunting down invalidations lately, for example:
As mentioned in this comment of the
discourse thread
, I've added a function calledreport_invalidations
to ReportMetrics.jl which summarizes the output ofSnoopCompileCore.@snoopr
with quantitative and actionable information.I'd like to call this function from the
julia-invalidations
github action. I thought it might be cleaner to add this functionality to SnoopCompile directly (rather than a separate package) since it's relatively light weight (barring PrettyTables) and uses SnoopCompile internals.The
ReportMetrics.jl
version of the printed table looks like this: