Replaced cusparse wrappers with simple unique_ptrs and more RAII #1342
Open
Bubullzz wants to merge 16 commits into
Open
Replaced cusparse wrappers with simple unique_ptrs and more RAII #1342Bubullzz wants to merge 16 commits into
Bubullzz wants to merge 16 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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors cuSPARSE descriptor ownership across barrier and PDLP code from wrapper and raw handles to RAII ChangescuSPARSE Descriptor RAII Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 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 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
Contributor
I love the clean up effort! Leaving the review for the experts. |
Kh4ster
requested review from
Kh4ster
and removed request for
chris-maes and
rg20
|
🔔 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
Contributor
Author
|
/ok to test 036469d |
Contributor
Author
|
/ok to test 520218a |
Contributor
Author
|
/ok to test 5e199e8 |
Contributor
Author
|
/ ok to test ba95897 |
Contributor
Author
|
/ok to test 90f1214 |
Contributor
Author
|
/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/ |
Contributor
Author
|
/ok to test b5ffd92 |
Contributor
Author
|
/ok to test 8bdebd7 |
Contributor
Author
|
/ok to test f5eeb57 |
Contributor
Author
|
/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/ |
Contributor
Author
|
/ok to test 5c436b7 |
Contributor
Author
|
/ok to test 1afbe40 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.