Repository navigation
feat(profiler): Wire hipFile as a rocprofiler-systems dependency - #7982
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
🟢 Approval recommended
The dependency wiring is consistent across CMake, topology, artifact fetching, and packaging, and includes a targeted regression test to prevent drift.
Pull request overview
This PR updates TheRock’s build topology and packaging so rocprofiler-systems can reliably resolve and link against hipFile (for optional GPU-direct storage I/O telemetry), aligning with how other non-base runtime dependencies are managed in the super-project.
Changes:
- Export hipFile CMake package metadata for both
find_package(hipfile)andfind_package(HIPFILE). - Wire hipFile into
rocprofiler-systemsbuild/runtime dependency wiring and build topology, while keeping examples optional viaIGNORE_PACKAGES. - Update artifact fetching logic, add a regression test for the fetch argv, and add Linux package Requires on
amdrocm-hipfile.
File summaries
| File | Description |
|---|---|
| test_tools/therock_consumer_graph.json | Records rocprofiler-systems as a hipFile consumer in the test consumer graph. |
| storage-libs/CMakeLists.txt | Advertises hipFile’s CMake package for both hipfile and HIPFILE names. |
| profiler/CMakeLists.txt | Adds hipFile as a rocprofiler-systems dependency, enables AUTO hipFile support, and ignores hipFile for examples’ optional detection. |
| CMakeLists.txt | Clarifies ordering rationale: storage-libs before profiler due to hipFile discovery. |
| BUILD_TOPOLOGY.toml | Adds hipfile as an artifact dependency of rocprofiler-systems. |
| build_tools/tests/install_rocm_from_artifacts_test.py | Adds coverage ensuring --rocprofiler-systems pulls hipFile + sysdeps-util-linux, while examples do not. |
| build_tools/packaging/linux/package.json | Adds amdrocm-hipfile dependency to profiler packages. |
| build_tools/install_rocm_from_artifacts.py | Ensures hipFile + sysdeps-util-linux are fetched when installing rocprofiler-systems artifacts. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e7ce8a3 to
ab1d4ae
Compare
dgaliffiAMD
left a comment
There was a problem hiding this comment.
Approving for the profiler CMakeLists changes.
|
@arvindcheru, do you mind reviewing the packaging changes? |
arvindcheru
left a comment
There was a problem hiding this comment.
From Native Linux Packaging side:
- Native linux packaging of existing hipFile packages looks good to me; the new profiler→hipFile Depends is the right TheRock pattern and did not break CI Tests, but it is not yet proven as a real loader/install edge until 10630 lands and you check profiler-only install + readelf.
- Additional full install Tests successful with this changes -https://github.com/ROCm/TheRock/actions/runs/35110831966
- Noted new pkg dependencies updated (https://therock-ci-artifacts.s3.amazonaws.com/35037941122-linux/packages/deb ) amdrocm-profiler10.2 and amdrocm-profiler-test10.2 now Require amdrocm-hipfile10.2
Thanks @arvindcheru . We will keep the hard Depends so |
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
|
@ScottTodd and @arvindcheru - I have resolved and fixed the code as per the review comments. Please re-review and approve as appropriate. |
|
Please re-request review if you want to appear back in code review queues (e.g. https://github.com/pulls/reviews) |
Thanks for the responses on the packaging dependency, but since it may impact the overall usage of profiler, want to get it notified/confirmed explicitly here Packaging + topology: hard hipFile on the profiler path Native packages What that means If the product intent is “profiler installs/builds without hipFile; telemetry off until hipFile is added,” topology artifact_deps, profiler-apps → storage-libs, and JSON Depends would need to be optional/Recommends. Would request to Check this again and confirm the hard install+topology edge is intended as product design with the known challenge of hard dependency. |
Thanks @arvindcheru for your guidance related to packaging. Agreed - hipFile is a runtime dlopen, not a package hard requirement.
Users who profile storage I/O typically already have hipFile (e.g. via
|
arvindcheru
left a comment
There was a problem hiding this comment.
Native Linux Packaging review: approve. (6f0912d)
Native Linux packaging is consistent with the agreed product: amdrocm-profiler does not hard-Depends amdrocm-hipfile (dlopen, full amdrocm already installs hipFile). amdrocm-profiler-test Does Requires hipFile. No copy of libhipfile into the profiler artifact. Topology/CI fetch of hipFile for find_package is in place.
Examples fetch + UT now include hipFile; install-script comment is dlopen. Optional: drop the stale PR note that examples-only fetch does not imply hipFile.
Merge before systems#10630 is in TheRock. After that bump, readelf -d librocprof-sys.so should not show libhipfile.so.0.
Kindly request to confirm other reviews to complete before proceeding to merge.
ScottTodd
left a comment
There was a problem hiding this comment.
LGTM for the build system and CI.
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # profiler/CMakeLists.txt
Pass -DROCPROFSYS_USE_HIPFILE=ON when hipFile is enabled, gate the hipfile runtime dep on that flag, and declare it on rocprofiler-systems-examples so telemetry cannot silently drop and the hipFile sample is actually built. Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # profiler/CMakeLists.txt
Removing unwanted comments.
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # .github/workflows/multi_arch_build_portable_linux.yml
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # build_tools/tests/install_rocm_from_artifacts_test.py
6f0912d to
673e2cc
Compare
## Motivation <!-- Explain the purpose of this PR and the goals it aims to achieve. --> ROCm Systems Profiler should be able to provide storage telemetry - letting users correlate file/buffer registrations, read/write throughput, fast-path vs. fallback usage, and I/O errors with GPU and application activity in a single Perfetto trace or RocPD database. This is increasingly important for GPU-direct storage workloads where the storage path is a common performance bottleneck that was previously invisible in the profiler. Depends on: - [x] #8252 - [x] #9584 - [x] #10548 - [x] ROCm/TheRock#7982 Todo: - [x] Ensure version gating for hipFile public API on 0.5.0? ## Technical Details <!-- Explain the changes along with any relevant GitHub links. --> - **hipFile backend wrapper** (`source/lib/backends/hipfile/`) - Thin policy seam around `hipFileGetStatsL3`; isolates the profiler from hipFile's C API and enables mock-based unit tests. - **PMC collector** (`library/pmc/collectors/hipfile/`) - Snapshots hipFile's per-GPU stats on the process-sampling thread and stores the whole snapshot as a single typed `hipfile_pmc_sample` per GPU per poll. - **Trace-cache sample type (`source/lib/core/trace_cache/`) - Adding the typed sample follows the existing `gpu_pmc_sample` / `ainic_pmc_sample` pattern and uses the shared plumbing. - **Metric groups and defaults** - `ROCPROFSYS_HIPFILE_METRICS` selects read/write pairs by group (`fastpath`, `fallback`, `bandwidth`, `bytes`, `ops`, `unaligned`, `errors`, or `all`/`none`). - Defaults to `fastpath, fallback, bandwidth, bytes, errors` so traces focus on path usage and transfer rate without collecting every counter. - `ROCPROFSYS_HIPFILE_METRICS=none` skips device enumeration.` - **Build-time and runtime gating** - `ROCPROFSYS_BUILD_HIPFILE` is tri-state `AUTO` / `ON` / `OFF` (default `AUTO`). - `cmake/Packages.cmake` sets a derived INTERNAL `ROCPROFSYS_HIPFILE_SUPPORT` (AINIC-style) and never shadows the user cache: AUTO + missing/too-old package → disable, STATUS ON + missing/too-old package → FATAL_ERROR (version found, version required, hipfile_DIR / CMAKE_PREFIX_PATH) OFF → never search - `ROCPROFSYS_USE_HIPFILE` (env/config, default OFF) enables collection at run time; zero overhead when disabled or when the target does not use hipFile. - The backend compiles against `hipfile.h` but does not link `libhipfile`. `wrapper.hpp` resolves the two entry points it uses (`hipFileGetVersion`, `hipFileGetStatsL3`) with `dlopen`/`dlsym` at first sample. - Consequently `libhipfile.so.0` is not required for the profiler to start. Absent library, missing stats API (`hipFile` 0.4 reuses SONAME .so.0), or a too-old reported version all degrade to a warning and no hipFile tracks. - **Runtime guards** - Does not set `HIPFILE_STATS_LEVEL` (hipFile already defaults to 1). `USE_HIPFILE=ON` with `HIPFILE_STATS_LEVEL=0`, or with `USE_PROCESS_SAMPLING=OFF`, logs a warning and produces no hipFile tracks. - **Version checks** - Requires hipFile 0.5.0+ at build time (for `hipFileGetStatsL3`) and re-checks the loaded `libhipfile` at run time; logs a warning and skips collection if the runtime is too old. - **Example workload and validation** - `examples//hipfile/hipfile-io.cpp` runs a short read/write loop for telemetry validation. - `hipfile-io` is skipped when `ROCPROFSYS_HIPFILE_SUPPORT` is off - `tests/pytest/test_hipfile_telemetry.py` plus RocPD/Perfetto validation rules under `tests/rocpd-validation-rules/hipfile-telemetry/`. - **Documentation and changelog** - How-to: `docs/how-to/hipfile-telemetry.rst` (build/run settings, metric interpretation, linking contract, troubleshooting). - CHANGELOG entry under the unreleased 1.9.0 section. - **Follow-up for future** - Expose remaining hipFile telemetry that is already collected but not shown to the user. - Emit process-scoped file/buffer registration counters on a process track (as the CPU collector does), if useful. - Add `rocprof-sys-avail` reporting of which hipFile metrics can be collected. - Add a `--hipfile[=metrics]` domain flag so storage telemetry composes with existing presets (do not enable hipFile inside `trace-gpu` / `--gpu`; no dedicated storage preset until that flag exists). - When hipFile fills per-slot UUID (AIHIPFILE-272), join those to SDK agents (same as AMD SMI) so permutations/HIP_VISIBLE_DEVICES UUID lists map correctly without parsing the env var. ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> JIRA ID: AIPROFSYST-710 ## Test Plan <!-- Explain any relevant testing done to verify this PR. --> - [ ] Unit tests: `hipfile-backend-tests`, `pmc-hipfile-collector-tests` - [ ] Integration: `ctest -R hipfile` / `pytest tests/pytest/test_hipfile_telemetry.py` (requires GPU + hipFile build) - [ ] Manual: profile a hipFile workload with `ROCPROFSYS_USE_HIPFILE=ON` and confirm `hipFile GPU<N> <metric>` tracks in Perfetto/RocPD ## Test Result <!-- Briefly summarize test outcomes. --> <img width="4062" height="952" alt="image" src="https://github.com/user-attachments/assets/db32f269-ad9e-4649-a958-807e72b604e7" /> <img width="4624" height="1164" alt="image" src="https://github.com/user-attachments/assets/d137757a-c3e9-4e4d-8ac8-b8201c57f6ea" /> ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: David Galiffi <David.Galiffi@amd.com> Co-authored-by: David Galiffi <David.Galiffi@amd.com>
Motivation
rocprofiler-systems compiles in GPU-direct storage I/O telemetry when hipFile is present (
ROCPROFSYS_USE_HIPFILE=ON). It find_packages hipFile for<hipfile.h>and the≥ 0.5.0gate; it does not DT_NEEDED libhipfile.so.0. The collector dlopenslibhipfile.so.0on first sample. The storage telemetry capability is built always and the runtime checks are performed at first sample.hipFile is its own
storage-libsartifact, not part of the shared test-prefix base set, so TheRock must declare, fetch, and stage it like other non-base deps.This PR is the TheRock wiring. Merge it before (ROCm/rocm-systems#10630) lands in TheRock via a submodule bump; otherwise find_package(hipfile) hits the strict provider.
JIRA ID: AIPROFSYST-710
Roadmap: ROCM-150
Super epic: ROCM-4010
Technical Details
find_package(hipfile)from the hipFile subproject (provide_packagefor bothhipfileandHIPFILE), matching hip-clr’ship/HIP.THEROCK_ENABLE_HIPFILEis on, addhipfileas aRUNTIME_DEPofrocprofiler-systemsandrocprofiler-systems-examples, and as an artifactSUBPROJECT_DEPSentry. Pass-DROCPROFSYS_USE_HIPFILE=ONso a missing or too-old package fails configure instead of silently dropping telemetry. When hipFile is off, passOFF.rocprofiler-systemsartifact_depsincludeshipfile; theprofiler-appsgroup depends onstorage-libs. Linux CIprofiler-appsneeds: storage-libssohipFileis fetched from that stage instead of rebuilt in parallel.amdrocm-profilerpackage does notRequires: amdrocm-hipfileandamdrocm-profiler-testRequires: amdrocm-hipfileso telemetry feature is not missed from the test artifact.hipfile→rocprofiler-sdk,rocprofiler-systems, androcprofiler-systems-examplesintherock_consumer_graph.json(generated from thoseRUNTIME_DEPS).Note:
base_artifact_pattern(storage-libs/hipfile_lib, notprofiler-core).--rocprofiler-systemstherefore appendshipfileandsysdeps-util-linuxso tests can dlopenhipFile(hipFile itselfDT_NEEDEDslibrocm_sysdeps_mount.so.1). Examples-only fetch still does not imply hipFile.Test Plan
Test Result
Submission Checklist