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.

Sign up for GitHub

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

Jump to bottom

Enable CEC for strings_udf #11884

Merged
merged 19 commits into from
Nov 2, 2022
Merged

Enable CEC for strings_udf #11884

merged 19 commits into from
Nov 2, 2022

Conversation

brandon-b-miller
Copy link
Contributor

This PR removes the runtime checks for CEC in strings_udf.

@github-actions github-actions bot added the Python Affects Python cuDF API. label Oct 10, 2022
@brandon-b-miller brandon-b-miller added feature request New feature or request non-breaking Non-breaking change labels Oct 10, 2022
@github-actions github-actions bot added the gpuCI label Oct 10, 2022
@codecov
Copy link

codecov bot commented Oct 10, 2022 •
edited
Loading

Codecov Report

Base: 87.47% // Head: 88.09% // Increases project coverage by +0.61% 🎉

Coverage data is based on head (41e19e4) compared to base (f817d96).
Patch has no changes to coverable lines.

❗ Current head 41e19e4 differs from pull request most recent head 610e85a. Consider uploading reports for the commit 610e85a to get more accurate results

Additional details and impacted files 
@@               Coverage Diff                @@
##           branch-22.12   #11884      +/-   ##
================================================
+ Coverage         87.47%   88.09%   +0.61%     
================================================
  Files               133      135       +2     
  Lines             21826    21984     +158     
================================================
+ Hits              19093    19367     +274     
+ Misses             2733     2617     -116
Impacted Files Coverage Δ
python/cudf/cudf/io/text.py 91.66% <0.00%> (-8.34%) ⬇️
python/strings_udf/strings_udf/__init__.py 76.47% <0.00%> (-7.85%) ⬇️
python/cudf/cudf/core/_base_index.py 81.28% <0.00%> (-4.27%) ⬇️
python/cudf/cudf/io/json.py 92.06% <0.00%> (-2.68%) ⬇️
python/cudf/cudf/utils/utils.py 89.91% <0.00%> (-0.69%) ⬇️
python/dask_cudf/dask_cudf/core.py 73.72% <0.00%> (-0.41%) ⬇️
python/cudf/cudf/io/parquet.py 90.45% <0.00%> (-0.39%) ⬇️
python/dask_cudf/dask_cudf/backends.py 84.90% <0.00%> (-0.37%) ⬇️
python/cudf/cudf/core/column/datetime.py 89.62% <0.00%> (-0.09%) ⬇️
python/cudf/cudf/io/orc.py 92.94% <0.00%> (-0.09%) ⬇️
... and 34 more

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 brandon-b-miller marked this pull request as ready for review October 13, 2022 12:57
@brandon-b-miller brandon-b-miller requested review from a team as code owners October 13, 2022 12:57
@brandon-b-miller brandon-b-miller added the 3 - Ready for Review Ready for review by team label Oct 13, 2022
brandon-b-miller
brandon-b-miller commented Oct 14, 2022
View reviewed changes
ci/gpu/build.sh Outdated Show resolved Hide resolved
vyasr
vyasr reviewed Oct 14, 2022
View reviewed changes
ci/gpu/build.sh Outdated Show resolved Hide resolved
@brandon-b-miller @vyasr
Update ci/gpu/build.sh
a912519 
Co-authored-by: Vyas Ramasubramani <[email protected]>
gmarkall
gmarkall requested changes Oct 17, 2022
View reviewed changes
Copy link
Contributor

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

This looks good - just a couple of minor points on the diff.

wence-
wence- requested changes Oct 18, 2022
View reviewed changes
python/cudf/cudf/core/udf/__init__.py Show resolved Hide resolved
python/strings_udf/strings_udf/__init__.py Outdated Show resolved Hide resolved
python/strings_udf/strings_udf/__init__.py Outdated Show resolved Hide resolved
python/strings_udf/strings_udf/__init__.py Outdated Show resolved Hide resolved
wence-
wence- requested changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Sorry, I should have been clearer first time round.

python/strings_udf/strings_udf/__init__.py Show resolved Hide resolved
python/strings_udf/strings_udf/__init__.py Outdated Show resolved Hide resolved
python/strings_udf/strings_udf/__init__.py Outdated Show resolved Hide resolved
python/strings_udf/strings_udf/__init__.py Outdated Show resolved Hide resolved
@brandon-b-miller brandon-b-miller mentioned this pull request Oct 21, 2022
Open
@github-actions github-actions bot added the conda label Oct 26, 2022
@brandon-b-miller
Copy link
Contributor Author

rerun tests

@brandon-b-miller
Copy link
Contributor Author

brandon-b-miller commented Oct 27, 2022 •
edited
Loading

@gmarkall @wence- now that the relevant ptxcompiler changes have been released I think this is ready for another look. I have added ptxcompiler >=0.7.0 in the run dependency specification for this package, meaning the installation of strings_udf should trigger an upgrade to the relevant version of ptxcompiler upon installation.

let me know if you have any further thoughts :)

@brandon-b-miller
Copy link
Contributor Author

rerun tests

@wence-
Copy link
Contributor

wence- commented Oct 31, 2022

@gmarkall @wence- now that the relevant ptxcompiler changes have been released I think this is ready for another look. I have added ptxcompiler >=0.7.0 in the run dependency specification for this package, meaning the installation of strings_udf should trigger an upgrade to the relevant version of ptxcompiler upon installation.

let me know if you have any further thoughts :)

I think some of the comments from my previous review (around unconditional initialisation of cuda contexts on import, and undefined ptxpath variable) still apply.

@brandon-b-miller
Copy link
Contributor Author

I think some of the comments from my previous review (around unconditional initialisation of cuda contexts on import, and undefined ptxpath variable) still apply.

made a few updates to this in 777e257

wence-
wence- reviewed Nov 2, 2022
View reviewed changes
python/strings_udf/strings_udf/__init__.py

def _get_ptx_file():
if "RAPIDS_NO_INITIALIZE" in os.environ:
cc = int(os.environ.get("STRINGS_UDF_CC", "52"))
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea here is that downstream consumers (like dask-cuda) that set RAPIDS_NO_INITIALIZE have a way to advertise the compute capability of the device they are running on (without creating a context). Do we care about bikeshedding this name?

@brandon-b-miller
Copy link
Contributor Author

brandon-b-miller commented Nov 2, 2022 •
edited
Loading

@gpucibot merge needs an ops review still

@brandon-b-miller
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 49fc3c7 into rapidsai:branch-22.12 Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyasr vyasr vyasr left review comments

@gmarkall gmarkall gmarkall approved these changes

@raydouglass raydouglass raydouglass approved these changes

@wence- wence- wence- approved these changes

@charlesbluca charlesbluca Awaiting requested review from charlesbluca charlesbluca is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brandon-b-miller @wence- @gmarkall @vyasr @raydouglass