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

Migrate string capitalize APIs to pylibcudf #15503

Merged

Conversation

brandon-b-miller
Copy link
Contributor

@brandon-b-miller brandon-b-miller commented Apr 10, 2024

This PR creates the pylibcudf.strings.capitalize namespace and migrates the cuDF cython to use it. Depends on #15489

Part of #15162

@brandon-b-miller brandon-b-miller added feature request New feature or request Python Affects Python cuDF API. non-breaking Non-breaking change labels Apr 10, 2024
@brandon-b-miller brandon-b-miller self-assigned this Apr 10, 2024
@github-actions github-actions bot added the CMake CMake build issue label Apr 10, 2024
@brandon-b-miller brandon-b-miller marked this pull request as ready for review May 22, 2024 22:39
@brandon-b-miller brandon-b-miller requested a review from a team as a code owner May 22, 2024 22:39
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Couple of small things, but this is nearly good to go.

python/cudf/cudf/_lib/pylibcudf/strings/CMakeLists.txt Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/pylibcudf/strings/capitalize.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/pylibcudf_tests/test_string_capitalize.py Outdated Show resolved Hide resolved
python/cudf/cudf/pylibcudf_tests/test_string_capitalize.py Outdated Show resolved Hide resolved
@brandon-b-miller brandon-b-miller changed the base branch from branch-24.06 to branch-24.08 May 28, 2024 14:53
@brandon-b-miller brandon-b-miller requested a review from a team as a code owner May 28, 2024 14:53
@brandon-b-miller brandon-b-miller requested review from a team as code owners May 28, 2024 14:53
@brandon-b-miller brandon-b-miller requested review from msarahan, harrism and shrshi and removed request for msarahan, harrism and shrshi May 28, 2024 14:53
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

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

Approved trivial CMake changes

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Couple of small comments, but nothing that really requires a change. Everything looks good!

@@ -15,6 +15,7 @@
set(cython_sources aggregation.pyx binaryop.pyx copying.pyx replace.pyx reduce.pxd
stream_compaction.pyx types.pyx unary.pyx
)
add_subdirectory(strings)
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with other files I'd suggest putting this after the rapids_cython_create_modules call, but it doesn't really matter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine leaving this as-is, but if the entire module is just the enum I sort of wonder if we shouldn't just lift it up directly into the __init__.py/pxd 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.

For me the main justification for leaving it is that if I'd usually #include a libcudf header to make use of some API or object, I think I should expect to import or cimport it from the corresponding pylibcudf "namespace".

@vyasr vyasr added the pylibcudf Issues specific to the pylibcudf package label May 28, 2024
@brandon-b-miller
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit bdafa73 into rapidsai:branch-24.08 May 29, 2024
70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue feature request New feature or request non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants