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

Add initial cupyx.spatial.distance support from pylibraft #6690

Merged
merged 35 commits into from Jun 23, 2022

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Apr 28, 2022

This is an initial PR to establish integration of pylibraft within cupy. For now, the intention is to use RAFT for supported distances and types and cupy kernels for other distances and types until we support them in RAFT. This provides us a path to make use of what's available and then continue optimizing RAFT underneath, allowing cupy to reap the benefits immediately.

This is really just a start in the direction of backing the entire cupyx.scipy.spatial package with RAFT, as well as the cupyx.scipy.cluster and cupyx.scipy.optimize packages (and potentially cupyx.scipy.stats / cupyx.scipy.qmc in the future).

Tasks:

  • Add cdist
  • Add scipy.spatial.distance_matrix, minkowski_distance
  • Add tests

@emcastillo emcastillo self-assigned this May 9, 2022
@emcastillo emcastillo added cat:feature New features/APIs prio:medium labels May 9, 2022
@emcastillo
Copy link
Member

I think the current functionality is enough for this PR.
Can we split the other APIs in subsequent ones?
Thanks!!!

@cjnolet
Copy link
Member Author

cjnolet commented May 9, 2022

@emcastillo sounds great. I'll do that

@cjnolet cjnolet marked this pull request as ready for review May 10, 2022 23:08
@cjnolet
Copy link
Member Author

cjnolet commented May 10, 2022

This PR depends upon the changes in rapidsai/raft#643 (which should be merged shortly).

@cjnolet
Copy link
Member Author

cjnolet commented May 10, 2022

@emcastillo, one thing I've noticed is that I don't believe pylibcugraph is being tested in CuPy's CI. At least I don't think it is, because I don't see it listed in any dependencies in the build so I'm assuming those tests are getting skipped.

It would be nice if we could run the tests for pylibraft and I think we might be safe installing it since I don't believe any of the RAFT python artifacts have any dependency on cupy. I suppose we could either just install it as a test or runtime dependency but I think that will depend on whether the sizes of dependent artifacts are a concern. What are your thoughts here?

@emcastillo
Copy link
Member

We will try to add pylibcugraph to the CI!

@emcastillo
Copy link
Member

emcastillo commented May 12, 2022

seems like it is installed in CI rapids configuration?

https://ci.preferred.jp/cupy.linux.cuda-rapids/101331/
This should test with libcugraph enabled

Need to add
tests/cupyx_tests/scipy_tests/spatial_tests
to https://github.com/cupy/cupy/blob/master/.pfnci/linux/tests/cuda-rapids.sh#L15

@kmaehashi
Copy link
Member

kmaehashi commented May 12, 2022

We intend to test CuPy code depending on RAPIDS in cuda-rapids test variant.

These are separated from regular unit tests (which are based on pip) as RAPIDS installation requires conda.

CI can be triggered in pull-requests using /test rapids (run the RAPIDS test variant only) or /test mini,rapids (run regular unit tests + RAPIDS test variant).

@cjnolet
Copy link
Member Author

cjnolet commented Jun 14, 2022

/test mini, rapids

@cjnolet
Copy link
Member Author

cjnolet commented Jun 15, 2022

/test mini

@cjnolet
Copy link
Member Author

cjnolet commented Jun 15, 2022

/test mini

@cjnolet
Copy link
Member Author

cjnolet commented Jun 15, 2022

/test rapids

@cjnolet
Copy link
Member Author

cjnolet commented Jun 15, 2022

/test mini,rapids

@cjnolet
Copy link
Member Author

cjnolet commented Jun 15, 2022

/test mini, rapids

@emcastillo
Copy link
Member

/test mini, rapids

@cjnolet
Copy link
Member Author

cjnolet commented Jun 16, 2022

@emcastillo we're working on the fix for the RAPIDS core-devel container build and then this should be ready.

@jakirkham
Copy link
Member

Depends on PR ( rapidsai/raft#714 )

@cjnolet
Copy link
Member Author

cjnolet commented Jun 17, 2022

/test rapids

@cjnolet
Copy link
Member Author

cjnolet commented Jun 17, 2022

/test rapids

@cjnolet
Copy link
Member Author

cjnolet commented Jun 17, 2022

@emcastillo @kmaehashi looks like the RAPIDS tests are finally running (and passing!).

Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM!

@jakirkham
Copy link
Member

Thanks all! 🙏 Looking forward to using these 😄

@cjnolet
Copy link
Member Author

cjnolet commented Jun 23, 2022

Yes, thanks again @emcastillo @kmaehashi and @leofang. As requested by @emcastillo, We'll continue opening smaller PRs to continue to fill in the API support now that we've got the integration path working end-to-end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants