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

Use CubinLinker for CUDA Minor Version Compatibility #11701

Merged
merged 14 commits into from
Sep 29, 2022

Conversation

gmarkall
Copy link
Contributor

@gmarkall gmarkall commented Sep 14, 2022

Description

This switches to using CubinLinker (from PTXCompiler, but CubinLinker uses PTXCompiler internally) for Minor Version Compatibility. This enables support for all Numba features except linking archives with MVC, in support of use cases such as String UDFs (#11319) with MVC.

Checklist

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

@gmarkall gmarkall requested review from a team as code owners September 14, 2022 10:42
@github-actions github-actions bot added conda Python Affects Python cuDF API. labels Sep 14, 2022
@gmarkall gmarkall added 2 - In Progress Currently a work in progress Python Affects Python cuDF API. conda improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed Python Affects Python cuDF API. conda labels Sep 14, 2022
@@ -7,6 +7,7 @@ channels:
- rapidsai-nightly
- dask/label/dev
- conda-forge
- gmarkall
Copy link
Contributor

@galipremsagar galipremsagar Sep 14, 2022

Choose a reason for hiding this comment

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

For the CI conda build to pick-up from your channel, you will need to have the channel added into build.sh files in the similar way I did in my PR here: https://github.com/rapidsai/cudf/pull/11617/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've just seen this whilst I was making a change with a similar aim. I will have a look at the way you did it too.

@gmarkall gmarkall requested a review from a team as a code owner September 14, 2022 11:04
@github-actions github-actions bot added the gpuCI label Sep 14, 2022
@gmarkall gmarkall added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Sep 14, 2022
@galipremsagar
Copy link
Contributor

CI errors related to :dask/dask#9490

@codecov
Copy link

codecov bot commented Sep 27, 2022

Codecov Report

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

❗ Current head 24918f7 differs from pull request most recent head c3c1973. Consider uploading reports for the commit c3c1973 to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.10   #11701   +/-   ##
===============================================
  Coverage                ?   86.56%           
===============================================
  Files                   ?      133           
  Lines                   ?    21781           
  Branches                ?        0           
===============================================
  Hits                    ?    18855           
  Misses                  ?     2926           
  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.

…untime-checks

Remove runtime checks for CUDA versions from strings_udf
@gmarkall gmarkall removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Sep 28, 2022
@gmarkall
Copy link
Contributor Author

Now that all the required packages are available in the usual channels (Numba 0.56.2 from conda-forge and CubinLinker from rapidsai) I've removed the "DO NOT MERGE" label. Additionally I've added some changes from Brandon that remove the checks on the driver / runtime versions for enabling string UDFs, as the changes in this branch mean that those checks should no longer be necessary.

Assuming the tests all pass this should be good for a review / merge if the review is OK.

@gmarkall gmarkall added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Sep 28, 2022
@galipremsagar
Copy link
Contributor

I opened an integration repo PR: rapidsai/integration#535

ci/gpu/build.sh Outdated
Comment on lines 273 to 276
# retest cudf with strings_udf present
py.test -n 8 --cache-clear --basetemp="$WORKSPACE/strings-udf-cuda-tmp" --junitxml="$WORKSPACE/junit-strings-udf.xml" -v --cov-config=.coveragerc --cov=strings_udf --cov-report=xml:"$WORKSPACE/python/strings_udf/strings-udf-coverage.xml" --cov-report term tests
gpuci_logger "Python py.test retest cuDF UDFs"
py.test tests/test_udf_masked_ops.py -n 8 --cache-clear
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right? Don't we need to cd $WORKSPACE/python/cudf/cudf between these two lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

you're right. Looks like I was a little overzealous with my deletions here @gmarkall , sorry about that!

@raydouglass raydouglass merged commit 2041caa into rapidsai:branch-22.10 Sep 29, 2022
@jakirkham
Copy link
Member

Thanks all! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants