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

Remove Arrow CUDA IPC code #10995

Merged
merged 11 commits into from
Jul 21, 2022

Conversation

shwina
Copy link
Contributor

@shwina shwina commented May 27, 2022

Closes #10994.

This PR removes the Arrow CUDA-IPC related code we have, which has two benefits:

  1. It deletes code (I have confirmed that no one uses this code today)
  2. It removes our dependency on Arrow CUDA, which contributes towards removing our shared lib dependency on libcuda.so

@github-actions github-actions bot added CMake CMake build issue Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels May 27, 2022
@davidwendt
Copy link
Contributor

One more spot I think:

- test -f $PREFIX/include/cudf/ipc.hpp

@github-actions github-actions bot added the conda label May 27, 2022
@jakirkham
Copy link
Member

What is the plan with the Java code? For example

class native_arrow_ipc_reader_handle final {

@shwina
Copy link
Contributor Author

shwina commented Jun 3, 2022

@jakirkham I don't believe that uses any of the libcudf code being deleted here. It also doesn't use anything Arrow CUDA (i.e., nothing from the arrow::cuda namespace), although it does use the non-GPU IPC bits from Arrow.

@github-actions
Copy link

github-actions bot commented Jul 3, 2022

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@vyasr
Copy link
Contributor

vyasr commented Jul 12, 2022

@shwina does this also close #3661, or does that also require addressing outstanding Jitify issues still?

@shwina shwina added breaking Breaking change improvement Improvement / enhancement to an existing function labels Jul 18, 2022
@shwina
Copy link
Contributor Author

shwina commented Jul 18, 2022

@vyasr - this will not close #3661. We will either need to remove jitify as a dependency, or resolve #10654.

@vyasr
Copy link
Contributor

vyasr commented Jul 18, 2022

I think removing Jitify, while definitely feasible, will require a bit more work than I anticipated. It turns out that the rolling functionality in libcudf that uses Jitify is still used by our Python rolling code. Until that is transitioned to use numba like the rest of our apply functionality, the libcudf Jitify dependency remains (this is of course completely ignoring the question of whether we will have additional uses for Jitify in the future, which I'm sure that we will).

@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

Merging #10995 (16980b2) into branch-22.08 (5f9da83) will increase coverage by 0.05%.
The diff coverage is 73.33%.

❗ Current head 16980b2 differs from pull request most recent head 8d2e5c5. Consider uploading reports for the commit 8d2e5c5 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.08   #10995      +/-   ##
================================================
+ Coverage         86.34%   86.39%   +0.05%     
================================================
  Files               144      143       -1     
  Lines             22811    22742      -69     
================================================
- Hits              19696    19648      -48     
+ Misses             3115     3094      -21     
Impacted Files Coverage Δ
python/cudf/cudf/_lib/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/core/groupby/groupby.py 91.02% <69.23%> (-0.42%) ⬇️
python/cudf/cudf/core/column/numerical.py 96.19% <100.00%> (+0.31%) ⬆️
python/cudf/cudf/core/dataframe.py 93.57% <0.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/string.py 88.80% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 91.70% <0.00%> (+0.97%) ⬆️
python/cudf/cudf/api/types.py 90.42% <0.00%> (+1.06%) ⬆️

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 5f9da83...8d2e5c5. Read the comment docs.

@shwina shwina marked this pull request as ready for review July 19, 2022 15:57
@shwina shwina requested review from a team as code owners July 19, 2022 15:57
@shwina shwina requested a review from trxcllnt July 19, 2022 15:57
@shwina shwina requested review from mroeschke and bdice July 19, 2022 15:57
@shwina shwina changed the title [DRAFT] Remove Arrow CUDA IPC code Remove Arrow CUDA IPC code Jul 19, 2022
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

The cmake changes look correct to me

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.

All the changes look fine to me.

@bdice
Copy link
Contributor

bdice commented Jul 19, 2022

@shwina Does this mean we no longer need to pin the CUDA build of Arrow in the cudf conda environment?

- pyarrow=8.0.0=*cuda

conda/recipes/cudf/meta.yaml Outdated Show resolved Hide resolved
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner changes. Also opened the accompanying PR below.

@shwina
Copy link
Contributor Author

shwina commented Jul 21, 2022

rerun tests

@jakirkham
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 3ea0638 into rapidsai:branch-22.08 Jul 21, 2022
@jakirkham
Copy link
Member

Thanks Ashwin for the PR and everyone for the reviews! 🙏

rapids-bot bot pushed a commit that referenced this pull request Aug 1, 2022
… cudf recipe (#11412)

Arrow 8.0.1 just got built and uploaded to conda-forge and there's an associated PR to update the pinning to 8.0.1. Currently the recipes pin to 8.0.0 explicitly which will cause solving issues. Instead of just changing to 8.0.1, this PR relaxes the pinning to allow minor and patch versions to get pulled as there hasn't been any nvcc compiler issues in a while and Arrow strives to not introduce API / ABI breakages in minor and patch versions. Additionally, the cudf recipe seems to be unnecessarily setting requiring a cuda build of pyarrow which should no longer be needed after #10995.

Would really like this in for 21.08 if possible, but happy to change to just pin 8.0.1 if preferred.

Authors:
  - Keith Kraus (https://github.com/kkraus14)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: #11412
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DISCUSSION] Remove the Arrow CUDA IPC related code from libcudf/cuDF
8 participants