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._libs.types.pyx #17665

Merged
merged 14 commits into from
Jan 9, 2025

Conversation

mroeschke
Copy link
Contributor

@mroeschke mroeschke commented Dec 31, 2024

Description

Contributes to #17317

  1. Moves some Python routines/objects to cudf/utils/dtypes.py
  2. Moves specific column only routines directly to cudf/_libs/column.pyx

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 31, 2024
@mroeschke mroeschke self-assigned this Dec 31, 2024
@mroeschke mroeschke requested a review from a team as a code owner December 31, 2024 19:32
@github-actions github-actions bot added the CMake CMake build issue label Dec 31, 2024
@mroeschke mroeschke requested a review from a team as a code owner January 7, 2025 02:23
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Some small nits

@@ -64,6 +62,80 @@ cdef get_element(column_view col_view, size_type index):
)


def dtype_from_pylibcudf_column(col):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def dtype_from_pylibcudf_column(col):
def dtype_from_pylibcudf_column(Column col not None):

python/cudf/cudf/_lib/column.pyx Outdated Show resolved Hide resolved
)
elif cdtype_id == plc.TypeID.STRUCT:
self._dtype = StructDtype.from_arrow(
plc.interop.to_arrow(self.c_value).type
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This now necessitates a device to host copy and synchronisation just to get the type (which is a host-side thing).

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 I was hoping that this would be temporarily OK as I'm aiming remove DeviceScalar (along with this code) by the next release cc @vyasr

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's ok if it's going soon anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I'm OK leaving this for now in anticipation of the code being removed altogether soon.

plc.interop.to_arrow(self.c_value).type
)
elif cdtype_id == plc.TypeID.LIST:
self._dtype = ListDtype.from_arrow(plc.interop.to_arrow(self.c_value).type)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: same issue here as with structs.

@mroeschke
Copy link
Contributor Author

/merge

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.

Ack I though I submitted this review already.

One suggestion for improvement, otherwise LGTM.

Comment on lines +16 to +17
cdef dtype_from_lists_column_view(column_view cv)
cdef dtype_from_column_view(column_view cv)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove these declarations? AFAICT these functions aren't needed outside of column.pyx, and you shouldn't have to declare functions in a pxd file if they aren't going to be cimported into other modules (you do have to declare cdef classes though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dtype_from_column_view is actually cimported in Morpheus currently nv-morpheus/Morpheus#2097 but ah yes I suppose dtype_from_lists_column_view didn't need to be defined here

@rapids-bot rapids-bot bot merged commit a8a4197 into rapidsai:branch-25.02 Jan 9, 2025
106 checks passed
@mroeschke mroeschke deleted the cln/cudf/types/2 branch January 9, 2025 22:18
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.

3 participants