cmake(pt): robustly detect PyTorch CXX11 ABI via Python; unify comments#4879
cmake(pt): robustly detect PyTorch CXX11 ABI via Python; unify comments#4879OutisLi wants to merge 5 commits intodeepmodeling:develfrom
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughReplaces flag-parsing ABI detection with Python-driven detection (torch.compiled_with_cxx11_abi), adds Python-based PyTorch prefix and library path discovery, extends ABI mismatch handling and compatibility flags, and ensures PyTorch include/library paths are appended to backend paths. Changes are contained to CMake integration logic. Changes
Sequence Diagram(s)sequenceDiagram
participant CMake as CMakeLists.txt
participant Python as python
participant Torch as torch (python package)
CMake->>Python: run script to import torch (if BUILD_CPP_IF/USE_PT_PYTHON_LIBS and conditions)
alt python+torch available
Python->>Torch: call compiled_with_cxx11_abi()
Torch-->>Python: return ABI (true/false)
Python-->>CMake: return OP_CXX_ABI_PT and prefix (site-packages / cmake path)
CMake->>CMake: set OP_CXX_ABI_PT, append CMAKE_PREFIX_PATH, derive library path, update BACKEND_LIBRARY_PATH
else fallback
CMake->>CMake: fallback parse -D_GLIBCXX_USE_CXX11_ABI from CMAKE_CXX_FLAGS
end
CMake->>CMake: compare OP_CXX_ABI (if set) with OP_CXX_ABI_PT
alt mismatch & not BUILD_PY_IF
CMake-->>User: FATAL_ERROR (abort)
else mismatch & BUILD_PY_IF
alt BUILD_CPP_IF false
CMake-->>User: STATUS about mismatch (non-fatal)
else BUILD_CPP_IF true
CMake-->>User: WARNING about mismatch
CMake->>CMake: enable C++ OP build, disable PT C++ libs, set DEEPMD_BUILD_COMPAT_CXXABI ON, set OP_CXX_ABI_COMPAT=OP_CXX_ABI_PT
end
else match
CMake->>CMake: set DEEPMD_BUILD_COMPAT_CXXABI OFF
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
source/CMakeLists.txt (1)
402-411: Refactor to use the importedTorch::Torchtarget and remove deprecatedLOCATION
- File:
source/CMakeLists.txt(lines 402–411)- # get torch library directory from target "torch" - get_target_property(_TORCH_LOCATION torch LOCATION) - get_filename_component(PyTorch_LIBRARY_PATH ${_TORCH_LOCATION} DIRECTORY) + # get torch library directory from imported target Torch::Torch + get_target_property(_TORCH_LOCATION Torch::Torch IMPORTED_LOCATION) + if(NOT _TORCH_LOCATION) + # handle multi-config generators + get_target_property(_TORCH_LOCATION Torch::Torch IMPORTED_LOCATION_${CMAKE_BUILD_TYPE}) + endif() + if(_TORCH_LOCATION) + get_filename_component(PyTorch_LIBRARY_PATH "${_TORCH_LOCATION}" DIRECTORY) + else() + message(WARNING "Unable to resolve Torch::Torch IMPORTED_LOCATION; BACKEND_LIBRARY_PATH may be incomplete.") + endif()
- Replace the global
link_directories()call with a guardedtarget_link_directories()(requires CMake 3.13+), or simply rely on the imported target’s link information. For the wheel layout:if(USE_PT_PYTHON_LIBS OR BUILD_PY_IF) if(EXISTS "${PyTorch_LIBRARY_PATH}/../../torch.libs") target_link_directories(<backend_target> PRIVATE "${PyTorch_LIBRARY_PATH}/../../torch.libs" ) endif() endif()
🧹 Nitpick comments (6)
source/CMakeLists.txt (6)
323-328: Fix variable expansion typo in fatal error message"${PYTORCH_CMAKE_PREFIX_PATH_RESULT_VAR}" is missing the opening brace in the message and will print literally, obscuring the real exit code.
- "Cannot determine PyTorch CMake prefix path, error code: $PYTORCH_CMAKE_PREFIX_PATH_RESULT_VAR}, error message: ${PYTORCH_CMAKE_PREFIX_PATH_ERROR_VAR}" + "Cannot determine PyTorch CMake prefix path, error code: ${PYTORCH_CMAKE_PREFIX_PATH_RESULT_VAR}, error message: ${PYTORCH_CMAKE_PREFIX_PATH_ERROR_VAR}"
307-315: Python discovery redundancy and scopeYou’re calling
find_package(Python COMPONENTS Interpreter REQUIRED)in two nearby blocks. Not harmful, but redundant. You can rely on the first call or wrap subsequent usage withif(NOT Python_Interpreter_FOUND). Minor cleanup only.
340-347: Gate ABI detection to relevant platforms to reduce noise
_GLIBCXX_USE_CXX11_ABImatters on libstdc++ (primarily Linux). On Windows/macOS this is usually irrelevant. Consider gating detection and definition underif(UNIX AND NOT APPLE)to avoid confusing logs and unnecessary defines on non-Linux platforms.
307-331: Add ENV error capture for cmake_prefix_path probe and avoid hard failure in mixed setupsFor the cmake_prefix_path probe, consider capturing
ERROR_VARIABLE(you already do) and downgrading toSTATUS/WARNINGwhenUSE_PT_PYTHON_LIBSis off, so libtorch-only builds don’t fail here. This aligns with the new ABI fallback approach.
425-428: Defaulting OP_CXX_ABI to 1 is fine; ensure the macro is defined consistentlyYou set the default value here; ensure
-D_GLIBCXX_USE_CXX11_ABI=${OP_CXX_ABI}is applied once globally after this point if not already added in earlier branches.
1-3: Silence CMP0144 policy warning (dev warning in CI)To address the policy warning seen in CI, set the policy explicitly near the top:
# after cmake_minimum_required(...) if(POLICY CMP0144) cmake_policy(SET CMP0144 NEW) endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/CMakeLists.txt(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test Python
source/CMakeLists.txt
[error] 361-361: CMake configuration failed: Failed to detect PyTorch CXX11 ABI. Please ensure 'torch' is installed and accessible in the current Python environment ('/home/runner/.cache/uv/builds-v0/.tmpWuw0dT/bin/python').
[warning] 302-302: CMake Warning (dev): CMP0144 is not set: find_package uses upper-case _ROOT variables. Run 'cmake --help-policy CMP0144' for policy details. Use the cmake_policy command to set the policy and suppress.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C library (2.14, >=2.5.0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Test C++ (true)
- GitHub Check: Test C++ (false)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
source/CMakeLists.txt (1)
332-339: C++ standard bumps look fineBumping C++ standard based on Torch version remains intact. No issues.
| execute_process( | ||
| COMMAND env PYTHONPATH=${Python_SITEARCH} ${Python_EXECUTABLE} -c | ||
| "import torch; print(int(torch.compiled_with_cxx11_abi()))" | ||
| OUTPUT_VARIABLE DETECTED_PT_ABI | ||
| RESULT_VARIABLE pt_abi_result | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make Python call portable and resilient (no 'env', avoid Python_SITEARCH dependency)
- Using
envinexecute_process(COMMAND ...)breaks on Windows. UseENVIRONMENTinstead. Python_SITEARCHis not guaranteed when only the Interpreter component is found. It can be empty and isn’t needed to import torch from the active interpreter.
- execute_process(
- COMMAND env PYTHONPATH=${Python_SITEARCH} ${Python_EXECUTABLE} -c
- "import torch; print(int(torch.compiled_with_cxx11_abi()))"
- OUTPUT_VARIABLE DETECTED_PT_ABI
- RESULT_VARIABLE pt_abi_result
- OUTPUT_STRIP_TRAILING_WHITESPACE)
+ execute_process(
+ COMMAND ${Python_EXECUTABLE} -c "import torch; print(int(torch.compiled_with_cxx11_abi()))"
+ OUTPUT_VARIABLE DETECTED_PT_ABI
+ RESULT_VARIABLE pt_abi_result
+ ERROR_VARIABLE pt_abi_err
+ OUTPUT_STRIP_TRAILING_WHITESPACE
+ # If you still need to influence sys.path, do it via ENVIRONMENT (portable):
+ # ENVIRONMENT "PYTHONPATH=${Python_SITEARCH}"
+ )Optional: If you want to prefer the active interpreter’s environment, drop PYTHONPATH entirely (as above). If you keep it, gate it with if(DEFINED Python_SITEARCH).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| execute_process( | |
| COMMAND env PYTHONPATH=${Python_SITEARCH} ${Python_EXECUTABLE} -c | |
| "import torch; print(int(torch.compiled_with_cxx11_abi()))" | |
| OUTPUT_VARIABLE DETECTED_PT_ABI | |
| RESULT_VARIABLE pt_abi_result | |
| OUTPUT_STRIP_TRAILING_WHITESPACE) | |
| execute_process( | |
| COMMAND ${Python_EXECUTABLE} -c "import torch; print(int(torch.compiled_with_cxx11_abi()))" | |
| OUTPUT_VARIABLE DETECTED_PT_ABI | |
| RESULT_VARIABLE pt_abi_result | |
| ERROR_VARIABLE pt_abi_err | |
| OUTPUT_STRIP_TRAILING_WHITESPACE | |
| # If you still need to influence sys.path, do it via ENVIRONMENT (portable): | |
| # ENVIRONMENT "PYTHONPATH=${Python_SITEARCH}" | |
| ) |
🤖 Prompt for AI Agents
In source/CMakeLists.txt around lines 348 to 354, the execute_process invocation
uses a platform-unsafe "env" wrapper and unconditionally sets PYTHONPATH to
Python_SITEARCH which may be empty; change it to call the interpreter directly
via ${Python_EXECUTABLE} and supply environment variables via CMake's
ENVIRONMENT keyword (not the "env" command) or simply drop PYTHONPATH entirely
to prefer the active interpreter’s environment; if you must set PYTHONPATH,
guard it with if(DEFINED Python_SITEARCH) and only add ENVIRONMENT
"PYTHONPATH=${Python_SITEARCH}" when defined so the command is portable on
Windows and resilient when Python_SITEARCH is missing.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
source/CMakeLists.txt (1)
307-330: Fix error-message variable expansion; keep the prefix-path discovery but fail with a clear, correct message
- The error message at Line 326 uses shell-style
$VAR instead of CMake’s $ {VAR} and has a stray closing brace. It will print literally or misformat.- The rest of this block looks good; keeping a hard error here is reasonable since USE_PT_PYTHON_LIBS explicitly opts into Python-wheel linkage.
Apply this diff:
- if(NOT ${PYTORCH_CMAKE_PREFIX_PATH_RESULT_VAR} EQUAL 0) - message( - FATAL_ERROR - "Cannot determine PyTorch CMake prefix path, error code: $PYTORCH_CMAKE_PREFIX_PATH_RESULT_VAR}, error message: ${PYTORCH_CMAKE_PREFIX_PATH_ERROR_VAR}" - ) - endif() + if(NOT ${PYTORCH_CMAKE_PREFIX_PATH_RESULT_VAR} EQUAL 0) + message( + FATAL_ERROR + "Cannot determine PyTorch CMake prefix path. " + "error code: ${PYTORCH_CMAKE_PREFIX_PATH_RESULT_VAR}, " + "error message: ${PYTORCH_CMAKE_PREFIX_PATH_ERROR_VAR}" + ) + endif()
♻️ Duplicate comments (3)
source/CMakeLists.txt (3)
377-416: Always emit the final -D_GLIBCXX_USE_CXX11_ABI after ABI resolution (handles mismatch path too)Currently, add_definitions is emitted in “match” and “no prior ABI” branches, but not when there is a mismatch and BUILD_PY_IF is ON (compat build). Emit the macro once after the decision so all targets consistently see OP_CXX_ABI.
Apply this minimal change:
else() # ABI matches; no compat build needed set(DEEPMD_BUILD_COMPAT_CXXABI OFF) - add_definitions(-D_GLIBCXX_USE_CXX11_ABI=${OP_CXX_ABI}) endif() else() # no prior ABI; default to PyTorch ABI set(OP_CXX_ABI ${OP_CXX_ABI_PT}) - add_definitions(-D_GLIBCXX_USE_CXX11_ABI=${OP_CXX_ABI}) endif() + # Ensure the macro is defined for our code based on final OP_CXX_ABI + if(DEFINED OP_CXX_ABI) + add_definitions(-D_GLIBCXX_USE_CXX11_ABI=${OP_CXX_ABI}) + endif()
348-354: Make the Python call portable; drop ‘env’, optionally guard PYTHONPATH via ENVIRONMENTUsing the external “env” program breaks on Windows and Python_SITEARCH may be unset when only Interpreter is found. Prefer direct call + CMake’s ENVIRONMENT.
Apply this diff:
- execute_process( - COMMAND env PYTHONPATH=${Python_SITEARCH} ${Python_EXECUTABLE} -c - "import torch; print(int(torch.compiled_with_cxx11_abi()))" - OUTPUT_VARIABLE DETECTED_PT_ABI - RESULT_VARIABLE pt_abi_result - OUTPUT_STRIP_TRAILING_WHITESPACE) + execute_process( + COMMAND ${Python_EXECUTABLE} -c "import torch; print(int(torch.compiled_with_cxx11_abi()))" + OUTPUT_VARIABLE DETECTED_PT_ABI + RESULT_VARIABLE pt_abi_result + ERROR_VARIABLE pt_abi_err + OUTPUT_STRIP_TRAILING_WHITESPACE + # If you still need to influence sys.path, guard and use ENVIRONMENT (portable): + # ENVIRONMENT "PYTHONPATH=${Python_SITEARCH}" + )
355-375: Improve fallback: prefer TORCH_CXX_FLAGS when available; include error diagnosticsGreat that you no longer hard-fail. Prefer parsing TORCH_CXX_FLAGS first (when set by Torch package), then fall back to CMAKE_CXX_FLAGS, and include the Python error output to aid debugging.
Apply this diff:
- else() - # Python torch not available -> warn and default - string(REGEX MATCH "-D_GLIBCXX_USE_CXX11_ABI=([01])" _m - "${CMAKE_CXX_FLAGS}") - if(CMAKE_MATCH_1) - set(OP_CXX_ABI_PT "${CMAKE_MATCH_1}") - message( - STATUS "Parsed PyTorch CXX11 ABI from CMAKE_CXX_FLAGS: ${OP_CXX_ABI_PT}" - ) - else() - message( - WARNING - "Could not detect PyTorch CXX11 ABI (torch import failed with code=${pt_abi_result}). " - "Defaulting OP_CXX_ABI_PT=1.") - set(OP_CXX_ABI_PT 1) - endif() - endif() + else() + # Python torch not available -> fall back to flags or default + if(DEFINED TORCH_CXX_FLAGS) + string(REGEX MATCH "-D_GLIBCXX_USE_CXX11_ABI=([01])" _pt "${TORCH_CXX_FLAGS}") + if(CMAKE_MATCH_1) + set(OP_CXX_ABI_PT "${CMAKE_MATCH_1}") + message(STATUS "Detected PyTorch CXX11 ABI from TORCH_CXX_FLAGS: ${OP_CXX_ABI_PT}") + endif() + endif() + if(NOT DEFINED OP_CXX_ABI_PT) + string(REGEX MATCH "-D_GLIBCXX_USE_CXX11_ABI=([01])" _cc "${CMAKE_CXX_FLAGS}") + if(CMAKE_MATCH_1) + set(OP_CXX_ABI_PT "${CMAKE_MATCH_1}") + message(STATUS "Parsed PyTorch CXX11 ABI from CMAKE_CXX_FLAGS: ${OP_CXX_ABI_PT}") + endif() + endif() + if(NOT DEFINED OP_CXX_ABI_PT) + message(WARNING + "Could not detect PyTorch CXX11 ABI (torch import failed with code=${pt_abi_result}" + "${pt_abi_err MATCHES \".+\" ? \"; see pt_abi_err\" : \"\"}). Defaulting OP_CXX_ABI_PT=1.") + set(OP_CXX_ABI_PT 1) + endif() + endif()
🧹 Nitpick comments (1)
source/CMakeLists.txt (1)
413-420: Modernize target/file path retrieval and guard wheel link dir
- LOCATION is legacy; prefer generator expression or IMPORTED_LOCATION for imported targets.
- Guard the torch.libs link directory to avoid noisy warnings when it doesn’t exist.
Apply this diff:
- get_target_property(_TORCH_LOCATION torch LOCATION) - get_filename_component(PyTorch_LIBRARY_PATH ${_TORCH_LOCATION} DIRECTORY) + # Prefer generator expression for target file path (works for imported targets) + get_filename_component(PyTorch_LIBRARY_PATH "$<TARGET_FILE:torch>" DIRECTORY) list(APPEND BACKEND_LIBRARY_PATH ${PyTorch_LIBRARY_PATH}) list(APPEND BACKEND_INCLUDE_DIRS ${TORCH_INCLUDE_DIRS}) if(USE_PT_PYTHON_LIBS OR BUILD_PY_IF) # when libtorch.so is in a wheel - link_directories(${PyTorch_LIBRARY_PATH}/../../torch.libs) + if(EXISTS "${PyTorch_LIBRARY_PATH}/../../torch.libs") + link_directories(${PyTorch_LIBRARY_PATH}/../../torch.libs) + endif() endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/CMakeLists.txt(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test C++ (true)
- GitHub Check: Test C++ (false)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C library (2.14, >=2.5.0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (2)
source/CMakeLists.txt (2)
332-338: C++ standard bump logic for Torch versions looks goodConditionally raising CMAKE_CXX_STANDARD based on Torch version is correct and minimal.
340-347: Python discovery and status logging are fineReusing find_package(Python ... Interpreter) here is OK; message aids diagnostics.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #4879 +/- ##
========================================
Coverage ? 84.34%
========================================
Files ? 702
Lines ? 68583
Branches ? 3573
========================================
Hits ? 57848
Misses ? 9595
Partials ? 1140 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
cmake(pt): robustly detect PyTorch CXX11 ABI via Python; unify comments
Summary
TORCH_CXX_FLAGSwith a robust, runtime-accurate ABI detection using Python:torch.compiled_with_cxx11_abi().Motivation
TORCH_CXX_FLAGScan be unreliable for wheel-based distributions and certain environments where the flag may be absent or misleading.torch.compiled_with_cxx11_abi()from Python reflects the actual build/runtime setting of the installed PyTorch, reducing false negatives/positives.Changes
env PYTHONPATH=${Python_SITEARCH} ${Python_EXECUTABLE} -c "import torch; print(int(torch.compiled_with_cxx11_abi()))"OP_CXX_ABI_PTand log a clear status message.OP_CXX_ABIwas previously defined (e.g., via TensorFlow), keep the existing mismatch handling:OP_CXX_ABI_COMPAT.OP_CXX_ABIwas not set, default to the PyTorch-detected value and add-D_GLIBCXX_USE_CXX11_ABI=${OP_CXX_ABI}.PyTorch_LIBRARY_PATH, includes, and optionaltorch.libslink directory for wheel installs.Compatibility
Testing
-DENABLE_PYTORCH=ONand build; expect a log line like:-- Detecting PyTorch CXX11 ABI via Python API...-- Detected PyTorch CXX11 ABI: 0|1OP_CXX_ABIis pre-set (mismatch branches).link_directories(${PyTorch_LIBRARY_PATH}/../../torch.libs)remains effective.Notes for Reviewers
torchis not importable in the active Python, the error message is explicit and actionable.Changelog
source/CMakeLists.txt.Summary by CodeRabbit