-
Notifications
You must be signed in to change notification settings - Fork 227
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
Profiler: Improve compatibility with Pluto.jl and friends. #2139
Conversation
By using a results object instead of writing to stdout.
ed061f9
to
b8d5545
Compare
print(stdout isa Base.TTY ? IOContext(stdout, :limit => true) : stdout, data; kwargs...) | ||
function print(io::IO, data; trace=false, raw=false) | ||
data = deepcopy(data) | ||
function Base.show(io::IO, results::ProfileResults) |
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.
Now it will be a long day before I understand our show methods hierarchy, but should this be?
function Base.show(io::IO, results::ProfileResults) | |
function Base.show(io::IO, ::MIME"text/plain", results::ProfileResults) |
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'm not sure we do, in this case atleast. Basically, you want the ProfileResults to be rendered, almost always. So when doing display
, which IIUC is MIME"text/plain"
, but also when doing string
or print
(which I think doesn't use a MIME input?), or when rendering in Pluto (which, again IIUC, uses text/html
).
I'm probably misunderstanding though, as I'm not very familiar with the show method hierarchy either, but I did verify the above cases and the output is rendered correctly.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2139 +/- ##
==========================================
- Coverage 72.19% 72.11% -0.09%
==========================================
Files 159 159
Lines 14386 14382 -4
==========================================
- Hits 10386 10371 -15
- Misses 4000 4011 +11
☔ View full report in Codecov by Sentry. |
ref JuliaGPU/CUDA.jl#2139 Probably should add methods to CUDA.jl to access the kernel times instead of grabbing them directly from the dataframe.
ref JuliaGPU/CUDA.jl#2139 Probably should add methods to CUDA.jl to access the kernel times instead of grabbing them directly from the dataframe.
By using a results object instead of writing to stdout.
As suggested by @vchuravy in https://discourse.julialang.org/t/how-would-i-retrieve-the-result-from-cuda-profile/105548
@thomasfaingnaert This removes
CUDA.@profiled
again, as the results are now returned byCUDA.@profile
. I would also suggest adding some accessors, e.g.,kernel_times(::ProfileResults)
(or an iterator), to avoid breaking GemmKernels.jl over and over again.