Adds an annotation-based profiling API for measuring the performance of Hatchet and Thicket #142
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
dyokelson,
cscully-allison,
michaelmckinsey1 and
slabasan
|
|
||
|
|
||
| src_file = 0 | ||
|
|
||
|
|
||
| _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.
| @@ -19,6 +20,11 @@ | |||
| ABC = ABCMeta("ABC", (object,), {"__slots__": ()}) | |||
|
|
|||
|
|
|||
| _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)
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.