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

Allow falling back to shim_60.ptx by default in strings_udf #12056

Conversation

brandon-b-miller
Copy link
Contributor

In the context of distributed, strings_udf needs to import and set itself up without creating a CUDA context, as this can interfere with up the way the network is being set up. In this situation it can't use it's normal mechanism (which requires a context) to query the compute capability of the device, and it falls back on an environment variable STRINGS_UDF_CC that it needs to be passed from dask instead. A user can set this and their code will work no problem, but we also need some default configuration that just works when someone builds their code. Without knowing their setup beforehand this can be problematic, as such I originally added the default value of cc=52 when the environment variable isn't set. This was however not exactly correct for a few reasons:

  • It should be 60 I think since pascal is the oldest arch supported by rapids
  • we don't always build shim_60.ptx especially in local mode.

This PR fixes this problem.

@brandon-b-miller brandon-b-miller added bug Something isn't working 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. non-breaking Non-breaking change labels Nov 3, 2022
@brandon-b-miller brandon-b-miller requested a review from a team as a code owner November 3, 2022 14:13
@brandon-b-miller brandon-b-miller self-assigned this Nov 3, 2022
@github-actions github-actions bot added the CMake CMake build issue label Nov 3, 2022
@brandon-b-miller
Copy link
Contributor Author

rerun tests

@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Base: 88.06% // Head: 88.09% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (f761878) compared to base (e402448).
Patch coverage: 92.85% of modified lines in pull request are covered.

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

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-22.12   #12056      +/-   ##
================================================
+ Coverage         88.06%   88.09%   +0.03%     
================================================
  Files               135      135              
  Lines             21984    21997      +13     
================================================
+ Hits              19360    19379      +19     
+ Misses             2624     2618       -6     
Impacted Files Coverage Δ
python/strings_udf/strings_udf/__init__.py 76.47% <0.00%> (ø)
python/cudf/cudf/core/column/struct.py 96.47% <100.00%> (+0.41%) ⬆️
python/cudf/cudf/core/dtypes.py 96.67% <100.00%> (+0.03%) ⬆️
python/cudf/cudf/core/dataframe.py 93.67% <0.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/string.py 88.65% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.51% <0.00%> (+0.20%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 93.75% <0.00%> (+0.96%) ⬆️

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.

@brandon-b-miller
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1d6931a into rapidsai:branch-22.12 Nov 4, 2022
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 bug Something isn't working CMake CMake build issue non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants