Repository navigation
fix(rocprofiler-sdk): allow PYTHON_EXECUTABLES to be passed in from TheRock - #10198
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
|
There was a problem hiding this comment.
Pull request overview
This PR updates rocprofiler-sdk’s CMake Python discovery so TheRock can provide explicit Python interpreter paths (via ROCPROFILER_PYTHON_EXECUTABLES) and reliably build Python bindings for versions that older CMake FindPython3 can’t auto-discover (notably Python 3.14 on manylinux).
Changes:
- Adds support for pre-seeding
Python3_EXECUTABLEfrom a caller-provided list of candidate Python executables before invokingfind_package(Python3). - Uses
execute_process()to probe each candidate interpreter’s major/minor version to select the matching one for the requested Python version.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
0c2a054 to
bc91b38
Compare
srihari-uttanur
left a comment
srihari-uttanur
left a comment
There was a problem hiding this comment.
Major: TheRock variable is not forwarded, CI sets THEROCK_DIST_PYTHON_EXECUTABLES
THEROCK_SHARED_PYTHON_EXECUTABLES in rocm-systems/.github/scripts/therock_configure_ci.py but the sdk reads ROCPROFILER_PYTHON_EXECUTABLES. TheRock must translate and forward the value. Without that step, rocprofiler-sdk never receives the executable paths, so the patch has no effect in TheRock CI.
srihari-uttanur
left a comment
srihari-uttanur
left a comment
There was a problem hiding this comment.
2. Default version detection bypasses the new list (Medium)
source/lib/python/CMakeLists.txt L10-12:
if(NOT DEFINED ROCPROFILER_PYTHON_VERSIONS)
get_default_python_versions(DEFAULT_PYTHON_VERSIONS)get_default_python_versions() calls find_package(Python3 ...) directly and
never looks at ROCPROFILER_PYTHON_EXECUTABLES. So if CMake cannot auto-discover
3.14 (the exact motivating scenario), 3.14 never lands in the version list,
rocprofiler_find_python3(3.14) is never called, and the pre-seed never runs.
Passing the executables alone therefore does nothing. Either derive the
versions from the supplied executables, or document that
ROCPROFILER_PYTHON_VERSIONS is mandatory alongside it.
3. Exact MAJOR.MINOR.PATCH requests can select the wrong interpreter and hard-fail (Medium)
The candidate match compares only major/minor, but the macro still supports exact
3-component requests a few lines below.
Verified in FindPython/Support.cmake: when the public Python3_EXECUTABLE
is set to an absolute path, it is copied into the internal cache var with no
version validation, and the whole search is guarded by
if (NOT _Python3_EXECUTABLE) — so discovery is skipped completely. The version
is only checked at the end by find_package_handle_standard_args(... VERSION_VAR Python3_VERSION ...).
So for rocprofiler_find_python3(3.11.9) with a list containing 3.11.8 before
3.11.9: the loop breaks on 3.11.8, discovery is bypassed, and configure dies
under REQUIRED — it does not fall back or try the remaining candidates,
even though a valid 3.11.9 exists both in the list and on the system.
Suggested direction — capture the full version and compare at the requested
precision:
# print 3 components
"import sys; print('.'.join(map(str, sys.version_info[:3])))"
if("${_VERSION}" MATCHES "^[0-9]+\\.[0-9]+\\.[0-9]+$")
set(_want "${_VERSION}")
set(_got "${_rocprofiler_candidate_ver}")
else()
set(_want "${_rocprofiler_requested_major_minor}")
string(REGEX MATCH "^[0-9]+\\.[0-9]+" _got "${_rocprofiler_candidate_ver}")
endif()4. Non-absolute entries are silently ignored (Low)
FindPython only honours the pre-seed when IS_ABSOLUTE is true. A relative path
or bare name (python3.14) falls through to normal discovery with no warning.
Worth rejecting explicitly, or at least documenting that entries must be absolute.
We need to merge this PR (#10198) first, then also merge TheRock PR (ROCm/TheRock#7350), where we change it so that TheRock DOES inject the ROCPROFILER_PYTHON_EXECUTABLES to the Only then, can we safely add the python3.14 to |
…and adds that to the available versions
For (2), you can see in TheRock, they always do pass in ROCPROFILER_PYTHON_VERSIONS (https://github.com/ROCm/TheRock/blob/main/profiler/CMakeLists.txt#L124 or you can check the pending PR ROCm/TheRock#7350), but, to be more safe, I added additional code into this PR so that if ROCPROFILER_PYTHON_EXECUTABLES is provided, then it will auto parse and add it to PYTHON_VERSIONS.
For (3), the goal of ROCPROFILER_PYTHON_EXECUTABLES is mainly about helping CMake discover the right interpreter path, and typically systems have one Python per minor version. It's not critical to exact match/care about the patch version. We also can see, that in TheRock, when they detect their python versions, they only take major.minor (https://github.com/ROCm/TheRock/blob/main/cmake/therock_detect_python_versions.cmake#L90-L136). Similarly, in SDK, our ROCPROFILER_PYTHON_VERSION_CANDIDATES all list only major.minor too (https://github.com/ROCm/rocm-systems/blob/develop/projects/rocprofiler-sdk/source/lib/python/utilities.cmake#L60-L62). So, I think we can safely ignore this comment. |
|
@yhuiYH I am not too sure about this as we did similar fix in the past and need to recall why that was removed. |
TheRock Submodule Bump ActivityNewest first
|
…7350) ## Motivation In ROCm 7.14 whl and tarballs, rocpd and roctx need to support multiple python versions. Looks like it's been limited to 3.10-3.13. When users installed onto Ubuntu 26.04 where the default python version is 3.14, they get an error: ```rocpd --help Traceback (most recent call last): File "/opt/venv/lib/python3.14/site-packages/_rocm_sdk_devel/bin/rocpd", line 74, in <module> main() ~~~~^^ File "/opt/venv/lib/python3.14/site-packages/_rocm_sdk_devel/bin/rocpd", line 46, in main raise ImportError( ...<5 lines>... ) ImportError: rocpd not supported for Python version 3.14 (sys.executable='/opt/venv/bin/python3'). rocpd supported python versions: 3.10, 3.11, 3.12, 3.13 ``` ## Technical Details Digging into TheRock build, it looks like the python versions used are defined in `DIST_PYTHON_EXECUTABLES`. However, the manylinux image SHA TheRock is using is slightly old, and only has up to 3.13 in /opt/python. To add 3.14, we would need to: 1) Update the manylinux image SHA to something newer, and then update `DIST_PYTHON_EXECUTABLES` or 2) Just use the `/opt/python-shared/cp314-cp314/bin/python3` that rocgdb is using Opting for 2, to minimize changes. We may need an additional rocprofiler-sdk change to make the python 3.14 discoverable since CMake is old in manylinux image. ROCm/rocm-systems#10198 Update 10/6: we don't need to update the SHA after all. So this PR just includes 3.14 builds for roctx & rocpd. Addresses ISSUE ID #7398 ## Test Plan Check the build artifacts to see if python3.14 is now usable with rocpd. There was discussion to add rocpd tests to TheRock, but there are additional issues that need to be resolved, before those tests can be added. ## Test Result rocpd should be usable with python 3.14 ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
Motivation
When adding Python 3.14 support for roctx and rocpd in TheRock, we need to allow them to pass in a hint to the python executable.
Technical Details
CMake 3.27 (used in the manylinux build container) was released in July 2023 — before Python 3.14 existed. Its FindPython3 module doesn't know to probe /opt/python/cp314-cp314/ on manylinux, so it falls back to the system /bin/python3 (wrong version) and fails. For 3.10–3.13, CMake already had those paths baked into its discovery logic.
rocprofiler_find_python3 now checks ROCPROFILER_PYTHON_EXECUTABLES after the cache reset. If a matching executable is found for the requested version, it pre-seeds Python3_EXECUTABLE in the CMake cache before calling find_package. CMake then uses the explicitly provided path instead of trying to auto-discover — bypassing the CMake version limitation entirely.
Needed in conjunction with this TheRock PR ROCm/TheRock#7350 to fix Python 3.14 support for roctx and rocpd in TheRock builds.
In TheRock multi-arch CI build, seeing error log like this, when adding Python 3.14:
Issue Tracking
ISSUE ID ROCm/TheRock#7398
Test Plan
N/A, this is just an optional ENV var passed in
Test Result
N/A
Submission Checklist