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

Update rapids-cmake packages to Thrust 1.15 #138

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Update rapids-cmake packages to Thrust 1.15 #138

merged 1 commit into from
Jan 11, 2022

Conversation

robertmaynard
Copy link
Contributor

Starting with 22.02, all of RAPIDS is using Thrust 1.15

@robertmaynard
Update rapids-cmake packages to Thrust 1.15
25cc245 
Starting with 22.02, all of RAPIDS is using Thrust 1.15
@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change 2 - In Progress Currenty a work in progress labels Dec 14, 2021
@robertmaynard
Copy link
Contributor Author

robertmaynard commented Dec 14, 2021 •
edited
Loading

Need to manually verify the following projects cleanly build with 1.15 before we can merge

@ChuckHastings ChuckHastings mentioned this pull request Jan 4, 2022
Merged
2 tasks
@robertmaynard
Copy link
Contributor Author

rerun tests

@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@robertmaynard robertmaynard added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Jan 11, 2022
@robertmaynard robertmaynard marked this pull request as ready for review January 11, 2022 13:07
@rapids-bot rapids-bot bot merged commit 7cdeb62 into rapidsai:branch-22.02 Jan 11, 2022
@robertmaynard robertmaynard deleted the fea/update_thrust_ver branch January 11, 2022 13:08
@bdice bdice mentioned this pull request Apr 4, 2022
Merged
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Apr 5, 2022
@bdice
Revert Thrust 1.16 to Thrust 1.15 (#10586)
090f6b8 
PR #10489 updated from Thrust 1.15 to Thrust 1.16. However, this appears to be causing conflicts with other repositories -- [cuSpatial](rapidsai/cuspatial#511 (comment)) and cuGraph have reported issues where their builds are finding Thrust 1.16 from libcudf instead of Thrust 1.15 which is [currently pinned by rapids-cmake](https://github.com/rapidsai/rapids-cmake/blob/06a657281cdd83781e49afcdbb39abc491eeab17/rapids-cmake/cpm/versions.json#L26).

This PR is intended to unblock local builds and CI builds for other RAPIDS packages until we are able to identify the root cause (which may be due to CMake include path orderingsrapids-cmake).

Last time Thrust was updated, [rapids-cmake was updated](rapidsai/rapids-cmake#138) one day before [libcudf was updated](#9912). That may explain why we didn't notice this problem with the 1.15 update.

The plan I currently have in mind is:

1. Merge this PR to roll back libcudf to Thrust 1.15 (and revert the patch for Thrust 1.16 [10577](#10577)). This will hopefully unblock CI for cugraph and cuspatial.
2. Try to work out whatever issues with CMake / include paths may exist.
3. Prepare all rapids-cmake repos for Thrust 1.16 compatibility. I've [done this for RMM already](rapidsai/rmm#1011), and I am working on [PR 4675](rapidsai/cuml#4675) to cuML now. I am planning to make the same fixes for `#include`s in cuCollections, raft, cuSpatial, and cuGraph so they will be compatible with Thrust 1.16.
4. Try to upgrade libcudf to Thrust 1.16 again (and re-apply the updated patch). If (2) has been resolved, I hope we won't see any issues in other RAPIDS libraries
5. Upgrade rapids-cmake to Thrust 1.16.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Mark Harris (https://github.com/harrism)

URL: #10586
abellina pushed a commit to abellina/cudf that referenced this pull request Apr 11, 2022
@bdice @abellina
Revert Thrust 1.16 to Thrust 1.15 (rapidsai#10586)
87f0798 
PR rapidsai#10489 updated from Thrust 1.15 to Thrust 1.16. However, this appears to be causing conflicts with other repositories -- [cuSpatial](rapidsai/cuspatial#511 (comment)) and cuGraph have reported issues where their builds are finding Thrust 1.16 from libcudf instead of Thrust 1.15 which is [currently pinned by rapids-cmake](https://github.com/rapidsai/rapids-cmake/blob/06a657281cdd83781e49afcdbb39abc491eeab17/rapids-cmake/cpm/versions.json#L26).

This PR is intended to unblock local builds and CI builds for other RAPIDS packages until we are able to identify the root cause (which may be due to CMake include path orderingsrapids-cmake).

Last time Thrust was updated, [rapids-cmake was updated](rapidsai/rapids-cmake#138) one day before [libcudf was updated](rapidsai#9912). That may explain why we didn't notice this problem with the 1.15 update.

The plan I currently have in mind is:

1. Merge this PR to roll back libcudf to Thrust 1.15 (and revert the patch for Thrust 1.16 [10577](rapidsai#10577)). This will hopefully unblock CI for cugraph and cuspatial.
2. Try to work out whatever issues with CMake / include paths may exist.
3. Prepare all rapids-cmake repos for Thrust 1.16 compatibility. I've [done this for RMM already](rapidsai/rmm#1011), and I am working on [PR 4675](rapidsai/cuml#4675) to cuML now. I am planning to make the same fixes for `#include`s in cuCollections, raft, cuSpatial, and cuGraph so they will be compatible with Thrust 1.16.
4. Try to upgrade libcudf to Thrust 1.16 again (and re-apply the updated patch). If (2) has been resolved, I hope we won't see any issues in other RAPIDS libraries
5. Upgrade rapids-cmake to Thrust 1.16.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Mark Harris (https://github.com/harrism)

URL: rapidsai#10586
@gnaggnoyil
Copy link

AFAIK thrust1.15 will not report an exact version match when executing find_package, if the version string passed to find_package is not exactly the string "1.15.0.0" instead of a simple "1.15.0". However the current version.json specifies thrust version as "1.15.0". Therefore when CPMFindPackage tries to find a local thrust package of version 1.15 exactly through find_package, the find process will fail even if there is a thrust 1.15 package in the find location.

I think the above thrust issue has been fixed since thrust 1.16, according to this PR. By far I think a workaroud that replaces "version" : "1.15.0", to "version" : "1.15.0.0", would be enough. @robertmaynard do you have any thoughts?

@robertmaynard
Copy link
Contributor Author

AFAIK thrust1.15 will not report an exact version match when executing find_package, if the version string passed to find_package is not exactly the string "1.15.0.0" instead of a simple "1.15.0". However the current version.json specifies thrust version as "1.15.0". Therefore when CPMFindPackage tries to find a local thrust package of version 1.15 exactly through find_package, the find process will fail even if there is a thrust 1.15 package in the find location.

I think the above thrust issue has been fixed since thrust 1.16, according to this PR. By far I think a workaroud that replaces "version" : "1.15.0", to "version" : "1.15.0.0", would be enough. @robertmaynard do you have any thoughts?

Yes that is an issue, see PR #180 which fixes it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team 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.
2 participants
@robertmaynard @gnaggnoyil