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

Enable using upstream jitify2 #11287

Merged
merged 22 commits into from
Aug 25, 2022

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Jul 18, 2022

This PR enables using upstream jitify2 rather than RAPIDS' fork of jitify2.

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.


Two major issues came up when making the switch:

  1. Cannot use <limits> and <cuda/std/limits> in the same source file 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.

@github-actions github-actions bot added CMake CMake build issue Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Jul 18, 2022
@shwina shwina added the non-breaking Non-breaking change label Jul 18, 2022
@github-actions github-actions bot removed the Python Affects Python cuDF API. label Jul 18, 2022
@shwina shwina added Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function labels Jul 18, 2022
@github-actions github-actions bot removed the Python Affects Python cuDF API. label Jul 20, 2022
@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@f220e90). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 97bc0ef differs from pull request most recent head ad4b613. Consider uploading reports for the commit ad4b613 to get more accurate results

@@               Coverage Diff               @@
##             branch-22.10   #11287   +/-   ##
===============================================
  Coverage                ?   86.40%           
===============================================
  Files                   ?      145           
  Lines                   ?    22959           
  Branches                ?        0           
===============================================
  Hits                    ?    19838           
  Misses                  ?     3121           
  Partials                ?        0           

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

@shwina shwina changed the base branch from branch-22.08 to branch-22.10 August 22, 2022 19:17
@shwina shwina marked this pull request as ready for review August 22, 2022 19:40
@shwina shwina requested review from a team as code owners August 22, 2022 19:40
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.

Minor requests but nothing blocking. I am not really a fan of the two workarounds required here, but the ability to remove the libcuda.so dependence is worth accepting these workarounds.

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
GIT_REPOSITORY https://github.com/rapidsai/jitify.git
GIT_TAG cudf_0.19
GIT_REPOSITORY https://github.com/NVIDIA/jitify.git
GIT_TAG jitify2
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we ask the Jitify team to make a tag so that we're not pinned to a "floating" branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that'd be great

cpp/CMakeLists.txt Show resolved Hide resolved
cpp/include/cudf/fixed_point/fixed_point.hpp Show resolved Hide resolved
@@ -22,6 +22,7 @@

// Note: The <cuda/std/*> versions are used in order for Jitify to work with our fixed_point type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be copied to the other files where these includes occur?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know -- this comment precedes this PR.

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@shwina
Copy link
Contributor Author

shwina commented Aug 25, 2022

@gpucibot merge

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.

4 participants