Replaced cusparse wrappers with simple unique_ptrs and more RAII - #1342
Replaced cusparse wrappers with simple unique_ptrs and more RAII #1342Bubullzz wants to merge 18 commits into
Conversation
Bubullzz
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughChangescuSPARSE descriptor migration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 (1)
cpp/src/pdlp/cusparse_view.cu (1)
240-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
has_value()check before dereferencing optional.
funcat line 242 is dereferenced without checking if the dlsym lookup succeeded.Proposed fix
void cusparse_spmvop_run(cusparseHandle_t handle, cusparseSpMVOpPlan_t plan, const void* alpha, const void* beta, cusparse_dn_vec_descr_view vecX, cusparse_dn_vec_descr_view vecY, cusparse_dn_vec_descr_view vecZ, cudaStream_t stream) { static const auto func = dynamic_load_runtime::function<cusparseSpMVOp_sig>("cusparseSpMVOp"); + cuopt_expects(func.has_value(), "cusparseSpMVOp symbol not found at runtime"); RAFT_CUSPARSE_TRY(cusparseSetStream(handle, stream)); RAFT_CUSPARSE_TRY((*func)(handle, plan, alpha, beta, vecX, vecY, vecZ)); }🤖 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 `@cpp/src/pdlp/cusparse_view.cu` around lines 240 - 242, The code dereferences the optional dynamic_load_runtime::function<cusparseSpMVOp_sig> named func without checking it; before calling (*func)(handle, plan, alpha, beta, vecX, vecY, vecZ) add a check like if (!func.has_value()) and handle the failure (log or return an error/throw) with a clear message including the symbol name "cusparseSpMVOp"; keep the existing RAFT_CUSPARSE_TRY usage for actual cuSPARSE calls and ensure the early error path prevents the dereference of func and returns/propagates an appropriate error.
🧹 Nitpick comments (2)
cpp/src/pdlp/cusparse_view.hpp (1)
38-57: 💤 Low valueConsider
_tsuffix for deleter types per project naming conventions.The coding guidelines specify types/structs should use
snake_case_twith_tsuffix (e.g.,cusparse_sp_mat_deleter_t). However, the current naming follows common STL deleter patterns, so this is a stylistic choice.🤖 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 `@cpp/src/pdlp/cusparse_view.hpp` around lines 38 - 57, The structs cusparse_sp_mat_deleter, cusparse_dn_vec_deleter, and cusparse_dn_mat_deleter do not follow the project naming convention requiring a snake_case_t suffix; rename them to cusparse_sp_mat_deleter_t, cusparse_dn_vec_deleter_t, and cusparse_dn_mat_deleter_t respectively, update all uses/typedefs/usings in this compilation unit and any headers that reference these types (e.g., unique_ptr deleter specializations or variable declarations), and ensure the operator() implementations remain unchanged and still call RAFT_CUSPARSE_TRY_NO_THROW on cusparseDestroySpMat/cusparseDestroyDnVec/cusparseDestroyDnMat.cpp/src/pdlp/cusparse_view.cu (1)
147-149: 💤 Low valueInconsistent error-checking macro.
Line 147 uses
CUSPARSE_CHECKwhile other cuSPARSE calls in this file useRAFT_CUSPARSE_TRY. Consider usingRAFT_CUSPARSE_TRYfor consistency.🤖 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 `@cpp/src/pdlp/cusparse_view.cu` around lines 147 - 149, Replace the inconsistent CUSPARSE_CHECK call with the RAFT_CUSPARSE_TRY macro to match the rest of the file: change the call to cusparseSetStream(...) so it is wrapped with RAFT_CUSPARSE_TRY rather than CUSPARSE_CHECK, keeping the same arguments and preserving the subsequent RAFT_CUSPARSE_TRY(cusparseSpMM_preprocess(...)) call; ensure you reference and use the RAFT_CUSPARSE_TRY macro for both cusparseSetStream and cusparseSpMM_preprocess to maintain consistent error handling.
🤖 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 `@cpp/src/pdlp/cusparse_view.cu`:
- Around line 213-218: The code dereferences the optional dynamic loader result
fn (dynamic_load_runtime::function<cusparseSpMVOp_createDescr_sig>) without
checking presence; update the factory that creates the cusparseSpMVOp_descr (the
block that calls (*fn)(...)) to first test fn.has_value() (or if(!fn) branch)
and handle the missing symbol by returning an empty cusparse_spmvop_descr_uptr
(or otherwise propagate a clear error) instead of dereferencing; ensure the
handling is applied where cusparseSpMVOp_createDescr is invoked so callers of
is_cusparse_runtime_spmvop_supported() are not assumed sufficient.
- Around line 221-228: The dynamic loader result `fn` in make_spmvop_plan is
dereferenced without checking it exists; update make_spmvop_plan to test
dynamic_load_runtime::function<...> fn for presence (e.g., if (!fn) throw or
return an error) before calling (*fn)(...), mirroring the fix used in
make_spmvop_descr so that cusparseSpMVOp_createPlan is only invoked when the
symbol lookup succeeded and RAFT_CUSPARSE_TRY is reached with a valid function
pointer.
---
Outside diff comments:
In `@cpp/src/pdlp/cusparse_view.cu`:
- Around line 240-242: The code dereferences the optional
dynamic_load_runtime::function<cusparseSpMVOp_sig> named func without checking
it; before calling (*func)(handle, plan, alpha, beta, vecX, vecY, vecZ) add a
check like if (!func.has_value()) and handle the failure (log or return an
error/throw) with a clear message including the symbol name "cusparseSpMVOp";
keep the existing RAFT_CUSPARSE_TRY usage for actual cuSPARSE calls and ensure
the early error path prevents the dereference of func and returns/propagates an
appropriate error.
---
Nitpick comments:
In `@cpp/src/pdlp/cusparse_view.cu`:
- Around line 147-149: Replace the inconsistent CUSPARSE_CHECK call with the
RAFT_CUSPARSE_TRY macro to match the rest of the file: change the call to
cusparseSetStream(...) so it is wrapped with RAFT_CUSPARSE_TRY rather than
CUSPARSE_CHECK, keeping the same arguments and preserving the subsequent
RAFT_CUSPARSE_TRY(cusparseSpMM_preprocess(...)) call; ensure you reference and
use the RAFT_CUSPARSE_TRY macro for both cusparseSetStream and
cusparseSpMM_preprocess to maintain consistent error handling.
In `@cpp/src/pdlp/cusparse_view.hpp`:
- Around line 38-57: The structs cusparse_sp_mat_deleter,
cusparse_dn_vec_deleter, and cusparse_dn_mat_deleter do not follow the project
naming convention requiring a snake_case_t suffix; rename them to
cusparse_sp_mat_deleter_t, cusparse_dn_vec_deleter_t, and
cusparse_dn_mat_deleter_t respectively, update all uses/typedefs/usings in this
compilation unit and any headers that reference these types (e.g., unique_ptr
deleter specializations or variable declarations), and ensure the operator()
implementations remain unchanged and still call RAFT_CUSPARSE_TRY_NO_THROW on
cusparseDestroySpMat/cusparseDestroyDnVec/cusparseDestroyDnMat.
🪄 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.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 43105512-0a29-4bb5-b0f5-b5cd32da0e79
📒 Files selected for processing (5)
cpp/src/barrier/barrier.cucpp/src/barrier/cusparse_info.hppcpp/src/barrier/cusparse_view.cucpp/src/pdlp/cusparse_view.cucpp/src/pdlp/cusparse_view.hpp
I love the clean up effort! Leaving the review for the experts. |
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
Bubullzz
added
improvement
|
/ok to test 036469d |
|
/ok to test 520218a |
|
/ok to test 5e199e8 |
|
/ ok to test ba95897 |
|
/ok to test 90f1214 |
@Bubullzz, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test b5ffd92 |
|
/ok to test 8bdebd7 |
|
/ok to test f5eeb57 |
|
/ok to test f5eeb57 |
@Bubullzz, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 5c436b7 |
|
/ok to test 1afbe40 |
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
Resolve barrier cuSPARSE conflicts by keeping unique_ptr-based RAII instead of main's reintroduced wrapper types, and update namespace references from detail:: to pdlp:: after the mathematical_optimization rename.
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
1 similar comment
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
|
/ok to test 7421314 |
tmckayus
left a comment
tmckayus
left a comment
There was a problem hiding this comment.
unclear to me why the PR is calling for infra code owners to approve because I don't see any test changes, but I'm approving to satisfy that status.
|
@Bubullzz please see this error from check_style. If you run the pre-commit hooks to enforce style, then the cpp builds and tests will run and we can get the full checks "If you are seeing this message in CI, reproduce locally with: |
CI Test Summary✅ All 0 test job(s) passed. |
4 participants
Currently in CuOpt we have many wrappers around cusparse data structures to implement the current behaviour:
All of these features are handmade using an internal
need_destructionstate. This seems dangerous and easily replacable by a unique_ptr.In this PR I replace these wrappers with unique_ptr when they are owned and I replace them with direct pointers, alliased as cusparse_object_view when passed as argument to functions.