Repository navigation
fix(rocprofiler-compute): Gate rocprof-compute analyze deps behind an opt-in compute-analyze extra - #7002
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.
Pull request overview
This PR updates the rocm-profiler wheel template so it declares runtime dependencies needed by rocprof-compute analyze, by reading them from the staged rocprofiler-compute payload during wheel build. This aligns the advertised install path (pip install rocm[profiler]) with actual runtime requirements.
Changes:
- Add build-time parsing of
libexec/rocprofiler-compute/requirements.txtfrom the staged wheel payload and pass it viainstall_requires. - Document the new dependency sourcing behavior in the template README.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| build_tools/packaging/python/templates/rocm-profiler/setup.py | Reads rocprofiler-compute’s staged requirements.txt and wires it into install_requires for the rocm-profiler wheel. |
| build_tools/packaging/python/templates/rocm-profiler/README.md | Updates packaging notes to reflect that dependencies are sourced from the staged payload file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f220f23 to
fbf56f8
Compare
jbonnell-amd
left a comment
There was a problem hiding this comment.
Assuming we are good with adding all of requirements.txt dependencies into the rocm-profiler install, should we also do a follow-up PR to add in requirements for sdk and systems as well? We would also need to eliminate duplicates and ensure version requirements do not mismatch, but this would resolve any other potential dependency issues we'd have.
@jbonnell-amd I dont think systems has any requirements.txt python deps and sdk is not part of rocm-profiler package |
|
@jbonnell-amd per my comment in ROCM-28653 we should not merge this PR since compute's python dependencies will clash with those of torch and the python application being profiled. Folks would be better of creating a virtual environment for analysis mode based of our requirements.txt and a message in analysis mode instructing users to do so along with the path to requirements.txt should be enough |
If you don't want to merge this, maybe close or move to draft? |
fbf56f8 to
17d51db
Compare
17d51db to
1021a68
Compare
I think we can still merge this PR if we don't use I have implemented this change. |
@jbonnell-amd @vedithal-amd how do you want to proceed here? |
Bit busy with other priority items, will circle back here. But high level - we really need Basically, folks need an easy way to create python venv with compute analyze deps pulled in instead of running pip install /path/to/requirements |
1021a68 to
844a6c1
Compare
|
Hi @ScottTodd and @marbre could you please review this PR. The idea is to support Note that However, Currently, folks need to run |
The rocm-profiler wheel shipped no dependency metadata, so `rocprof-compute analyze` failed at import with dash, pandas and numpy missing. Declaring them as install_requires would fix analyze at the cost of profile mode: this wheel installs into the environment being profiled, and compute's requirements.txt pins exact versions (numpy==1.26.4, pandas==2.2.3) that fight torch there. Analyze is meant to run from its own venv, so the deps go under an extra and the pins stay appropriate. The list is read from the staged rocprofiler-compute payload's requirements.txt, keeping rocprofiler-compute the single source of truth. The extra is named for the tool, not the mode, because this wheel carries rocprofiler-systems too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
844a6c1 to
e5f6eb7
Compare
ScottTodd
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me now.
I checked the generated whl from the CI build:
- https://github.com/ROCm/TheRock/actions/runs/35145382838?pr=7002
- https://therock-ci-artifacts.s3.amazonaws.com/35145382838-linux/python/index.html
- https://therock-ci-artifacts.s3.amazonaws.com/35145382838-linux/python/rocm_profiler-10.2.0.dev0%2B6ee43d2fadf65113c3c23212e823c93b6b953281-py3-none-linux_x86_64.whl
Here's the metadata:
Metadata-Version: 2.4
Name: rocm-profiler
Version: 10.2.0.dev0+6ee43d2fadf65113c3c23212e823c93b6b953281
Summary: ROCm profiler applications (rocprofiler-systems and rocprofiler-compute)
Requires-Python: >=3.8
Provides-Extra: compute-analyze
Requires-Dist: astunparse==1.6.2; extra == "compute-analyze"
Requires-Dist: dash-bootstrap-components==2.0.4; extra == "compute-analyze"
Requires-Dist: dash-svg==0.0.12; extra == "compute-analyze"
Requires-Dist: dash==3.4.0; extra == "compute-analyze"
Requires-Dist: numpy==1.26.4; python_version < "3.13" and extra == "compute-analyze"
Requires-Dist: numpy==2.5.2; python_version >= "3.13" and extra == "compute-analyze"
Requires-Dist: pandas==2.2.3; extra == "compute-analyze"
Requires-Dist: plotext==5.3.2; extra == "compute-analyze"
Requires-Dist: plotille==5.0.0; extra == "compute-analyze"
Requires-Dist: pyyaml==6.0.3; extra == "compute-analyze"
Requires-Dist: sqlalchemy==2.0.46; extra == "compute-analyze"
Requires-Dist: tabulate==0.9.0; extra == "compute-analyze"
Requires-Dist: textual==7.3.0; extra == "compute-analyze"
Requires-Dist: textual_plotext==1.0.1; extra == "compute-analyze"
Dynamic: provides-extra
Dynamic: requires-python
Dynamic: summary
Install rocm-profiler[compute-analyze] into its own venv in the wheel test workflow and run `rocprof-compute analyze --help`, which imports the whole extra and so fails if a dependency is missing. Add --extra-index-url to setup_venv.py for this: the extra's dependencies come from PyPI, which --index-url alone replaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed the CI coverage for the extra. The new steps are in the
Worth confirming these are green before merge. The install step resolves the analyze pins from PyPI, so a failure there is dependency resolution rather than anything wrong with the wheel. |
CI passes an empty --index-url and installs from --find-links, which meant the compute-analyze step asked for no index and an extra index at once. Promote the extra to the index in that case instead of failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Seems like the Merging this PR |
…ute-analyze extra (#11928) ## Motivation Analyze mode needs Python packages that profile mode does not. There was no way to check whether an environment had them without also having a workload to analyze, because the existing check only ran on the way into an analysis. The failure message also pointed at the requirements file, which lists the pins but not how to install them. ### Validated using latest nightly ``` python3 -m venv ~/.venvs/rocprof-compute-analyze source ~/.venvs/rocprof-compute-analyze/bin/activate pip install --extra-index-url https://nightly.repo.amd.com/rocm/whl-next/ "rocm-profiler[compute-analyze]==10.2.0a20260922" ``` The rocprof-compute along with its python dependencies are installed in the venv - so no more manually pip installing the requirements.txt ## Technical Details - `rocprof-compute analyze --verify-deps` runs the existing `verify_deps()` check and exits: zero when every pin is installed at the required version, non-zero otherwise. `-p` stays optional, so the flag works as a standalone environment check. - The failure message now points at the install docs, following the doc-link constant style already used in `utils_profile.py` and `profiler_base.py`. - Docs gain an `analyze-deps` section in `core-install.rst` covering both install paths: the `rocm-profiler[compute-analyze]` extra for pip users and the requirements file for packaged ROCm installations. The quickstart step links there instead of repeating it, matching how that page already defers to `core-install` for detail. - The extra is new in TheRock (ROCm/TheRock#7002), which is what makes the pip path documentable. ## Issue Tracking JIRA ID: AIPROFCOMP-788 ## Test Plan Ran `analyze --verify-deps` from the source tree in two environments: one with the requirements installed and one bare virtual environment. Added unit tests for the flag's default and for it parsing without `-p`, then ran the argparser suite and ruff. ## Test Result With the dependencies present the flag reports success and exits zero. In the bare environment it lists each missing package, prints the docs link, and exits one. The argparser unit tests pass and ruff is clean. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ute-analyze extra (#11928) ## Motivation Analyze mode needs Python packages that profile mode does not. There was no way to check whether an environment had them without also having a workload to analyze, because the existing check only ran on the way into an analysis. The failure message also pointed at the requirements file, which lists the pins but not how to install them. ### Validated using latest nightly ``` python3 -m venv ~/.venvs/rocprof-compute-analyze source ~/.venvs/rocprof-compute-analyze/bin/activate pip install --extra-index-url https://nightly.repo.amd.com/rocm/whl-next/ "rocm-profiler[compute-analyze]==10.2.0a20260922" ``` The rocprof-compute along with its python dependencies are installed in the venv - so no more manually pip installing the requirements.txt ## Technical Details - `rocprof-compute analyze --verify-deps` runs the existing `verify_deps()` check and exits: zero when every pin is installed at the required version, non-zero otherwise. `-p` stays optional, so the flag works as a standalone environment check. - The failure message now points at the install docs, following the doc-link constant style already used in `utils_profile.py` and `profiler_base.py`. - Docs gain an `analyze-deps` section in `core-install.rst` covering both install paths: the `rocm-profiler[compute-analyze]` extra for pip users and the requirements file for packaged ROCm installations. The quickstart step links there instead of repeating it, matching how that page already defers to `core-install` for detail. - The extra is new in TheRock (ROCm/TheRock#7002), which is what makes the pip path documentable. ## Issue Tracking JIRA ID: AIPROFCOMP-788 ## Test Plan Ran `analyze --verify-deps` from the source tree in two environments: one with the requirements installed and one bare virtual environment. Added unit tests for the flag's default and for it parsing without `-p`, then ran the argparser suite and ruff. ## Test Result With the dependencies present the flag reports success and exits zero. In the bare environment it lists each missing package, prints the docs link, and exits one. The argparser unit tests pass and ruff is clean. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ute-analyze extra (#11928) ## Motivation Analyze mode needs Python packages that profile mode does not. There was no way to check whether an environment had them without also having a workload to analyze, because the existing check only ran on the way into an analysis. The failure message also pointed at the requirements file, which lists the pins but not how to install them. ### Validated using latest nightly ``` python3 -m venv ~/.venvs/rocprof-compute-analyze source ~/.venvs/rocprof-compute-analyze/bin/activate pip install --extra-index-url https://nightly.repo.amd.com/rocm/whl-next/ "rocm-profiler[compute-analyze]==10.2.0a20260922" ``` The rocprof-compute along with its python dependencies are installed in the venv - so no more manually pip installing the requirements.txt ## Technical Details - `rocprof-compute analyze --verify-deps` runs the existing `verify_deps()` check and exits: zero when every pin is installed at the required version, non-zero otherwise. `-p` stays optional, so the flag works as a standalone environment check. - The failure message now points at the install docs, following the doc-link constant style already used in `utils_profile.py` and `profiler_base.py`. - Docs gain an `analyze-deps` section in `core-install.rst` covering both install paths: the `rocm-profiler[compute-analyze]` extra for pip users and the requirements file for packaged ROCm installations. The quickstart step links there instead of repeating it, matching how that page already defers to `core-install` for detail. - The extra is new in TheRock (ROCm/TheRock#7002), which is what makes the pip path documentable. ## Issue Tracking JIRA ID: AIPROFCOMP-788 ## Test Plan Ran `analyze --verify-deps` from the source tree in two environments: one with the requirements installed and one bare virtual environment. Added unit tests for the flag's default and for it parsing without `-p`, then ran the argparser suite and ruff. ## Test Result With the dependencies present the flag reports success and exits zero. In the bare environment it lists each missing package, prints the docs link, and exits one. The argparser unit tests pass and ruff is clean. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
## Motivation * Progress on #8272 * Follow-up to #7828 * Follow-up to #8275 * Follow-up to #7002 See https://docs.zizmor.sh/audits/#template-injection and https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/github_actions_style_guide.md#security---avoid-template-injection. ## Test Plan * `.github/workflows/test_code_coverage_component.yml` untested. * `.github/workflows/test_rocm_wheels.yml` will run on CI. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
Motivation
rocprof-compute analyzeneeds dash, pandas and numpy;rocprof-compute profileneeds nothing. Therocm-profilerwheel declares no dependency metadata, so analyze fails at import.The two modes want different environments:
pip install rocm rocm-profiler torchpip install --extra-index-url <rocm_index> rocm-profiler[compute-analyze]Under that split the deps belong in an extra, and exact pins remain appropriate because the analyze venv is isolated.
Related Issues
ISSUE ID: #7003
JIRA ID: AIPROFCOMP-788
Technical Details
libexec/rocprofiler-compute/requirements.txt, whichPROFILER_WHEEL_INCLUDESalready carries into the wheel. rocprofiler-compute stays the single source of truth, so pin changes need no change here.populate_runtime_files()stages that payload beforebuild_packages()invokessetup.py, so the file is on disk when read.setup.pyalready loads_dist_info.pyoff disk the same way.pip install rocm-profiler[compute-analyze]resolves against any build rather than warning about an unknown extra.--extra-index-url <rocm_index>, so the extra's deps come from PyPI androcm-profilercomes from the ROCm index.analyzegets ambiguous once anything else here needs Python packages. A second tool should get its own extra reading its own requirements file, not a merged list, which would force both tools' pins to relax.rocm[profiler]does not chain into the extra, by design. Analyze deps stay opt-in.Test Plan
Staged the profiler package with
PopulatedDistPackage, built the wheel through TheRock's ownbuild_packages(), then read METADATA and ranpip install --dry-runin a clean venv.Test Result
Every dep from the staged
requirements.txtis gated behind the extra, with comment lines dropped. A plain install pulls no analyze deps;[compute-analyze]pulls the full set. A payload staged withoutrequirements.txtbuilds cleanly with the extra declared and empty.Submission Checklist
🤖 Generated with Claude Code