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

Support more numeric types in Groupby.apply with engine='jit' #13729

Conversation

brandon-b-miller
Copy link
Contributor

draft

This PR adds additional numeric dtypes to GroupBy.apply with engine='jit'.

@github-actions github-actions bot added the Python Affects Python cuDF API. label Jul 21, 2023
@brandon-b-miller brandon-b-miller added feature request New feature or request non-breaking Non-breaking change labels Jul 21, 2023
@brandon-b-miller
Copy link
Contributor Author

I ran into some issues compiling the shim library with int16 and int8 dtypes here. It looks like our implementations of the underlying block level reduction functions rely on libcu++ atomic_ref which per the documentation may only be instantiated with a T that are either 4 or 8 bytes. This seems consistent with the error I'm getting:

atomic_cuda.h(111): error: static assertion failed with "atomic_ref does not support 1 or 2 byte types

This means that to support shorter ints we will need to do a little more involved development on the c++ side that I think should be separated out into another PR. I'm updating the PR title to reflect that this PR itself only goes as far as rounding us out with int32 and float32.

@brandon-b-miller
Copy link
Contributor Author

raised #13736

@brandon-b-miller brandon-b-miller marked this pull request as ready for review July 24, 2023 14:06
@brandon-b-miller brandon-b-miller requested a review from a team as a code owner July 24, 2023 14:06
@brandon-b-miller brandon-b-miller requested review from bdice and isVoid and removed request for a team July 24, 2023 14:06
@brandon-b-miller brandon-b-miller added the 3 - Ready for Review Ready for review by team label Jul 24, 2023
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I'm not a huge fan of repeating the typecasting logic twice in Python and once in C++... we'll need to find a better architecture for this before we get too deep into adding more types.

_register_cuda_idx_reduction_caller("IdxMax", ty)
_register_cuda_idx_reduction_caller("IdxMin", ty)

_register_cuda_reduction_caller("Sum", types.int32, types.int64)
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like the typecasting rules were hard-coded twice. Once here, and once in GroupSum. Is there a way to reuse a single mapping / type promotion function?

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, this is a good point. There should only be one type mapping. Let me see what I can do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the latest commit, I've refactored towards an attribute generating function which checks the existing registry of cuda functions for a match in order to return a signature. I think that makes it so that this is the only place in python where the signatures are written by hand. Let me know what you think.

@brandon-b-miller
Copy link
Contributor Author

@bdice thanks for your review. I've thought a bit about making it so that there's only one centralized place to keep all of the function signatures that could be read directly from c++ or python. Since the shim function library is built before the python is imported, I suppose it should be the source of truth as well. This leads my mind towards some kind of solution which inspects the shim file at import and generates signatures dynamically from it. @gmarkall can cuobjdump or nvdisasm help us here?

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Much better to only define these once in Python. I'm okay with redefining in C++ and Python if that is needed. It is probably difficult to do this dynamically. I have one question, otherwise approving.

python/cudf/cudf/core/udf/groupby_typing.py Show resolved Hide resolved
@brandon-b-miller
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 43aca00 into rapidsai:branch-23.10 Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants