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

Correctly raise for unimplemented timelike binary ops in cudf-polars #16188

Draft
wants to merge 1 commit into
base: branch-24.08
Choose a base branch
from

Conversation

brandon-b-miller
Copy link
Contributor

This PR adds logic that should trigger CPU fallback for a couple of unsupported binaryops that are incorrectly making it to libcudf in the cudf-polars code.

WIP

@github-actions github-actions bot added Python Affects Python cuDF API. cudf.polars Issues specific to cudf.polars labels Jul 3, 2024
@brandon-b-miller
Copy link
Contributor Author

@wence- I had a general thought about this - this logic is currently very rough, but broadly speaking, I think our intention here is to fallback to CPU for any ops that we can't support through libcudf. Rather than rolling our own logic to check for all the cases that should throw, what about creating a pylibcudf wrapper for is_supported_binaryop (moved to public in #15470) and using it to query if an op is supported, then raising NotImplementedError if not? If this seems like a reasonable idea I can push that PR forward by cutting it down a bit and then move forward here.

@wence-
Copy link
Contributor

wence- commented Jul 3, 2024

@wence- I had a general thought about this - this logic is currently very rough, but broadly speaking, I think our intention here is to fallback to CPU for any ops that we can't support through libcudf. Rather than rolling our own logic to check for all the cases that should throw, what about creating a pylibcudf wrapper for is_supported_binaryop (moved to public in #15470) and using it to query if an op is supported, then raising NotImplementedError if not? If this seems like a reasonable idea I can push that PR forward by cutting it down a bit and then move forward here.

Yes, let's get that over the line rather than replicating functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cudf.polars Issues specific to cudf.polars Python Affects Python cuDF API.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants