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

Remove cudf._lib.nvtext in favor of inlining pylibcudf #17535

Merged
merged 11 commits into from
Dec 13, 2024

Conversation

mroeschke
Copy link
Contributor

Description

Contributes to #17317

Checklist

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

@mroeschke mroeschke added Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Dec 6, 2024
@mroeschke mroeschke self-assigned this Dec 6, 2024
@mroeschke mroeschke requested a review from a team as a code owner December 6, 2024 00:37
@mroeschke mroeschke requested review from wence- and Matt711 December 6, 2024 00:37
@github-actions github-actions bot added the CMake CMake build issue label Dec 6, 2024
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.

One general thought for future improvements, but overall looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's worth capturing the general pattern of:

return type(self).from_pylibcudf(
    getattr(plc, func)(
        self.to_pylibcudf(mode="read"),
        *args,
        **kwargs,
    )
)

into a helper function is worthwhile. In addition to reducing boilerplate, it could also be useful to inject standard bits error handling or other pylibcudf wrapping that we always want in place. It may be harder to placate mypy with that kind of code though.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the long run I'd envision this function also being able to handle things like _with_type_metadata, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the to/from_pylibcudf pattern is pretty common. I think it may be worth capturing in a helper function, but it may also depend on how much I can make the current cudf Column just be a pylibcudf Column directly

@mroeschke
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 8a3e5f1 into rapidsai:branch-25.02 Dec 13, 2024
105 checks passed
@mroeschke mroeschke deleted the cudf/_lib/nvtext branch December 13, 2024 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants