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

Patch dask-expr var logic in dask-cudf #15347

Merged
merged 21 commits into from Apr 8, 2024
Merged

Patch dask-expr var logic in dask-cudf #15347

merged 21 commits into from Apr 8, 2024

Conversation

rjzamora
Copy link
Member

Description

The var logic in dask-expr relies on pandas -> numpy conversion that does not work for cudf -> cupy when null values are present. This PR copies over the custom var logic being used in dask-cudf for the legacy API.

Checklist

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

@rjzamora rjzamora added the 2 - In Progress Currently a work in progress label Mar 20, 2024
@rjzamora rjzamora self-assigned this Mar 20, 2024
@github-actions github-actions bot added cuDF (Python) Affects Python cuDF API. ci labels Mar 20, 2024
@rjzamora rjzamora added bug Something isn't working non-breaking Non-breaking change labels Mar 20, 2024
@rjzamora rjzamora mentioned this pull request Mar 20, 2024
Open
28 tasks
bdice
bdice requested changes Mar 20, 2024
View reviewed changes
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.

We need to add a test in dask-cudf for the functions used in the 10min notebook.

@rjzamora
Copy link
Member Author

We need to add a test in dask-cudf for the functions used in the 10min notebook.

Agreed - This is still a draft.

@rjzamora
Copy link
Member Author

rjzamora commented Mar 20, 2024 •
edited

Note that the custom VarCudf expression implemented in this PR will not always work without an upstream change like dask/dask#11013 (I used a "workaround" in this PR that is simple, but not very intuitive)

@rjzamora
Copy link
Member Author

CI failures are due to a bad/old dask-expr version being pulled (dask/distributed#8574)

@rjzamora rjzamora marked this pull request as ready for review April 2, 2024 17:36
@rjzamora rjzamora requested review from a team as code owners April 2, 2024 17:36
@rjzamora
Copy link
Member Author

rjzamora commented Apr 2, 2024

The dask/dask-expr nightly-conda situation should be fixed now. So dask-related CI should start passing now.

@bdice
Copy link
Contributor

bdice commented Apr 3, 2024 •
edited

I reran the failed CI jobs (network issues). @rjzamora Is this ready for review?

bdice
bdice approved these changes Apr 3, 2024
View reviewed changes
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.

Changes look fine to me.

python/dask_cudf/dask_cudf/expr/_expr.py Outdated Show resolved Hide resolved
@rjzamora
Copy link
Member Author

rjzamora commented Apr 3, 2024

Is this ready for review?

Yeah, thanks @bdice !

@rjzamora
Copy link
Member Author

rjzamora commented Apr 8, 2024

/merge

@rapids-bot rapids-bot bot merged commit 3896222 into rapidsai:branch-24.06 Apr 8, 2024
70 checks passed
@rjzamora rjzamora deleted the fix-var branch April 8, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raydouglass raydouglass raydouglass approved these changes

@bdice bdice bdice approved these changes

@galipremsagar galipremsagar galipremsagar approved these changes

Assignees

@rjzamora rjzamora

Labels
2 - In Progress Currently a work in progress bug Something isn't working ci cuDF (Python) Affects Python cuDF API. non-breaking Non-breaking change
Projects
cuDF/Dask/Numba/UCX
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rjzamora @bdice @raydouglass @galipremsagar