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] fetch thrust/cub from github #378

Merged
merged 5 commits into from
Jun 5, 2020

Conversation

rongou
Copy link
Contributor

@rongou rongou commented May 22, 2020

Use FetchContent to obtain thrust/cub release 1.9.0. This is needed to get per-thread default stream to work properly. Otherwise the rmm::device_vector falls back to the legacy default stream for some operations.

@harrism @jrhemstad @kkraus14

@rongou rongou requested a review from a team as a code owner May 22, 2020 00:18
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@harrism
Copy link
Member

harrism commented May 25, 2020

@mt-jones can you review this?

@harrism harrism requested a review from mtjrider May 25, 2020 02:31
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

This is a significant change in behavior that shouldn't be made hastily. We shouldn't be jumping the gun on updating to the bleeding edge of Thrust/CUB in RMM when interaction with libcudf hasn't been tested. Furthermore, we haven't had a conversation about what we want to do with Thrust/CUB versioning in RAPIDS libraries.

@jrhemstad
Copy link
Contributor

Otherwise the rmm::device_vector falls back to the legacy default stream for some operations.

Users can build RMM against a custom Thrust version if they want to get this fix in the meantime

@rongou
Copy link
Contributor Author

rongou commented May 26, 2020

I agree this can potentially be a big change for cuDF, but I'm not so sure about RMM. The thrust api surface for RMM is pretty small and looks to be stable. Since RMM and cuDF are in two separate repos, this necessarily has to be a two-step process. I can create a PR under cuDF to also pull in the thrust/cub release and let it run through the CI. Would that help?

This is really the first step towards enabling PTDS for Spark. We probably should add some multi-threaded host tests for both RMM and cuDF to verify PTDS behavior. Changing the Spark build with the new thrust release is fine for a prototype, but it's hardly "production ready" when neither RMM or cuDF has been built or tested with the release.

@jrhemstad
Copy link
Contributor

I can create a PR under cuDF to also pull in the thrust/cub release and let it run through the CI. Would that help?

It would be a necessary first step, yes. But that alone isn't enough. We need to decide if we are okay with overriding Thrust that is shipped with the toolkit.

@harrism harrism added 3 - Ready for review Ready for review by team CMake labels May 28, 2020
@mtjrider
Copy link
Contributor

mtjrider commented May 28, 2020

@jrhemstad is there a place where discussing thrust override is taking place?

If so, can it be linked to this PR?

@harrism
Copy link
Member

harrism commented May 29, 2020

@mt-jones the discussion was on slack, can't link it here.

@kkraus14
Copy link
Contributor

@mt-jones the discussion was on slack, can't link it here.

Sounds like we need to open a github issue with a summary of the discussion and current thoughts so that we can have an open conversation.

@leofang
Copy link
Member

leofang commented May 29, 2020

Sounds like we need to open a github issue with a summary of the discussion and current thoughts so that we can have an open conversation.

@kkraus14 I happened to open one here as you typed! 😂
conda-forge/cub-feedstock#4

@harrism
Copy link
Member

harrism commented Jun 1, 2020

Sounds like we need to open a github issue with a summary of the discussion and current thoughts so that we can have an open conversation.

@kkraus14 I happened to open one here as you typed! joy
conda-forge/cub-feedstock#4

@leofang this discussion is about RAPIDS depending on CUB and Thrust from github rather than from the CUDA toolkit, so discussing it on conda-forge would not be appropriate. :) I opened a cuDF issue here: rapidsai/cudf#5346

@rongou rongou requested a review from jrhemstad June 5, 2020 01:44
@harrism
Copy link
Member

harrism commented Jun 5, 2020

Merged rapidsai/cudf#5315, so merging this.

@harrism harrism merged commit 89f6bfe into rapidsai:branch-0.15 Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for review Ready for review by team CMake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants