Add regex_program class for use with all regex APIs by davidwendt · Pull Request #11927 · NVIDIA/cudf

davidwendt · 2022-10-14T21:16:18Z

Description

Adds a new regex_program class to encapsulate a regex pattern and parameters used for executing regex calls on strings columns in libcudf. This provides a single object to hold the regex settings rather than adding or updating parameters to every call. Given a pattern (and other settings), it will compile and validate the pattern and build the set of instructions/commands needed to execute the regex on a strings column. Converting the pattern is done in CPU code. The object contains no state data and can be reused on the same API or other similar calls as appropriate (per the settings).
The object can also be queried to help with resource allocation/expectations.

The main files to review are the new regex_program* source files plus the corresponding changes in regexec.cpp (renamed from .cu). The remainder are simply side-effects and have common patterns to use the new object.
No function or behavior has changed but rather an new interface has been added over existing function but additional tests have been added to exercise through the companion APIs.

Currently, all regex APIs are duplicated -- the original API plus a new one accepting a regex_progam object. Once accepted we may consider deprecating the non-object APIs and then removing them in a future release.

This will help with changes needed for #10852

Checklist

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

codecov · 2022-10-14T23:41:50Z

Codecov Report

Base: 87.47% // Head: 88.07% // Increases project coverage by +0.59% 🎉

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

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-22.12   #11927      +/-   ##
================================================
+ Coverage         87.47%   88.07%   +0.59%     
================================================
  Files               133      135       +2     
  Lines             21826    22025     +199     
================================================
+ Hits              19093    19398     +305     
+ Misses             2733     2627     -106

Impacted Files	Coverage Δ
python/cudf/cudf/core/column/interval.py	`85.45% <0.00%> (-9.10%)`	⬇️
python/cudf/cudf/io/text.py	`91.66% <0.00%> (-8.34%)`	⬇️
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/cudf/cudf/core/column/timedelta.py	`90.17% <0.00%> (-0.58%)`	⬇️
python/cudf/cudf/core/column/datetime.py	`89.21% <0.00%> (-0.51%)`	⬇️
python/cudf/cudf/core/column/column.py	`87.96% <0.00%> (-0.46%)`	⬇️
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%)`	⬇️
... and 39 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.

robertmaynard

CMake changes LGTM

vyasr

I like the refactor. I mostly have questions about the current encapsulation of the program impl class.

Also, should we be concerned about overloading the "prog" name given that we already use this on device for the reprog_device class? Are the two similar enough that it makes sense to have similar names? Should we consider an alternative?

vyasr · 2022-11-02T23:42:43Z

 namespace cudf {
 namespace strings {
+
+struct regex_program;


We may want to include a note in the developer guide about our preferred uses of forward declarations of includes (if you can come up with a pithy summary).

vyasr

LGTM

bdice

Looks great! Encapsulating information in the regex_program will help keep the public APIs constrained while adding new features that can be controlled via the program.

davidwendt · 2022-11-09T18:52:19Z

@gpucibot merge

jlowe · 2022-11-10T16:47:02Z

FYI, surprisingly this PR has broken a large number of RAPIDS Accelerator tests related to regex. Tests pass before this change and fail afterwards. In at least one of the tests, the nature of the failure appears to be solely related to the data column. There are null characters (or uninitialized memory?) appearing in blocks of rows periodically in the output. I'll try to narrow this down to a repro case in C++.

davidwendt · 2022-11-10T16:51:14Z

I just noticed this too. I'm working on a fix.

Fixes error in `working_memory_size()` member function passing the parameters incorrectly. This was introduce in #11927 and found in the nightly compute-sanitizer check. https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci/job/cudf/job/branches/job/cudf-gpu-build-branch-22.12/19/CUDA=11.5/testReport/junit/cudamemcheck/STRINGS_TEST/StringsContainsTests_ContainsTest/ Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Jason Lowe (https://github.com/jlowe) - Yunsong Wang (https://github.com/PointKernel) - Nghia Truong (https://github.com/ttnghia) - Vukasin Milovanovic (https://github.com/vuule) URL: #12119

Deprecates the libcudf regex APIs that do not accept `cudf::strings::regex_program` objects. The libcudf regex functions were converted to accept `regex_program` objects instead of a pattern string and regex-flags directly in PR #11927 Most of this is removing calls to the deprecated functions in the gtests. The declarations in the headers had to be reordered to make the reference links work in the doxygen output. These deprecated functions will be removed in a future release. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Vukasin Milovanovic (https://github.com/vuule) - Nghia Truong (https://github.com/ttnghia) URL: #12891

Add regex_program class for use with all regex APIs

0b0b1f6

davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python) improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 14, 2022

davidwendt self-assigned this Oct 14, 2022

github-actions Bot added the CMake CMake build issue label Oct 14, 2022

fix missing doxygen

f6c8b6b

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

davidwendt added 11 commits October 17, 2022 08:24

Merge branch 'branch-22.12' into regex-program-class

c6a9b14

add regex_program dtor decl/def

87783ac

fix doxygen for compute-working-memory-size function

1ebfc3b

add gtests with regex_program

52d86fd

Merge branch 'branch-22.12' into regex-program-class

885c51e

add gtests for split_re functions

bd9cd9c

Merge branch 'branch-22.12' into regex-program-class

999c622

fix merge conflict

c9cce60

Merge branch 'branch-22.12' into regex-program-class

9f7ba6e

Merge branch 'branch-22.12' into regex-program-class

aa3e021

update doxygen for future work

8c904f6

davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Oct 25, 2022

davidwendt marked this pull request as ready for review October 25, 2022 19:01

davidwendt requested review from a team as code owners October 25, 2022 19:01

davidwendt requested review from harrism and vyasr October 25, 2022 19:01

robertmaynard approved these changes Oct 25, 2022

View reviewed changes

davidwendt added 7 commits October 26, 2022 09:21

Merge branch 'branch-22.12' into regex-program-class

7b1457f

fix merge conflict

3587ee5

Merge branch 'branch-22.12' into regex-program-class

6fb423c

Merge branch 'branch-22.12' into regex-program-class

0d8aaaa

Merge branch 'branch-22.12' into regex-program-class

1783af9

Merge branch 'branch-22.12' into regex-program-class

93d3ba2

Merge branch 'branch-22.12' into regex-program-class

9759277

vyasr requested changes Nov 3, 2022

View reviewed changes

davidwendt added 2 commits November 2, 2022 21:12

delete def ctor; fix parameter order

915df87

replace get_impl() with friend access class

c4c686b

davidwendt requested a review from vyasr November 3, 2022 12:50

davidwendt added 2 commits November 3, 2022 08:52

Merge branch 'branch-22.12' into regex-program-class

6324240

add ctors for the impl class

f85897f

vyasr approved these changes Nov 3, 2022

View reviewed changes

davidwendt added 2 commits November 4, 2022 12:49

Merge branch 'branch-22.12' into regex-program-class

7421135

fix merge conflicts

e90c4d0

bdice approved these changes Nov 7, 2022

View reviewed changes

Comment thread cpp/include/cudf/strings/regex/regex_program.hpp Outdated

Comment thread cpp/include/cudf/strings/regex/regex_program.hpp Outdated

fix wording in doxygen comment for regex_program

d2d997a

raydouglass approved these changes Nov 7, 2022

View reviewed changes

rapids-bot Bot merged commit 74053f4 into NVIDIA:branch-22.12 Nov 9, 2022

davidwendt deleted the regex-program-class branch November 9, 2022 18:52

jlowe mentioned this pull request Nov 10, 2022

[BUG] regexp_test and many other test failures NVIDIA/cudf-spark#7041

Closed

davidwendt mentioned this pull request Nov 10, 2022

Fix regex working-memory-size refactor error #12119

Merged

3 tasks

davidwendt mentioned this pull request Mar 7, 2023

Deprecate libcudf regex APIs accepting pattern strings directly #12891

Merged

3 tasks

Uh oh!

Conversation

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

Uh oh!

Description

Checklist

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

robertmaynard left a comment

Choose a reason for hiding this comment

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

vyasr Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

bdice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidwendt commented Nov 9, 2022

Uh oh!

jlowe commented Nov 10, 2022

Uh oh!

davidwendt commented Nov 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davidwendt commented Oct 14, 2022 •

edited

Loading

codecov Bot commented Oct 14, 2022 •

edited

Loading