Enable CEC for `strings_udf` by brandon-b-miller · Pull Request #11884 · NVIDIA/cudf

brandon-b-miller · 2022-10-10T14:10:49Z

This PR removes the runtime checks for CEC in strings_udf.

codecov · 2022-10-10T21:31:00Z

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.

Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>

gmarkall

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

gmarkall

This looks good - many thanks for the updates.

wence-

Sorry, I should have been clearer first time round.

brandon-b-miller · 2022-10-27T15:33:22Z

rerun tests

brandon-b-miller · 2022-10-27T15:52:49Z

@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 · 2022-10-27T19:26:24Z

rerun tests

wence- · 2022-10-31T16:47:18Z

@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 · 2022-11-01T14:20:46Z

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- · 2022-11-02T16:42:09Z

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


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 · 2022-11-02T19:12:59Z

~~@gpucibot merge~~ needs an ops review still

brandon-b-miller · 2022-11-02T22:05:46Z

@gpucibot merge

remove version guards

f596e93

github-actions Bot added the Python Affects Python cuDF API. label Oct 10, 2022

brandon-b-miller added feature request New feature or request non-breaking Non-breaking change labels Oct 10, 2022

brandon-b-miller added 3 commits October 10, 2022 07:15

style

a70932b

work around style conflict bug

759804d

always test string udfs

bd2219d

github-actions Bot added the gpuCI label Oct 10, 2022

brandon-b-miller added 2 commits October 12, 2022 12:24

maybe patch numba linker

1f54f39

merge and resolve conflicts, go back to just retesting udfs

b38073e

brandon-b-miller marked this pull request as ready for review October 13, 2022 12:57

brandon-b-miller requested review from a team as code owners October 13, 2022 12:57

brandon-b-miller requested review from charlesbluca, gmarkall and wence- October 13, 2022 12:57

brandon-b-miller added the 3 - Ready for Review Ready for review by team label Oct 13, 2022

gmarkall reviewed Oct 13, 2022

View reviewed changes

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

add context for maybe_patch_numba_linker

76fbf1e

brandon-b-miller commented Oct 14, 2022

View reviewed changes

Comment thread ci/gpu/build.sh Outdated

vyasr reviewed Oct 14, 2022

View reviewed changes

Comment thread ci/gpu/build.sh Outdated

Update ci/gpu/build.sh

a912519

Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>

gmarkall reviewed Oct 17, 2022

View reviewed changes

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

gmarkall reviewed Oct 17, 2022

View reviewed changes

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

gmarkall reviewed Oct 17, 2022

View reviewed changes

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

gmarkall suggested changes Oct 17, 2022

View reviewed changes

address reviews

29cf1c7

gmarkall approved these changes Oct 17, 2022

View reviewed changes

wence- requested changes Oct 18, 2022

View reviewed changes

Comment thread python/cudf/cudf/core/udf/__init__.py

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

brandon-b-miller added 4 commits October 18, 2022 08:40

half way address reviews

d725aec

address reviews

1fdc399

add comment

b56108a

refactor

049e63a

wence- requested changes Oct 19, 2022

View reviewed changes

Comment thread python/strings_udf/strings_udf/__init__.py

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

brandon-b-miller mentioned this pull request Oct 21, 2022

[Story] Support string UDFs via Numba #9639

Closed

brandon-b-miller added 2 commits October 26, 2022 10:57

Merge branch 'branch-22.12' into fea-strings-udf-cec

4ba3124

use updated ptxcompiler api

63992b1

github-actions Bot added the conda label Oct 26, 2022

brandon-b-miller requested a review from wence- October 31, 2022 14:03

wence- requested changes Oct 31, 2022

View reviewed changes

Comment thread python/strings_udf/strings_udf/__init__.py Outdated

brandon-b-miller added 2 commits November 1, 2022 07:18

fix up reviews

777e257

minor update

08f384d

wence- reviewed Nov 1, 2022

View reviewed changes

Comment thread python/cudf/cudf/core/udf/__init__.py

brandon-b-miller added 2 commits November 2, 2022 07:07

adjust logic

600b97d

add STRINGS_UDF_CC as an environment variable

610e85a

wence- reviewed Nov 2, 2022

View reviewed changes

wence- approved these changes Nov 2, 2022

View reviewed changes

raydouglass approved these changes Nov 2, 2022

View reviewed changes

rapids-bot Bot merged commit 49fc3c7 into NVIDIA:branch-22.12 Nov 2, 2022

Uh oh!

Conversation

brandon-b-miller commented Oct 10, 2022

Uh oh!

codecov Bot commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmarkall left a comment

Choose a reason for hiding this comment

Uh oh!

gmarkall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wence- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brandon-b-miller commented Oct 27, 2022

Uh oh!

brandon-b-miller commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandon-b-miller commented Oct 27, 2022

Uh oh!

Uh oh!

wence- commented Oct 31, 2022

Uh oh!

brandon-b-miller commented Nov 1, 2022

Uh oh!

Uh oh!

wence- Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

brandon-b-miller commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandon-b-miller commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Oct 10, 2022 •

edited

Loading

brandon-b-miller commented Oct 27, 2022 •

edited

Loading

brandon-b-miller commented Nov 2, 2022 •

edited

Loading