Enhancement: Add rocprofv2 trace support for AMD GPUs - #817
Open
Shenghsun Cho (shcho) wants to merge 12 commits into
Open
Enhancement: Add rocprofv2 trace support for AMD GPUs#817Shenghsun Cho (shcho) wants to merge 12 commits into
Shenghsun Cho (shcho) wants to merge 12 commits into
Conversation
- runner.py: Add SB_ENABLE_ROCPROF/SB_ROCPROF_TRACE_DIR env vars to enable rocprofv2 profiling (--hip-trace --kernel-trace --plugin json) in local, torch.distributed, and mpi modes - pytorch_base.py: Extend GPU guard to support ROCm (torch.version.hip) so PyTorch profiler works on AMD GPUs
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds ROCm profiling support alongside existing Nsight Systems tracing and updates model benchmark GPU detection to include AMD (HIP) builds.
Changes:
- Add
rocprofv2command injection (gated by env vars) for local/distributed/mpi runner modes. - Introduce ROCm trace directory env var support (
SB_ROCPROF_TRACE_DIR). - Expand PyTorch GPU check to treat HIP builds as GPU-capable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| superbench/runner/runner.py | Adds optional rocprofv2 profiling prefixes/trace commands controlled by environment variables. |
| superbench/benchmarks/model_benchmarks/pytorch_base.py | Updates GPU detection to include ROCm/HIP PyTorch builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
added 2 commits
May 19, 2026 18:46
Address PR review: wrap all interpolated path/name segments in shlex.quote() to prevent command injection or broken commands when paths contain whitespace or shell metacharacters. Applied to both nsys and rocprofv2 trace commands across all three execution modes (local, torch.distributed, mpi).
Address PR review: the variable holds either an nsys or rocprofv2 prefix, so rename to trace_prefix to avoid implying Nsight-only behavior.
added 2 commits
July 7, 2026 18:19
Address PR review: torch.version.hip (and .cuda) may not exist on all PyTorch builds/versions, raising AttributeError. Use getattr with a None default to make the check compatible across all PyTorch builds.
Address PR review: add the conventional -- end-of-options separator after rocprofv2 flags so that benchmark command arguments starting with - are not misinterpreted as rocprofv2 options. Applied to all three execution modes (local, torch.distributed, mpi).
Author
|
@microsoft-github-policy-service agree company="Microsoft" |
Hongtao Zhang (polarG)
requested changes
Jul 21, 2026
added 5 commits
July 22, 2026 21:25
Address PR review: shlex.quote() wraps values with spaces or shell metacharacters in single quotes, which would terminate the outer bash -lc '...' quoting used to dispatch commands. Add a helper that runs shlex.quote() and then replaces each ' with '\'' so the quoted value is safe to embed inside the outer single-quoted command string. Applied to all six nsys/rocprofv2 trace output paths.
Address PR review: previously SB_ENABLE_ROCPROF was silently ignored whenever SB_ENABLE_NSYS was also set, producing no traces and no diagnostic. Log a warning documenting that nsys takes precedence and explicitly disable rocprofv2 in that case.
Address PR review: clarify that ProfilerActivity.CUDA and event.cuda_time are aliases that cover HIP kernels on ROCm builds of PyTorch, so the same profiler code path works for both NVIDIA and AMD GPUs. Verified end-to-end on MI300x with ROCm 6.3.4.
Address PR review: the nsys/rocprofv2 selection + path quoting logic was copy-pasted across all three mode branches (local, torch.distributed, mpi). Extract __build_trace_command() so future changes to profiler flags, quoting, or additional tools live in a single place.
Apply yapf column-limit reflows and mark the _quote_for_bash_lc docstring as raw (D301) since it contains backslash escapes.
Shenghsun Cho (shcho)
force-pushed
the
dev/choshenghsun/reliaguard-tracegen-mi300x
branch
from
July 22, 2026 22:51
a81f9be to
b4b5e54
Compare
rocprofv2 does not recognize the conventional -- separator and errors with 'Wrong option "--", Please use the following options:'. Drop the separator; rocprofv2 treats the first non-option token as the target executable, matching the syntax originally validated on MI300x. Add a comment noting the incompatibility so it is not reintroduced.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
superbench/benchmarks/model_benchmarks/pytorch_base.py:594
- Grammar/capitalization in the comment: use “an NVIDIA” (not “a Nvidia”) for correct article usage and consistent vendor capitalization.
# Check if this is a Nvidia or AMD GPU
gusui-msft
approved these changes
Aug 6, 2026
Hongtao Zhang (polarG)
approved these changes
Sep 1, 2026
Address PR review: cover _quote_for_bash_lc round-trips through both bash -lc and bash -c wrappers with values containing spaces, single quotes, command substitution, backticks, semicolons, and pipes. Also add tests for __get_mode_command with SB_ENABLE_ROCPROF and SB_ENABLE_NSYS, plus mutual-exclusion precedence.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The core rocprofv2/ROCm enablement and added tests look correct, with only minor doc/comment nits to address if desired.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
superbench/benchmarks/model_benchmarks/pytorch_base.py:594
- Grammar/capitalization: "a Nvidia" should be "an NVIDIA".
superbench/runner/runner.py:132 - Docstring says
benchmark_name/suffixare used in an "output filename", but for rocprofv2 the value is an output directory (-d) and for nsys it’s an output path prefix. Clarifying this avoids confusion about what the constructed string represents.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extend trace generation to support AMD GPUs using rocprofv2