Drop constness in raft handle for data_model_view_t (routing) - #1241
Conversation
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…o a recent change in raft. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
nguidotti
added
bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📝 WalkthroughWalkthroughConst qualifiers removed from RAFT handle parameters and stored pointer: data_model_view now stores/returns mutable handles; generator helper functions and Cython/pxd bindings take non-const handles; a utilities call site now passes the handle pointer directly. ChangesHandle constness propagation
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/routing/utilities/cython.cu (1)
111-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject shared handles before mutating streams in parallel.
This loop changes the CUDA stream on each model's RAFT handle from multiple OpenMP workers. If two
data_modelsalias the sameraft::handle_t, theset_cuda_stream/restore sequence races and the solves can run on the wrong stream nondeterministically. Please enforce distinct handles up front or create a per-worker handle copy instead of mutating shared state.One simple guard before entering the parallel region
+#include <unordered_set> + const std::size_t size = data_models.size(); std::vector<std::unique_ptr<vehicle_routing_ret_t>> list(size); + std::unordered_set<raft::handle_t*> seen_handles; + for (auto* data_model : data_models) { + cuopt_expects(data_model != nullptr, + error_type_t::ValidationError, + "data_model cannot be null"); + auto* handle_ptr = data_model->get_handle_ptr(); + cuopt_expects(handle_ptr != nullptr, + error_type_t::ValidationError, + "data_model handle cannot be null"); + cuopt_expects(seen_handles.insert(handle_ptr).second, + error_type_t::ValidationError, + "call_batch_solve requires a distinct RAFT handle per data model"); + }As per coding guidelines, "Flag missing synchronization causing non-deterministic GPU failures" and "Flag missing input validation at library and server boundaries".
Also applies to: 152-154
🤖 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/routing/utilities/cython.cu` around lines 111 - 123, The loop mutates each model's RAFT handle CUDA stream (data_models[i]->get_handle_ptr() and raft::resource::set_cuda_stream) from multiple OpenMP threads, which races if two data_models alias the same raft::handle_t; before entering the parallel region, detect if any get_handle_ptr() pointers are duplicated and either (A) reject/raise an error describing non-unique handles, or (B) create per-worker raft::handle_t copies (or per-thread handles) and use those local handles/streams in the parallel loop (use the copied handle in place of data_models[i]->get_handle_ptr() when calling sync_stream and set_cuda_stream and restore on the copy), ensuring device_id and stream_pool usage remain correct; update both the loop starting at the RAFT_CUDA_TRY(cudaSetDevice(device_id)) block and the similar block at lines ~152-154 to use the same guard/copy strategy.
🤖 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/include/cuopt/routing/data_model_view.hpp`:
- Around line 614-617: The accessor get_handle_ptr on data_model_view_t breaks
const-correctness by returning a mutable raft::handle_t* from a const method;
fix this by providing two overloads: a non-const getter raft::handle_t*
get_handle_ptr() noexcept and a const getter raft::handle_t const*
get_handle_ptr() const noexcept (leave handle_ptr_ as raft::handle_t*
handle_ptr_{nullptr}); update call sites to accept the const-pointer from const
objects or cast only where mutation is intended.
---
Outside diff comments:
In `@cpp/src/routing/utilities/cython.cu`:
- Around line 111-123: The loop mutates each model's RAFT handle CUDA stream
(data_models[i]->get_handle_ptr() and raft::resource::set_cuda_stream) from
multiple OpenMP threads, which races if two data_models alias the same
raft::handle_t; before entering the parallel region, detect if any
get_handle_ptr() pointers are duplicated and either (A) reject/raise an error
describing non-unique handles, or (B) create per-worker raft::handle_t copies
(or per-thread handles) and use those local handles/streams in the parallel loop
(use the copied handle in place of data_models[i]->get_handle_ptr() when calling
sync_stream and set_cuda_stream and restore on the copy), ensuring device_id and
stream_pool usage remain correct; update both the loop starting at the
RAFT_CUDA_TRY(cudaSetDevice(device_id)) block and the similar block at lines
~152-154 to use the same guard/copy strategy.
🪄 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: 76f94c06-143b-464b-88bf-129e6b61b558
📒 Files selected for processing (3)
cpp/include/cuopt/routing/data_model_view.hppcpp/src/routing/data_model_view.cucpp/src/routing/utilities/cython.cu
|
The breaking tag shouldn't be needed for C++ API changes. |
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
|
/ok to test 5ff3738 |
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
|
/ok to test 35cc92c |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/routing/generator/generator.hpp`:
- Around line 106-111: The header declaration of generate_vehicle_time_windows
currently includes an unused parameter fleet_order_constraints that is not
present in the implementation or call site; remove the fleet_order_constraints
parameter from the function declaration of generate_vehicle_time_windows (in
generator.hpp) so its signature matches the definition in generator.cu and the
call site, ensuring the template parameters and other arguments (handle, params,
order_info, fleet_size) remain unchanged.
🪄 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: a342b7e9-1b2f-4cbc-9ff5-0eedd0b00a6d
📒 Files selected for processing (5)
cpp/include/cuopt/routing/cython/cython.hppcpp/src/routing/generator/generator.cucpp/src/routing/generator/generator.hppcpp/src/routing/utilities/cython.cupython/cuopt/cuopt/routing/structure/routing_utilities.pxd
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
| * @brief Get raft handle object containing GPU resource objects | ||
| * @return Handle object | ||
| */ | ||
| raft::handle_t* get_handle_ptr() noexcept; |
There was a problem hiding this comment.
We should add a const overload. Maybe even have
raft::handle_t const* get_handle_ptr() const noexcept;
raft::handle_t* get_mutable_handle_ptr() noexcept;
In the future run we can potentially take out the ownership of the handle from data_model_view_t and pass it to solve(&handle, data_model, settings) or have an owning data_model_t
There was a problem hiding this comment.
There is a const overload right below. I think the non-const version should be right below it; shouldn't need a different docstring.
There was a problem hiding this comment.
Sounds good. I wanted to avoid silent modifications as constness of the handle during solve is a strong assumption.
|
/merge |
|
Many thanks for fixing this and sorry I didn't spot this before! |
7 participants
Dropped the
constqualifier for theraft::handle_ton thedata_model_view_t(routing) due to the recent changes inraft::resources(NVIDIA/raft#3005).Checklist