Switch to cuDSS 0.8 - #11
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis pull request consolidates cuDSS integration by removing local-build support and adopting a prebuilt release (version 0.8.0.10). CMake now always fetches cuDSS via FetchContent with CUDA-version and architecture-specific selection. Build flags and preprocessor conditionals for legacy API support are removed, and all C++ code is updated to use only the new cuDSS API unconditionally. ChangescuDSS Prebuilt Release Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)cunls/common/cudss_helper.cppcunls/common/cudss_helper.cpp:18:10: fatal error: 'cunls/common/cudss_helper.h' file not found ... [truncated 1118 characters] ... stall/lib/clang/18/include" cunls/linear_solver/cudss_sparse_linear_solver.cppcunls/linear_solver/cudss_sparse_linear_solver.cpp:18:10: fatal error: 'cunls/linear_solver/cudss_sparse_linear_solver.h' file not found ... [truncated 1241 characters] ... e" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cunls/linear_solver/cudss_sparse_linear_solver.cpp (1)
32-51:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix cuDSS 0.8 reordering constant identifiers (likely compile break) + align docs
cuDSS 0.8’s public API uses
cudssAlgType_tvalues likeCUDSS_ALG_DEFAULTandCUDSS_ALG_1..5(e.g.,CUDSS_ALG_1is “block triangular reordering + COLAMD”); it does not defineCUDSS_REORDERING_ALG_DEFAULTorCUDSS_REORDERING_ALG_BTF_COLAMD, so these return values may not compile with cuDSS 0.8.Also update the
@brief/description to say “reordering algorithm” (not “algorithm type”) and remove “constant used for factorization” wording to match what the function returns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cunls/linear_solver/cudss_sparse_linear_solver.cpp` around lines 32 - 51, Update GetOrderingType to use cuDSS 0.8 public reordering constants (e.g., return CUDSS_ALG_DEFAULT for SlowInitFastSolve and CUDSS_ALG_1 for the former BTF+COLAMD case) instead of the non-existent CUDSS_REORDERING_ALG_* identifiers, and adjust the doc comment to read that it maps cuDSSLinearSolverMode to a "reordering algorithm" value (remove "algorithm type" and "constant used for factorization" phrasing); change references in the switch to CUDSS_ALG_DEFAULT and CUDSS_ALG_1 (or the appropriate CUDSS_ALG_n for the BTF+COLAMD variant) so it compiles against cuDSS 0.8 while keeping the function name GetOrderingType and the cuDSSLinearSolverMode enum intact.cunls/common/cudss_helper.cpp (2)
300-303:⚠️ Potential issue | 🟠 MajorReorder enum validation: current bounds check likely doesn’t match cuDSS 0.8 API
cuDSS 0.8’s reordering-algorithm configuration usescudssAlgType_tvalues likeCUDSS_ALG_DEFAULTandCUDSS_ALG_1/2/3; the cuDSS 0.8 docs shown don’t listCUDSS_REORDERING_ALG_DEFAULT/CUDSS_REORDERING_ALG_NONE(or aNONEvalue). This makes the “DEFAULT..NONE” enum range assumption likely incorrect and risks rejecting valid reordering algorithms—use explicit validation against the actual cuDSS 0.8 constants (or accept via switch/set-membership).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cunls/common/cudss_helper.cpp` around lines 300 - 303, The current bounds check on reordering_algorithm using CUDSS_REORDERING_ALG_DEFAULT and CUDSS_REORDERING_ALG_NONE is incorrect for cuDSS 0.8; replace the range check in cudss_helper.cpp with explicit validation against the cuDSS 0.8 enum values (e.g. compare to cudssAlgType_t constants like CUDSS_ALG_DEFAULT and the specific CUDSS_ALG_1/CUDSS_ALG_2/CUDSS_ALG_3 members) or implement a switch/set-membership check that accepts only those known constants before using reordering_algorithm, updating the thrown std::invalid_argument path accordingly.
319-324:⚠️ Potential issue | 🟠 Major | 💤 Low valueReplace hardcoded
16with a cuDSS-named value forCUDSS_CONFIG_HYBRID_EXECUTE_MODE.
cudssConfigSet(cfg, CUDSS_CONFIG_HYBRID_EXECUTE_MODE, ...)is being passed16, but cuDSS documentation describes this parameter as taking0(disabled, default) or1(enabled). Use the corresponding cuDSS constant/enum (or define a named local constant) and document why16is required if it’s intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cunls/common/cudss_helper.cpp` around lines 319 - 324, The code sets hybrid_execute_mode to the hardcoded value 16 before calling cudssConfigSet(cfg, CUDSS_CONFIG_HYBRID_EXECUTE_MODE, ...); replace the magic number with the documented cuDSS constant (or a local named constant) indicating enabled/disabled (e.g., use the cuDSS enum for HYBRID_EXECUTE = 1 or define const int HYBRID_EXECUTE_ENABLED = 1) and pass that instead of 16; update the local variable name hybrid_execute_mode if needed and add a brief comment referencing CUDSS_CONFIG_HYBRID_EXECUTE_MODE and why the enabled value is used (or, if 16 is truly required, replace it with a named constant and document the justification).
🧹 Nitpick comments (2)
cmake/AddCUDSS.cmake (2)
43-46: ⚡ Quick winConsider validating downloaded archive structure.
The code assumes the archive contains
lib/libcudss_static.aandinclude/directories without verification. If the archive structure differs, the build will fail with cryptic errors.Add validation after
FetchContent_Populate:✅ Suggested validation logic
FetchContent_Populate(cudss) + + if(NOT EXISTS "${cudss_SOURCE_DIR}/lib/libcudss_static.a") + message(FATAL_ERROR "cuDSS archive missing expected library: lib/libcudss_static.a") + endif() + if(NOT EXISTS "${cudss_SOURCE_DIR}/include") + message(FATAL_ERROR "cuDSS archive missing expected include directory") + endif() set_target_properties(cudss PROPERTIES🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/AddCUDSS.cmake` around lines 43 - 46, The build assumes cudss archive unpacks into ${cudss_SOURCE_DIR}/lib/libcudss_static.a and ${cudss_SOURCE_DIR}/include; add explicit validation right after FetchContent_Populate to check those paths exist (use EXISTS or IS_DIRECTORY checks against ${cudss_SOURCE_DIR}/lib/libcudss_static.a and ${cudss_SOURCE_DIR}/include) and emit a clear message(FATAL_ERROR) describing the missing file/dir if validation fails; update the set_target_properties(cudss ...) usage to run only after the validation passes so you fail fast with a helpful error instead of cryptic later build errors.
21-27: ⚡ Quick winCUDA version detection may not be future-proof.
The logic assumes only CUDA 12 and 13 exist. If NVIDIA releases CUDA 14+ in the future and the URL structure changes, this will silently use the
cuda13tag which may not be correct.Consider adding validation or a more explicit version mapping:
🔮 Suggested improvement for future CUDA versions
if(${CMAKE_CUDA_COMPILER_VERSION} VERSION_GREATER_EQUAL 13.0) set(CUDSS_CUDA_TAG "cuda13") message(STATUS "Using CUDA 13.0 or newer") + if(${CMAKE_CUDA_COMPILER_VERSION} VERSION_GREATER_EQUAL 14.0) + message(WARNING "CUDA 14.0+ detected. Using cuda13 tag - verify compatibility.") + endif() else() set(CUDSS_CUDA_TAG "cuda12") message(STATUS "Using CUDA 12.0 or older") endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/AddCUDSS.cmake` around lines 21 - 27, The current version check in AddCUDSS.cmake uses CMAKE_CUDA_COMPILER_VERSION to set CUDSS_CUDA_TAG only for "cuda12" or "cuda13", which will silently pick cuda13 for any future CUDA >=13; update the logic to parse the major version from CMAKE_CUDA_COMPILER_VERSION, map known majors explicitly (e.g., 12 -> "cuda12", 13 -> "cuda13"), and for unknown/newer majors either produce a clear warning and fail the configuration or choose a configurable fallback (e.g., "cuda13" with a logged warning) so that CUDSS_CUDA_TAG is never silently incorrect; reference the symbols CMAKE_CUDA_COMPILER_VERSION and CUDSS_CUDA_TAG when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/AddCUDSS.cmake`:
- Around line 36-41: Add checksum verification to the FetchContent download by
providing URL_HASH in FetchContent_Declare for cudss; implement a small lookup
that selects the correct SHA256 string based on the combination of
${CUDSS_CUDA_TAG} and ${CMAKE_SYSTEM_PROCESSOR} (e.g., a map or if/elseif table
that sets a variable like CUDSS_URL_HASH), and then pass that variable as
URL_HASH ${CUDSS_URL_HASH} into FetchContent_Declare (identify and update the
existing FetchContent_Declare(cudss ...) block and the CUDSS_URL usage); ensure
all archive variants you support have their SHA256 values populated in the
lookup before calling FetchContent_Populate(cudss).
- Around line 11-12: Change the hardcoded CUDSS_VERSION to a cache variable
(set(... CACHE STRING)) and set a safe default matching an actual release (e.g.,
"0.8.0" or "0.7.1"), add a configurable URL_HASH/CUDSS_URL_HASH and include it
in FetchContent_Declare (URL_HASH or let users override), and after
FetchContent_MakeAvailable validate that
${cudss_SOURCE_DIR}/lib/libcudss_static.a and ${cudss_SOURCE_DIR}/include exist
before calling the code that sets the imported target properties (references:
CUDSS_VERSION, CUDSS_URL, CUDSS_URL_HASH, FetchContent_Declare,
cudss_SOURCE_DIR, libcudss_static.a and the imported target configuration).
---
Outside diff comments:
In `@cunls/common/cudss_helper.cpp`:
- Around line 300-303: The current bounds check on reordering_algorithm using
CUDSS_REORDERING_ALG_DEFAULT and CUDSS_REORDERING_ALG_NONE is incorrect for
cuDSS 0.8; replace the range check in cudss_helper.cpp with explicit validation
against the cuDSS 0.8 enum values (e.g. compare to cudssAlgType_t constants like
CUDSS_ALG_DEFAULT and the specific CUDSS_ALG_1/CUDSS_ALG_2/CUDSS_ALG_3 members)
or implement a switch/set-membership check that accepts only those known
constants before using reordering_algorithm, updating the thrown
std::invalid_argument path accordingly.
- Around line 319-324: The code sets hybrid_execute_mode to the hardcoded value
16 before calling cudssConfigSet(cfg, CUDSS_CONFIG_HYBRID_EXECUTE_MODE, ...);
replace the magic number with the documented cuDSS constant (or a local named
constant) indicating enabled/disabled (e.g., use the cuDSS enum for
HYBRID_EXECUTE = 1 or define const int HYBRID_EXECUTE_ENABLED = 1) and pass that
instead of 16; update the local variable name hybrid_execute_mode if needed and
add a brief comment referencing CUDSS_CONFIG_HYBRID_EXECUTE_MODE and why the
enabled value is used (or, if 16 is truly required, replace it with a named
constant and document the justification).
In `@cunls/linear_solver/cudss_sparse_linear_solver.cpp`:
- Around line 32-51: Update GetOrderingType to use cuDSS 0.8 public reordering
constants (e.g., return CUDSS_ALG_DEFAULT for SlowInitFastSolve and CUDSS_ALG_1
for the former BTF+COLAMD case) instead of the non-existent
CUDSS_REORDERING_ALG_* identifiers, and adjust the doc comment to read that it
maps cuDSSLinearSolverMode to a "reordering algorithm" value (remove "algorithm
type" and "constant used for factorization" phrasing); change references in the
switch to CUDSS_ALG_DEFAULT and CUDSS_ALG_1 (or the appropriate CUDSS_ALG_n for
the BTF+COLAMD variant) so it compiles against cuDSS 0.8 while keeping the
function name GetOrderingType and the cuDSSLinearSolverMode enum intact.
---
Nitpick comments:
In `@cmake/AddCUDSS.cmake`:
- Around line 43-46: The build assumes cudss archive unpacks into
${cudss_SOURCE_DIR}/lib/libcudss_static.a and ${cudss_SOURCE_DIR}/include; add
explicit validation right after FetchContent_Populate to check those paths exist
(use EXISTS or IS_DIRECTORY checks against
${cudss_SOURCE_DIR}/lib/libcudss_static.a and ${cudss_SOURCE_DIR}/include) and
emit a clear message(FATAL_ERROR) describing the missing file/dir if validation
fails; update the set_target_properties(cudss ...) usage to run only after the
validation passes so you fail fast with a helpful error instead of cryptic later
build errors.
- Around line 21-27: The current version check in AddCUDSS.cmake uses
CMAKE_CUDA_COMPILER_VERSION to set CUDSS_CUDA_TAG only for "cuda12" or "cuda13",
which will silently pick cuda13 for any future CUDA >=13; update the logic to
parse the major version from CMAKE_CUDA_COMPILER_VERSION, map known majors
explicitly (e.g., 12 -> "cuda12", 13 -> "cuda13"), and for unknown/newer majors
either produce a clear warning and fail the configuration or choose a
configurable fallback (e.g., "cuda13" with a logged warning) so that
CUDSS_CUDA_TAG is never silently incorrect; reference the symbols
CMAKE_CUDA_COMPILER_VERSION and CUDSS_CUDA_TAG when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 0d0a9da3-135a-47e5-a195-c7a102b9ffd8
📒 Files selected for processing (6)
CMakeLists.txtcmake/AddCUDSS.cmakecunls/common/CMakeLists.txtcunls/common/cudss_helper.cppcunls/linear_solver/CMakeLists.txtcunls/linear_solver/cudss_sparse_linear_solver.cpp
💤 Files with no reviewable changes (2)
- cunls/linear_solver/CMakeLists.txt
- cunls/common/CMakeLists.txt
|
Is this keeping the ability for users to decide on version of cuDSS? Let's keep the option of using older versions of cuDSS, some might have restrictions on versions based on local environments. |
2 participants
Summary by CodeRabbit