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

Generic linalg::map #1337

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Conversation

achirkin
Copy link
Contributor

Update the implementation behind raft::linalg::map and raft::linalg::map_offset to allow multiple inputs and optional index.

Originally, this is a part of the effort to reduce the latency of ivf-pq search. The new implementation replaces several helpers, which have been using thrust; at the moment, raft uses a thrust policy that occasionally inserts extra cudaStreamSynchronize, and this negatively affects the latency on small inputs.

The new implementation is generic enough to replace many raft's utility functions. It uses vectorized load/stores if possible, which improves performance.

This is the second take on the PR #1329 that keeps the deprecated map_k function, which is used in cuml.

@achirkin achirkin requested a review from a team as a code owner March 14, 2023 05:19
@github-actions github-actions bot added the cpp label Mar 14, 2023
@cjnolet
Copy link
Member

cjnolet commented Mar 14, 2023

/merge

@rapids-bot rapids-bot bot merged commit 1053f29 into rapidsai:branch-23.04 Mar 14, 2023
@MatthiasKohl
Copy link
Contributor

@cjnolet This PR just broke our builds in cugraph-ops because it uses a host constexpr function (std::min) in device code.
The relevant compiler flag is mentioned here: https://github.com/rapidsai/raft/blob/branch-23.04/docs/source/build.md#using-raft-in-downstream-projects
but I was just wondering: is there some list of compiler flags that's required to build RAFT?
It's possible that we're missing other flags like this one

lowener pushed a commit to lowener/raft that referenced this pull request Mar 15, 2023
Update the implementation behind `raft::linalg::map` and `raft::linalg::map_offset` to allow multiple inputs and optional index.

Originally, this is a part of the effort to reduce the latency of ivf-pq search. The new implementation replaces several helpers, which have been using thrust; at the moment, raft uses a thrust policy that occasionally inserts extra `cudaStreamSynchronize`, and this negatively affects the latency on small inputs.

The new implementation is generic enough to replace many raft's utility functions. It uses vectorized load/stores if possible, which improves performance.

This is the second take on the PR rapidsai#1329 that keeps the deprecated `map_k` function, which is used in cuml.

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: rapidsai#1337
@MatthiasKohl
Copy link
Contributor

@achirkin @cjnolet IIUC, this PR breaks at least the API semantics of raft::linalg::writeOnlyUnaryOp because the Lambda now takes as first input a reference to the output that should be written to the output pointer, whereas previously it took as first input the output pointer.
This was also documented clearly at the API level here :

* @tparam Lambda Device lambda performing the actual operation, with the signature
* `void func(OutType* outLocationOffset, IdxType idx);`
* where outLocationOffset will be out + idx.

Semantically, the new function doesn't do the same thing at all, and it took me a lot of time to trace our issues in CI to this.

I know this PR is marked as breaking but it would be great that if we change the semantics of very generic functions like these, we notify others ahead of time so that we can check whether we make use of those semantics (that were guaranteed before, see the docs linked above !).

Thanks

@cjnolet
Copy link
Member

cjnolet commented Mar 16, 2023

@MatthiasKohl I do apologize for this. I did a quick verification to make sure this wouldn't break any other repositories downstream but I admit I did not check cugraph-ops (and am honestly surprised to see that it's being used there).

Would it help if we went ahead and backed out the change for the write only unary op for now? Please note that we are planning to deprecate the writeOnlyUnaryOp function very soon in favor of map_offset anyways, unless there's any pressing need to keep the original semantics as-is.

@MatthiasKohl
Copy link
Contributor

@cjnolet I already updated our codes now, so everything should still work correctly with the next PR being merged on our side. I don't think it's worth it to back out again, but I wasn't sure if other projects weren't affected by this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review breaking Breaking change cpp feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants