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

Clean up misc, unneeded pylibcudf.libcudf in cudf._lib #17309

Merged
merged 3 commits into from
Nov 13, 2024
Merged

Clean up misc, unneeded pylibcudf.libcudf in cudf._lib #17309

merged 3 commits into from
Nov 13, 2024

Conversation

mroeschke
Copy link
Contributor

Description

  • Removed ctypedef const scalar constscalar usage
  • Use dtype_to_pylibcudf_type where appropriate
  • Use pylibcudf enums instead of pylibcudf.libcudf types

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 improvement Improvement / enhancement to an existing function non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package labels Nov 13, 2024
@mroeschke mroeschke self-assigned this Nov 13, 2024
@mroeschke mroeschke requested a review from a team as a code owner November 13, 2024 03:12
@mroeschke mroeschke requested review from bdice and Matt711 November 13, 2024 03:12
@github-actions github-actions bot added the Python Affects Python cuDF API. label Nov 13, 2024
Matt711
Matt711 approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@Matt711 Matt711 left a comment

Choose a reason for hiding this comment

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

Non blocking question

python/pylibcudf/pylibcudf/io/types.pyx
Comment on lines +185 to +186
# TODO: Keep the sources alive (self.byte_sources = sources)
# for str data (e.g. read_json)?
Copy link
Contributor

Choose a reason for hiding this comment

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

I was curious why we need to do this? Should we doing it for other types of sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing that for raw string like data we're passing in views of the data into these readers, and Python will tend to garbage collect the sources sooner than the views that are passed into the readers.

Should we doing it for other types of sources?

Ideally no, I think generally it's good to not unnecessarily hold a reference to source if not needed so that memory (albeit probably small) is cleaned up

@mroeschke
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 353d2de into rapidsai:branch-24.12 Nov 13, 2024
102 checks passed
@mroeschke mroeschke deleted the plc/cln/misc branch November 13, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Matt711 Matt711 Matt711 approved these changes

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

Assignees

@mroeschke mroeschke

Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
cuDF Python
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mroeschke @Matt711