Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enable Dask tests with UCX-Py/UCXX in CI #5697
Enable Dask tests with UCX-Py/UCXX in CI #5697
Changes from 23 commits
a076e1e
348407e
d4988c2
b88b7bd
b07197f
f60d58a
55ad2da
3a06e29
c049eb3
2fa180f
b4e22fb
cee0d25
a695651
13f1d4c
5b3d468
8ed59fe
4e20acf
8359624
57c5f4f
6d0819f
b7c31d1
ea613f0
2d39085
764bba9
f5fd345
f4d14f4
055beb8
2cb76dd
d28568c
364dcb6
6be59e8
89484c3
1f29e03
06c6c7a
13862f2
31fb4a6
ce9590a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not rely on raft-dask to pull in the ucxx dependency? cuml doesn't use it directly, only via raft-dask, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intent was to make
ucxx
an optional dependency ofraft-dask
although I now realize that's not the case in https://github.com/rapidsai/raft/blob/branch-24.06/conda/recipes/raft-dask/meta.yaml . I think it's ok to haveucxx
(and thuslibucxx
) pulled in by RAFT -- even though it's technically not a hard dependency because it won't be activated by default -- but it will bloat a bit the installation. What I'm saying is we have two options:ucxx
a soft dependency ofraft-dask
and then leave this here; orucxx
as a hard dependency ofraft-dask
and remove this.Do you have thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just stick with a hard dependency for now. It's easier to manage and that is the long-term plan anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just thinking a bit more about this and perhaps it should indeed be an optional dependency everywhere, both in
raft-dask
andcuml
. Since UCX is not a requirement to run either one (people may choose NCCL as a replacement, for example) it isn't the case that users always require either UCX-Py (or UCXX).If it makes things simpler for us for the moment, I think it's ok to make it a hard dependency, but in the long run I don't think it makes sense from the packaging perspective to require UCX-Py/UCXX. WDYT @vyasr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately raft-dask is going to have to be compiled against libucxx, right? The C++ dependency will be there no matter what, unless you plan to rewrite the raft C++ to use a dlopen of ucxx.