ROCm / rocprofiler-compute

Advanced Profiling and Analytics for AMD Hardware
https://rocm.docs.amd.com/projects/omniperf/en/latest/
MIT License
135 stars 49 forks source link

refactoring saved df files #124

Closed feizheng10 closed 1 year ago

feizheng10 commented 1 year ago

Save analysis dataframes in that loading function with verbose actually is not a bad idea! However, there are some reasons to refactoring the code a bit:

coleramos425 commented 1 year ago

Tip for formatting test:

python3 -m pip install black
cd omniperf/
black --diff --check . # check all files for any formatting discrepancies
black . # re-format files which don't comply
feizheng10 commented 1 year ago
black --diff --check .

Thanks! As long as we black with the same format style:)

feizheng10 commented 1 year ago
black --diff --check .

Thanks! As long as we black with the same format style:)

Just check some discussion back a while ago... I would suggest to use yapf instead of black. The reason is: a few ROCm components have being evaluated python formatting solutions and we are all on the same page that yapf default formatting is much better than black, even though it is not perfect.

More details you might follow:

yapf installation: sudo apt install yapf

yapf version >= 0.32.0

For your local file formatting: yapf --style pep8 -ip your_file

CI verification: find . -iname '*.py' | grep -v 'build/' | xargs -n 1 -P 1 -I{} -t sh -c 'yapf -ip --style pep8 {}

Disable/enable syntax:

# yapf: disable
...

# yapf: enable 
coleramos425 commented 1 year ago

I would suggest to use yapf instead of black.

Thanks for the suggestion. Omniperf uses black format to remain consistent with the rest of the Omni* toolset (i.e. Omnitrace). I'll need to evaluate performance differences