Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new compatibility test suite for Tensor::record_stream and updates build configuration to include CUDA Toolkit headers when available, improving compilation in CUDA-enabled environments (especially when headers pull in cuda_runtime_api.h).
Changes:
- Added
test/RecordStreamTest.cppcovering multiplerecord_streamcall scenarios (various dtypes/shapes, non-contiguous, CPU tensor cases). - Updated
CMakeLists.txttofind_package(CUDAToolkit)and append CUDA include directories to Torch/Paddle include paths when found.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/RecordStreamTest.cpp | Introduces record_stream API coverage and a helper to obtain a CUDA stream across Torch vs Paddle compat builds. |
| CMakeLists.txt | Adds optional CUDA Toolkit discovery and include paths to support CUDA-dependent headers during compilation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "${TORCH_DIR}/include/torch/csrc/api/include/") | ||
| if(CUDAToolkit_FOUND) | ||
| list(APPEND TORCH_INCLUDE_DIR "${CUDAToolkit_INCLUDE_DIRS}") | ||
| endif() |
There was a problem hiding this comment.
find_package(CUDAToolkit QUIET) + conditional include path append can still leave builds failing later with missing cuda_runtime_api.h (e.g., CUDA-enabled libtorch headers included but CUDA toolkit not found), and the failure will be a compiler error with no actionable CMake message. Consider adding a clear message(FATAL_ERROR ...) (or at least message(WARNING ...)) when CUDA-dependent headers/tests are being built but CUDAToolkit is not found, so the configuration fails early and is easier to debug.
| endif() | |
| endif() | |
| if(NOT CUDAToolkit_FOUND) | |
| message( | |
| FATAL_ERROR | |
| "CUDAToolkit was not found, but CUDA-dependent Torch tests are enabled. " | |
| "Please install the CUDA toolkit or disable building these Torch tests.") | |
| endif() |
| if(CUDAToolkit_FOUND) | ||
| list(APPEND PADDLE_INCLUDE_DIR "${CUDAToolkit_INCLUDE_DIRS}") | ||
| endif() |
There was a problem hiding this comment.
Appending ${CUDAToolkit_INCLUDE_DIRS} to PADDLE_INCLUDE_DIR is guarded on CUDAToolkit_FOUND, but PADDLE_LIBRARIES unconditionally includes GPU libraries (e.g. libphi_gpu.so). If the environment is missing CUDA toolkit headers, the build will fail with confusing compile errors. Consider either (1) making CUDA toolkit a required dependency when linking Paddle GPU libs, or (2) gating the GPU libs and CUDA include dirs behind a single explicit option/check so CPU-only environments fail fast or build a CPU-only variant consistently.
youge325
force-pushed
the
cRecord
branch
2 times, most recently
from
724bdcf to
887eff7
Compare
- cuda_stream.md: mark query/synchronize as ✅ (implemented via c10::Stream); update getDefaultCUDAStream to ✅ (returns null stream, id=0); clarify getCurrentCUDAStream/setCurrentCUDAStream TLS semantics; update stats - stream.md: mark native_handle/query/synchronize as fully implemented in Stream.cpp (not just declared); update reference section accordingly - compat_arch.md (new): Mermaid diagram showing c10::Stream → c10::cuda::CUDAStream → phi layer → cudaStream_t mapping; documents TLS state, stream pool, StreamId encoding, and Paddle-specific adaptations with rationale for each deviation from PyTorch
新增 record_stream 测试