feature: importing HPCToolkit data into Hatchet #126
Conversation
|
@draganagrbicllnl I'm going to review your code shortly, but, before I do, I wanted to let you know that you need to add license comments to the top of your new file. You can copy those comments from any of the other Python files. |
There was a problem hiding this comment.
I haven't run the code yet, so this is just a review based on reading the code.
Most of the code looks good. There are a few minor issues I've commented on, but, besides one possible bug in _read_cct, there's nothing major.
The biggest thing I'd want to see added to this PR is a unit test or two to make sure the reader is working as expected.
...22/proj-shared/wyphan/hpctoolkit/hpctoolkit/src/tool/hpcrun/gpu/gpu-application-thread-api.c
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Look good. I've only got a few more changes that should be made. This time, they're mostly related to the unit tests. The main code for the reader is pretty much good to go.
However, you need to fix formatting. The CI will not run unit tests unless the code satisfies the black and flake8 checks, and we won't be able to merge until we see the unit tests passing in CI. Regarding the flake8 checks, please rebase this PR on top of develop. Some changes just went in that will prevent certain failures related to flake8.
hatchet/tests/hpctoolkit_latest.py
Outdated
| assert len(graphframe.dataframe) == 10824 | ||
| assert "name" in graphframe.dataframe.columns | ||
| assert "realtime (i)" in graphframe.dataframe.columns | ||
| assert "realtime (e)" in graphframe.dataframe.columns |
There was a problem hiding this comment.
@pearce8 I know that, in the Caliper reader, we have a mapping of certain timing metrics to the names "time" and "time (inc)". Do we want to do something similar here?
There was a problem hiding this comment.
Similarly, we normally use the (inc) metric suffix for inclusive metrics and no suffix for exclusive metrics (at least those that aren't derived by Hatchet). @pearce8 should we continue using that convention here?
There was a problem hiding this comment.
@ilumsden @pearce8 I was using (i) for inclusive metric mapping, and (e) for exclusive metric mapping. I can update this to be consistent with the Caliper convention you used. Also, for HPCToolkit we support three time metrics when measuring application - realtime, cputime, and cycles. I can map them to time for consistency.
There was a problem hiding this comment.
@draganagrbicllnl I'd hold off on the time mapping for now until we hear back from @pearce8. As for the (i) and (e) part, it would probably be best to change them to (inc) and no suffix respectively. That convention isn't actually from Caliper. It's a Hatchet convention.
There was a problem hiding this comment.
@ilumsden @draganagrbicllnl Yes, lets go with (inc) and no suffix to match the Caliper convention.
ilumsden
added
area-readers
|
@ilumsden fixed the formatting |
draganaurosgrbic
force-pushed
the
hpctoolkit-integration
branch
2 times, most recently
from
eadf7f5 to
422ebbd
Compare
ilumsden
added
status-approved
|
Note that this PR now depends on #131 |
draganaurosgrbic
force-pushed
the
hpctoolkit-integration
branch
from
b69b937 to
e7d98ee
Compare
|
@michaelmckinsey1 rebased on develop |
No description provided.