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 use of CUDA driver API calls from libcudf #11370

Merged
merged 29 commits into from
Aug 30, 2022

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Jul 27, 2022

Description

This PR adds the final bits that remove libcuda.so as a shared library dependency of libcudf. Currently, that's just one place that uses the driver API cuContextGetCurrent() to get the current CUDA context and maintain a per-context singleton. It was determined in the discussions within this PR that that could be replaced with a per-device singleton and a call to the runtime API cudaGetDevice() instead.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Jul 27, 2022
@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@d241458). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.10   #11370   +/-   ##
===============================================
  Coverage                ?   86.41%           
===============================================
  Files                   ?      145           
  Lines                   ?    22993           
  Branches                ?        0           
===============================================
  Hits                    ?    19870           
  Misses                  ?     3123           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rapids-bot bot pushed a commit that referenced this pull request Aug 25, 2022
This PR enables using [upstream jitify2](https://github.com/NVIDIA/jitify/tree/jitify2) rather than RAPIDS' fork of [jitify2](https://github.com/rapidsai/jitify/tree/cudf_0.19). 

This enables us to take advantage of the latest additions/improvements to jitify. Most notably: upstream jitify2 dlsym/dlopens `libcuda.so` which enables us to [drop our shared library dependency on `libcuda.so`](#11370).

---

Two major issues came up when making the switch:

1. NVIDIA/jitify#107 - I used the workaround mentioned in that issue. Hopefully it is fixed soon and we can eliminate the workaround.
2. We need to pass `-D_FILE_OFFSET_BITS=64` to jitify. Due to limitations in the way conda-forge builds glibc, we must explicitly state we require 64bit file offset support.

Authors:
  - Ashwin Srinath (https://github.com/shwina)

Approvers:
  - Yunsong Wang (https://github.com/PointKernel)
  - Bradley Dice (https://github.com/bdice)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #11287
@shwina shwina added improvement Improvement / enhancement to an existing function and removed inactive-30d labels Aug 29, 2022
@shwina shwina changed the title Remove libcuda.so shared library dependency dlopen rather than link to libcuda.so Aug 29, 2022
@shwina shwina marked this pull request as ready for review August 29, 2022 13:50
@shwina shwina requested review from a team as code owners August 29, 2022 13:50
@shwina shwina requested review from bdice and mythrocks August 29, 2022 13:50
@shwina shwina added the non-breaking Non-breaking change label Aug 29, 2022
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.

I'm fine with merging this PR for now and switching to cudaGetDevice in a later PR.

@shwina shwina changed the title dlopen rather than link to libcuda.so Remove use of CUDA driver API calls from libcudf Aug 30, 2022
@shwina
Copy link
Contributor Author

shwina commented Aug 30, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a7b2e0c into rapidsai:branch-22.10 Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants