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.

Sign up for GitHub

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

Jump to bottom

Switch thrust over to use rapids-cmake patches #265

Merged
merged 1 commit into from
Oct 3, 2022
Merged

Switch thrust over to use rapids-cmake patches #265

merged 1 commit into from
Oct 3, 2022

Conversation

robertmaynard
Copy link
Contributor

@robertmaynard robertmaynard commented Sep 8, 2022 •
edited
Loading

Leverages the infrastructure of #264 to switch rapids_cpm_thrust over to using the built-in patch support.

@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change 5 - Merge After Dependencies Depends on another PR: do not merge out of order. labels Sep 8, 2022
@robertmaynard robertmaynard requested a review from a team as a code owner September 8, 2022 15:39
@robertmaynard robertmaynard changed the title Fea/switch thrust and nvcomp over to patches Switch thrust and nvcomp over to use rapids-cmake patches Sep 8, 2022
bdice
bdice reviewed Sep 8, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I have only minor comments. Overall this looks good.

docs/packages/patches.json Outdated Show resolved Hide resolved
rapids-cmake/cpm/detail/display_patch_status.cmake Outdated Show resolved Hide resolved
rapids-cmake/cpm/detail/generate_patch_command.cmake Outdated Show resolved Hide resolved
rapids-cmake/cpm/detail/generate_patch_command.cmake Outdated Show resolved Hide resolved
@robertmaynard robertmaynard force-pushed the fea/switch_thrust_and_nvcomp_over_to_patches branch from fb2c8e6 to 58ba508 Compare September 9, 2022 13:43
@robertmaynard robertmaynard removed the 5 - Merge After Dependencies Depends on another PR: do not merge out of order. label Sep 9, 2022
vyasr
vyasr requested changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

See rapidsai/cudf#11665 (review): is this urgent, or can we wait for upstream patches for thrust?

I assume that we'll need the nvcomp patch either way.

@robertmaynard
Copy link
Contributor Author

See rapidsai/cudf#11665 (review): is this urgent, or can we wait for upstream patches for thrust?

I assume that we'll need the nvcomp patch either way.

With Thrust 1.17.2 now released I will make the cub patch conditional. The nvcomp and thrust intall rule patches are still needed.

@robertmaynard
Copy link
Contributor Author

We could also merge #264 first as it is just the infrastructure bits of this PR

@robertmaynard robertmaynard force-pushed the fea/switch_thrust_and_nvcomp_over_to_patches branch 3 times, most recently from e83f47f to 2cf0fc0 Compare September 19, 2022 13:33
bdice
bdice approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have one question.

rapids-cmake/cpm/versions.json Outdated Show resolved Hide resolved
@robertmaynard
Copy link
Contributor Author

This needs to be merged after #273 so we have a version of CPM that won't warn about dirty source trees after we patch them.

@bdice
Copy link
Contributor

bdice commented Sep 19, 2022

This needs to be merged after #273 so we have a version of CPM that won't warn about dirty source trees after we patch them.

I approved and merged #273, it seemed fine to merge that directly to unblock this PR.

@robertmaynard robertmaynard force-pushed the fea/switch_thrust_and_nvcomp_over_to_patches branch from 2cf0fc0 to af8db4f Compare September 19, 2022 18:04
@robertmaynard robertmaynard requested a review from vyasr September 19, 2022 18:04
@robertmaynard
Copy link
Contributor Author

Do we want to push this to 22.12 since none of these patches are blocking 22.10? @vyasr @bdice

vyasr
vyasr reviewed Sep 27, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Apologies, I let this review slip. I am fine with pushing this if you want, but I can also iterate to get it done in the next couple of days before code freeze if you prefer. It looks to be basically done at this point.

@robertmaynard
Copy link
Contributor Author

Apologies, I let this review slip. I am fine with pushing this if you want, but I can also iterate to get it done in the next couple of days before code freeze if you prefer. It looks to be basically done at this point.

Since it isn't critical, lets push to 22.12

vyasr
vyasr approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

LGTM. We can merge early in 22.12 then.

@robertmaynard robertmaynard changed the base branch from branch-22.10 to branch-22.12 September 28, 2022 20:48
@robertmaynard robertmaynard changed the title Switch thrust and nvcomp over to use rapids-cmake patches Switch thrust over to use rapids-cmake patches Sep 28, 2022
@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4102c93 into rapidsai:branch-22.12 Oct 3, 2022
@robertmaynard robertmaynard deleted the fea/switch_thrust_and_nvcomp_over_to_patches branch October 3, 2022 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@vyasr vyasr vyasr approved these changes

Assignees
No one assigned
Labels
feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robertmaynard @bdice @vyasr