Adds an annotation-based profiling API for measuring the performance of Hatchet and Thicket#142
Adds an annotation-based profiling API for measuring the performance of Hatchet and Thicket#142ilumsden wants to merge 8 commits intollnl:developfrom
Conversation
…nd annotations all of Hatchet
ilumsden
added
area-utils
ilumsden
force-pushed
the
configurable_annotations
branch
from
9cd3fb6 to
856e382
Compare
ilumsden
force-pushed
the
configurable_annotations
branch
from
d62c31c to
3345880
Compare
pearce8
requested review from
cscully-allison,
dyokelson,
michaelmckinsey1 and
slabasan
dyokelson
left a comment
dyokelson
left a comment
There was a problem hiding this comment.
A few changes requested but overall looks pretty good - my big picture concern is that going forward we will have to add the annotation decorator to every single function for any new functionality we add? Is there a way to automate it?
|
|
||
|
|
||
| _hpctk_reader_mod_annotate = annotate(fmt="hpctoolkit_reader.{}") | ||
| _hpctk_reader_annotate = annotate(fmt="HPCToolkitReader.{}") |
There was a problem hiding this comment.
I could see the naming for fmt to be kind of confusing for someone seeing these two naming conventions - we are basically wanting to separate anything not in the class from the functionality within the class right? Is there a better way to name the regions to be more intuitive? Maybe mod could be fmt="hpctoolkit_external" or something to imply it's external to the class functionality, or it could be generically annotated like in node.py, graphframe.py, and string_dialect.py where you just annotate() the function outside the class.
|
|
||
|
|
||
| _hpctk_reader_latest_mod_annotate = annotate(fmt="hpctoolkit_reader_latest.{}") | ||
| _hpctk_reader_latest_annotate = annotate(fmt="HPCToolkitReaderLatest.{}") |
There was a problem hiding this comment.
Similar comment here as to the old hpctoolkit_reader.py - let's rename of of the annotation regions to more obviously separate the two.
|
|
||
|
|
||
| _dataframe_writer_mod_annotate = annotate(fmt="dataframe_writer.{}") | ||
| _dataframe_writer_annotate = annotate(fmt="DataframeWriter.{}") |
There was a problem hiding this comment.
Similar comment here as to hpctoolkit readers - let's rename the annotated region. I think just changing the case is kind of confusing to imply it is external to the class...another idea would be to add a qualifier like "module" or "class" to the annotation string possibly? Just an idea.
hatchet/util/perf_measure.py
Outdated
| return "caliper" | ||
| elif plugin_name.lower() == "perfflowaspect" and _PYCALIPER_AVAILABLE: | ||
| return "perfflowaspect" | ||
| return "none" |
There was a problem hiding this comment.
This looks like it will fail silently - what if someone wants perfflowaspect and sets the env var but the library is not installed correctly? We should probably display a helpful error message in cases like that (somewhat like how you display caliper information in your prints maybe).
hatchet/util/perf_measure.py
Outdated
| print("Caliper is available?", _PYCALIPER_AVAILABLE) | ||
| if plugin_name.lower() == "caliper" and _PYCALIPER_AVAILABLE: | ||
| return "caliper" | ||
| elif plugin_name.lower() == "perfflowaspect" and _PYCALIPER_AVAILABLE: |
There was a problem hiding this comment.
should this line say and _PERFFLOWASPECT_AVAILABLE
hatchet/util/perf_measure.py
Outdated
|
|
||
| def _validate_perf_plugin(plugin_name): | ||
| print("Perf Plugin =", plugin_name) | ||
| print("Caliper is available?", _PYCALIPER_AVAILABLE) |
There was a problem hiding this comment.
This print line could be moved inside the if-statement below, and a similar one could be used for perfflowaspect within it's if-statement to be more verbose (see comment for line 31)
2 participants
This PR adds APIs that can be used to measure Hatchet and Thicket's performance. By default, these APIs will do nothing. However, users can set the
HATCHET_PERF_PLUGINenvironment variable to enable these profiling APIs with a specified profiling tool. Currently, the following values forHATCHET_PERF_PLUGINare recognized:caliper: use the Caliper Python bindings from Adds Python bindings to Caliper with pybind11 Caliper#573perfflowaspect: use PerfFlow Aspectnone: disable profiling APIs. This is what is used when the environment variable is not setThese values for
HATCHET_PERF_PLUGINare case-insensitive.