Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEA] Update raft::handle_t to be coupled with one cudaStream_t instance #293

Closed
divyegala opened this issue Jul 14, 2021 · 1 comment · Fixed by #291
Closed

[FEA] Update raft::handle_t to be coupled with one cudaStream_t instance #293

divyegala opened this issue Jul 14, 2021 · 1 comment · Fixed by #291
Assignees
Labels
feature request New feature or request Public API

Comments

@divyegala
Copy link
Member

divyegala commented Jul 14, 2021

The idea is to create a "light" handle that does not do any resource initialization except on request. The changes to the handle will be as follows:

  1. Copy and move constructors/assignment operators are now deleted, since it's unsafe to be sure about safely copying or moving the number of resources held by the handle
  2. The only constructor will now be raft::handle_t(rmm::cuda_stream_view = {}, const rmm::cuda_stream_pool& = {0}. This means both the main stream and stream pool are now not owned by the handle, except for NULL stream and 0 sized pool
  3. The above ensures that the handle is always created with a light copy of a pre-existing stream and stream pool. It also ensures that no stream or stream pool will be deleted by mistake in the scope of the handle. This was not confirmed by the previous handle.set_stream() method
  4. The idea behind rmm::cuda_stream_pool being created by the developer and not the handle allows for the safe creation and deletion of streams (which are a light resource) by the scope which they are bound. This will enable several programming patterns:

Previous pattern No. 1:

void foo() {
  // work on main stream
  // code block requiring multiple streams
  // continue on main stream
}

New pattern No. 1:

// refactor code-block with multiple streams
void multiple_stream_foo() {
    // create a stream pool with as many streams as needed at this point
    rmm::cuda_stream_pool stream_pool(n_streams);
    // code block requiring multiple streams
    // pool deleted and synced at end if needed
}

void foo() {
    // work on main stream
    multiple_stream_foo();
    // continue work on main stream
}

Previous pattern No. 2:

raft::handle_t h(5);
for (int i = 0; i < 5; i++)
    // confusing extra stream parameter on prim
    prim(h, ..., h.get_internal_stream(i));

New pattern No. 2:

rmm::cuda_stream_pool stream_pool(5);
raft::handle_t heavy(rmm::cuda_stream_default, stream_pool)
for (int i = 0; i < 5; i++)
    // create light handle with stream from pool
   // similar to thrust API for example, each API has a main stream
    prim(raft::handle_t(heavy.get_stream_pool().get_stream()), ...);
  1. New APIs called raft::handle_t::get_stream() and raft::handle_t::get_stream_pool()
  2. 3 new APIs for stream sync (previous ones were not syncing main stream correctly):
    raft::handle_t::sync_stream() -> syncs main stream
    raft::handle_t::sync_stream_pool() -> syncs stream pool if size > 0
    raft::handle_t::wait_stream_pool_on_stream() -> make stream pool wait on main stream to finish its latest task
  3. Remove raft::handle_t::set_stream() method since a new light raft handle can be created instead, without needing to update resources of previous handle. Also removed equivalent method from cython
  4. CUDA math libraries to use setStream* function to correctly set their handles on main stream. Previously, this was not being done
  5. Related updates to cython/python API
  6. New cython Handle constructor can accept stream=None and n_streams=0 in its constructor
@github-actions
Copy link

github-actions bot commented Dec 1, 2021

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@rapids-bot rapids-bot bot closed this as completed in #291 Dec 13, 2021
rapids-bot bot pushed a commit that referenced this issue Dec 13, 2021
closes #293 
closes #115 

This PR also updates the cython build to `std=c++17`.

Authors:
  - Divye Gala (https://github.com/divyegala)
  - Corey J. Nolet (https://github.com/cjnolet)

Approvers:
  - Chuck Hastings (https://github.com/ChuckHastings)
  - William Hicks (https://github.com/wphicks)
  - Seunghwa Kang (https://github.com/seunghwak)
  - Corey J. Nolet (https://github.com/cjnolet)
  - Tamas Bela Feher (https://github.com/tfeher)
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: #291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request Public API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants