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

Pass stream to device_scalar::value() calls. #259

Merged

Conversation

harrism
Copy link
Member

@harrism harrism commented Jun 8, 2021

In comms/test.hpp device_scalar::value was not being passed an explicit stream, which means that the default stream was being synced. rapidsai/rmm#789 will remove the default from this parameter, and would have therefore broken the RAFT build. So this PR fixes the oversynchronization and ensures RAFT will build after the RMM PR is merged.

Note this PR includes the cmake changes from #258 (just so I could build locally). Once #258 is merged this PR's changes will be simplified.

Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Changes look good to me, same comment about merging 21.08 so CI passes

@harrism
Copy link
Member Author

harrism commented Jun 8, 2021

Changes look good to me, same comment about merging 21.08 so CI passes

Done.

@dantegd dantegd added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 8, 2021
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Jun 8, 2021
This PR refactors `device_scalar` to use a  single-element `device_uvector` for its storage. This simplifies the implementation of device_scalar. Also changes the API of `device_scalar` so that its asynchronous / stream-ordered methods use the same API style (with explicit stream parameter) as `device_uvector` and `device_buffer`.

Closes #570

This is a breaking change. When it is merged, PRs are likely to need to be merged immediately in other libraries to account for the API changes. 

 - [x] cuDF: rapidsai/cudf#8411
 - [x] cuGraph: rapidsai/cugraph#1637
 - [x] RAFT: rapidsai/raft#259  
 - [x] ~cuML~ (unused)
 - [x] ~cuSpatial~ (unused)

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Rong Ou (https://github.com/rongou)
  - Jake Hemstad (https://github.com/jrhemstad)

URL: #789
@harrism
Copy link
Member Author

harrism commented Jun 8, 2021

@gpucibot merge

@harrism
Copy link
Member Author

harrism commented Jun 8, 2021

I don't think I can merge.

@ajschmidt8
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit f65ed02 into rapidsai:branch-21.08 Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants