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

Revert Thrust 1.16 to Thrust 1.15 #10586

Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Apr 4, 2022

PR #10489 updated from Thrust 1.15 to Thrust 1.16. However, this appears to be causing conflicts with other repositories -- cuSpatial 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.

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 one day before libcudf was updated. 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). 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, and I am working on PR 4675 to cuML now. I am planning to make the same fixes for #includes 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.

@bdice bdice requested a review from a team as a code owner April 4, 2022 21:02
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Apr 4, 2022
@vyasr vyasr added bug Something isn't working ! - Hotfix Hotfix is a bug that affects the majority of users for which there is no reasonable workaround non-breaking Non-breaking change labels Apr 4, 2022
@bdice bdice self-assigned this Apr 4, 2022
@bdice bdice added this to PR-WIP in v22.06 Release via automation Apr 4, 2022
@galipremsagar
Copy link
Contributor

Since this is for 22.06, this won't need a patch to be released yet right @vyasr | @bdice?

@bdice
Copy link
Contributor Author

bdice commented Apr 4, 2022

@galipremsagar Correct. This only affects 22.06.

@galipremsagar galipremsagar removed the ! - Hotfix Hotfix is a bug that affects the majority of users for which there is no reasonable workaround label Apr 4, 2022
@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #10586 (d281550) into branch-22.06 (291fbcf) will increase coverage by 0.00%.
The diff coverage is 95.83%.

❗ Current head d281550 differs from pull request most recent head 3c8c503. Consider uploading reports for the commit 3c8c503 to get more accurate results

@@              Coverage Diff              @@
##           branch-22.06   #10586   +/-   ##
=============================================
  Coverage         86.31%   86.32%           
=============================================
  Files               140      140           
  Lines             22300    22292    -8     
=============================================
- Hits              19249    19243    -6     
+ Misses             3051     3049    -2     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/numerical.py 96.17% <ø> (ø)
python/cudf/cudf/testing/testing.py 81.69% <ø> (-2.82%) ⬇️
python/cudf/cudf/core/index.py 92.31% <90.90%> (+0.05%) ⬆️
python/cudf/cudf/core/single_column_frame.py 96.45% <93.75%> (-0.40%) ⬇️
python/cudf/cudf/core/column/categorical.py 89.77% <100.00%> (ø)
python/cudf/cudf/core/column/column.py 89.34% <100.00%> (+0.03%) ⬆️
python/cudf/cudf/core/column/datetime.py 89.31% <100.00%> (ø)
python/cudf/cudf/core/column/numerical_base.py 98.90% <100.00%> (+0.01%) ⬆️
python/cudf/cudf/core/column/string.py 89.10% <100.00%> (+0.12%) ⬆️
python/cudf/cudf/core/column/struct.py 96.42% <100.00%> (-0.09%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff1ff80...3c8c503. Read the comment docs.

@bdice
Copy link
Contributor Author

bdice commented Apr 5, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 090f6b8 into rapidsai:branch-22.06 Apr 5, 2022
v22.06 Release automation moved this from PR-WIP to Done Apr 5, 2022
abellina pushed a commit to abellina/cudf that referenced this pull request Apr 11, 2022
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
@bdice bdice mentioned this pull request Aug 2, 2022
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Aug 11, 2022
Updates the bundled version of Thrust to 1.17.0. I will run benchmarks and include results in a comment below.

Depends on #11457.

Supersedes #10489, #10577, #10586. Closes #10841. **This should be merged concurrently with rapidsai/rapids-cmake#231

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

Approvers:
  - David Wendt (https://github.com/davidwendt)
  - Nghia Truong (https://github.com/ttnghia)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #11437
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants