[rocprofiler-compute] Default PC sampling sort to count, add --pc-sampling-rows#7763
Conversation
…pling-rows - Default --pc-sampling-sorting-type to count so the analysis table leads with the most-sampled instructions (hotspots). - Add --pc-sampling-rows analyze option (default 10) to cap the PC sampling table; 0 or negative shows all rows. Co-Authored-By: Claude Opus 4 (1M context)
github-actions
Bot
added
documentation
- Validate --pc-sampling-rows as non-negative; 0 shows all rows. - Clarify the 0-means-all behavior in help text, code comment, and docs. Co-Authored-By: Claude Opus 4 (1M context)
- Merge the per-option PC sampling analyze tests into one function. - Assert the negative-rejection message via a mocked stderr instead of capsys. Co-Authored-By: Claude Opus 4 (1M context)
…rror - Assert argparse raises by mocking ArgumentParser.error instead of capturing stderr. Co-Authored-By: Claude Opus 4 (1M context)
- Drop the bespoke fixture and fold the num_rows cases into one parametrized test over the existing helper. Co-Authored-By: Claude Opus 4 (1M context)
There was a problem hiding this comment.
Pull request overview
This PR updates rocprofiler-compute’s PC sampling analysis defaults to better surface hotspots by default, and introduces a new row-limit option to keep CLI output manageable while ensuring consistent behavior across CLI and Web UI.
Changes:
- Defaulted
--pc-sampling-sorting-typefromoffsettocount. - Added
--pc-sampling-rows(default 10) and applied row limiting after sorting in the shared loader. - Updated docs/changelog and expanded unit tests to cover the new defaults and row limiting.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocprofiler-compute/src/utils/parser.py | Adds num_rows support and applies row limiting after sorting in the shared PC sampling loader. |
| projects/rocprofiler-compute/src/argparser.py | Changes default sorting to count and introduces --pc-sampling-rows with validation. |
| projects/rocprofiler-compute/tests/test_pc_sampling_analysis.py | Adds coverage for num_rows limiting behavior and updates args used by non-metrics table tests. |
| projects/rocprofiler-compute/tests/test_argparser.py | Adds coverage for default/override behavior of the new analyze options and validation of --pc-sampling-rows. |
| projects/rocprofiler-compute/docs/how-to/pc_sampling.rst | Documents new default sorting and the new --pc-sampling-rows option. |
| projects/rocprofiler-compute/CHANGELOG.md | Records the new option and the changed default sorting behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # num_rows of 0 (or None) means show all rows; argparse rejects negatives. | ||
| if num_rows: | ||
| df_sorted = df_sorted.head(num_rows) |
There was a problem hiding this comment.
Technically this could be called as public method but should not be used in such cases, this should have been screened for negatives already by the argparser. Optional suggestion to add.
| default="count", | ||
| type=str, | ||
| help="\t\tSet the sorting type of pc sampling: " | ||
| "offset or count (DEFAULT: offset).", | ||
| "offset or count (DEFAULT: count).", | ||
| ) |
| if parsed < 0: | ||
| raise argparse.ArgumentTypeError( | ||
| f"must be a non-negative integer (0 means all), got {parsed}" | ||
| ) |
cfallows-amd
left a comment
cfallows-amd
left a comment
There was a problem hiding this comment.
LGTM, please add two of the suggestions from copilot- update PR description to match implementation for negative error vs all, and the "choices" in argparser to add rigidity when specifying sort type. Marking approved since these are straightforward changes
| # num_rows of 0 (or None) means show all rows; argparse rejects negatives. | ||
| if num_rows: | ||
| df_sorted = df_sorted.head(num_rows) |
There was a problem hiding this comment.
Technically this could be called as public method but should not be used in such cases, this should have been screened for negatives already by the argparser. Optional suggestion to add.
Motivation
End users analyzing PC sampling mostly care about the top hotspots. Defaulting the sort to count surfaces the most-sampled instructions first, and a new --pc-sampling-rows option keeps the table from flooding the CLI by default while letting users widen it as needed.
Technical Details
JIRA ID
AIPROFCOMP-681
Test Plan
Test Result
Unit tests pass locally (199 passed). Ruff clean.
Submission Checklist