Skip to content

Fix cagra::optimize modifying the state of raft::resources#2103

Merged
rapids-bot[bot] merged 1 commit into
release/26.06from
achirkin-patch-7
May 18, 2026
Merged

Fix cagra::optimize modifying the state of raft::resources#2103
rapids-bot[bot] merged 1 commit into
release/26.06from
achirkin-patch-7

Conversation

@achirkin
Copy link
Copy Markdown
Contributor

The new, improved version of CAGRA optimize() function makes use of raft CUDA stream pool resource.
Setting the resource (raft::resource::set_cuda_stream_pool) affects the state of raft::resources and may lead to unintended consequences for a user.
Using non-const raft::resources will be enforced at compile time in rapidsai/raft#3005 , so this PR fixes the use case ahead of the upstream change by copying the resources handle.

The new, improved version of CAGRA optimize() function makes use of raft CUDA stream pool resource.
Setting the resource (`raft::resource::set_cuda_stream_pool`) affects the state of `raft::resources` and may lead to unintended consequences for a user.
Using non-const `raft::resources` will be enforced at compile time in rapidsai/raft#3005 , so this PR fixes the use case ahead of the upstream change by copying the resources handle.
@achirkin achirkin self-assigned this May 18, 2026
@achirkin achirkin requested a review from a team as a code owner May 18, 2026 08:36
@achirkin achirkin added bug Something isn't working non-breaking Introduces a non-breaking change labels May 18, 2026
@achirkin achirkin moved this to In Progress in Unstructured Data Processing May 18, 2026
@achirkin achirkin requested a review from mfoerste4 May 18, 2026 08:37
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ea3ae3df-fa6f-4ab7-813c-9bf57e217e65

📥 Commits

Reviewing files that changed from the base of the PR and between 972de08 and 5b9a38e.

📒 Files selected for processing (1)
  • cpp/src/neighbors/detail/cagra/graph_core.cuh

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Updated internal resource handling in graph optimization logic to prevent unintended state mutations during processing.

Walkthrough

The optimize() function signature is changed to accept resources as a const reference parameter (res_const), and the function now immediately constructs a mutable local copy (res) for internal operations. Comments document the rationale: stream-pool modification requires non-const resources, and this pattern isolates such modifications from affecting the caller's input.

Changes

Resource parameter isolation

Layer / File(s) Summary
Resource parameter const-correctness with local copy
cpp/src/neighbors/detail/cagra/graph_core.cuh
The optimize() function parameter is renamed to res_const and the function constructs a local mutable copy for stream-pool setup, with clarifying comments explaining the non-const requirement and isolation intent.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly summarizes the main change: fixing cagra::optimize to avoid modifying raft::resources state.
Description check ✅ Passed The description explains the problem (state modification of raft::resources), the solution (copying the resources handle), and the motivation (upstream RAFT changes requiring const resources).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch achirkin-patch-7

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@mfoerste4 mfoerste4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @achirkin for fixing this!

Copy link
Copy Markdown
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

// raft::resource::set_cuda_stream_pool below modifies the resource, so it cannot be const.
// The optimize() is a heavy function, so copying the resource and creating a private stream pool
// is not a big overhead.
raft::resources res{res_const};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a GitHub issue for this and link it in the code so we don't lose sight of this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achirkin
Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot rapids-bot Bot merged commit 507c1ec into release/26.06 May 18, 2026
88 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Unstructured Data Processing May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Introduces a non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants