[ROCM-CORE] Update rocm-core rdhc script to support rocm install prefix#3581
[ROCM-CORE] Update rocm-core rdhc script to support rocm install prefix#3581arvindcheru wants to merge 4 commits into
Conversation
f83ac1c to
9b83218
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds support for specifying a custom ROCm installation prefix via a new --install-prefix command-line argument to the ROCm Deployment Health Check (RDHC) tool. This allows users to test ROCm installations in non-standard locations without modifying environment variables.
Changes:
- Added
--install-prefixcommand-line argument that accepts a custom ROCm installation directory path - Updated
ROCMHealthCheckclass constructor to accept and use a configurablerocm_pathparameter throughout all health checks - Updated documentation with usage examples for the new option
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| projects/rocm-core/rdhc/rdhc.py | Added --install-prefix argument, updated ROCMHealthCheck class to accept and use configurable rocm_path, and replaced all hardcoded ROCM_PATH references with self.rocm_path |
| projects/rocm-core/rdhc/README.md | Added documentation for the new --install-prefix option with usage examples |
Comments suppressed due to low confidence (4)
projects/rocm-core/rdhc/rdhc.py:2018
- The logic for handling --install-prefix is inconsistent. When args.install_prefix is an empty string (not None), rocm_path will be set to an empty string on line 2012, but the code relies on the ROCMHealthCheck constructor to fall back to the default. This creates unnecessary coupling and makes the code harder to understand.
Consider falling back to the environment variable or default path in main() when the stripped install_prefix is empty, similar to the else branch (lines 2015-2018). This would make the behavior more explicit and consistent.
# If --install-prefix was passed and is non-empty; else keep current logic
if args.install_prefix is not None:
rocm_path = (args.install_prefix or "").strip()
if rocm_path:
logger.debug(f"Using ROCm install prefix: {rocm_path}")
else:
# Incase no install prefix provided
# Support Legacy logic to use ROCM_PATH or default /opt/rocm
rocm_path = os.environ.get("ROCM_PATH", "/opt/rocm")
projects/rocm-core/rdhc/rdhc.py:2016
- Spelling error: 'Incase' should be 'In case' (two words).
# Incase no install prefix provided
projects/rocm-core/rdhc/rdhc.py:288
- The docstring references a hardcoded path /opt/rocm/.info/version, but this method now uses the configurable self.rocm_path. Update the docstring to reflect this, for example: "Get the ROCm version string from {rocm_path}/.info/version"
"""Get the ROCm version string from /opt/rocm/.info/version"""
projects/rocm-core/rdhc/rdhc.py:2010
- The comment is slightly misleading. The if-condition checks whether args.install_prefix is not None, not whether it's non-empty. The non-empty check happens inside on line 2013. Consider updating the comment to be more accurate.
# If --install-prefix was passed and is non-empty; else keep current logic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ebc7fa5 to
a063aee
Compare
…_feb25" This reverts commit 406f86b2bdaaee92b923146174b8e39aab42076f, reversing changes made to a063aeefe38d78530cc811fed4cdb0cdedad70ac.
This reverts commit a063aeefe38d78530cc811fed4cdb0cdedad70ac.
06c3ee0 to
7b8d65c
Compare
|
Due to issues with reversion conflicts, replaced this PR with #3596 |
Motivation
This pull request introduces support for specifying a custom ROCm installation prefix via the new
--install-prefixcommand-line argument inrdhc.py, The codebase has been updated to consistently use this prefix throughout all health check operations. Documentation and usage examples have also been updated to reflect this new option.additional change - Add path LD_LIBRARY_PATH
Technical Details
New install prefix support:
--install-prefixargument tordhc.py, allowing users to specify the ROCm installation directory, which overrides theROCM_PATHenvironment variable or defaults to/opt/rocm. [1] [2]ROCMHealthCheckclass and all relevant methods to use therocm_pathprovided via the constructor, ensuring all checks reference the correct installation path.JIRA ID
Test Plan
Test Result
Submission Checklist