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

[Review] Enable matrix::copyRows for row major input #176

Merged
merged 3 commits into from
Mar 27, 2021

Conversation

tfeher
Copy link
Contributor

@tfeher tfeher commented Mar 19, 2021

Enable matrix::copyRows operation for row_major input. This will be used in SVM to enable row major input rapidsai/cuml#2198

We use a helper function prim cache:get_vecs() from cuML. Because of this, the cache_util.cuh file is copied to raft (only the namespace is updated). A separate PR shall update the the rest of the cache function and the associated tests and move it to RAFT.

@tfeher tfeher added 3 - Ready for Review improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 19, 2021
@github-actions github-actions bot added the cpp label Mar 19, 2021
@tfeher
Copy link
Contributor Author

tfeher commented Mar 23, 2021

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Mar 23, 2021

@tfeher, the Linkage gtest assertion failure should be fixed when #178 is merged. I'm still investigating timeouts in cuda 10.2. Don't have any leads on that yet, unfortunately.

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM. Mostly just small typos and I don't think those need to hold up the merge if they aren't fixed right away.

cpp/include/raft/cache/cache_util.cuh Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

Thanks @cjnolet for the review! I have addressed the issues.

cpp/include/raft/cache/cache_util.cuh Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
cpp/include/raft/cache/cache_util.cuh Outdated Show resolved Hide resolved
@cjnolet
Copy link
Member

cjnolet commented Mar 24, 2021

@tfeher, I just merged branch-0.19 into your branch in hopes maybe CI wasn't seeing some of the recent changes. I'm still at a loss for why these hangs are occurring in CI.

@cjnolet
Copy link
Member

cjnolet commented Mar 24, 2021

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Mar 24, 2021

The failures appear to be related to whether a P100 is selected as the CI machine. I notice the tests never timeout when a V100 or A100 is selected.

@cjnolet
Copy link
Member

cjnolet commented Mar 26, 2021

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Mar 26, 2021

@gpucibot merge

@cjnolet cjnolet merged commit aed0ed5 into rapidsai:branch-0.19 Mar 27, 2021
dantegd pushed a commit to dantegd/raft that referenced this pull request Jul 23, 2024
Forward-merge branch-24.06 into branch-24.08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge 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.

2 participants