-
Notifications
You must be signed in to change notification settings - Fork 931
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
Register cudf.core.groupby.Grouper
objects to dask grouper_dispatch
#10838
Register cudf.core.groupby.Grouper
objects to dask grouper_dispatch
#10838
Conversation
cudf.core.groupby.Grouper
objects to dask get_grouper
cudf.core.groupby.Grouper
objects to dask grouper_dispatch
@galipremsagar do I need to bump the dask version here to account for dask/dask#9074 ? |
Ideally yes, but since there is no dask version released yet. We will have to move the dispatch importing and registering to the try/except block just below it. Similar to try:
from dask.dataframe.dispatch import grouper_dispatch
@grouper_dispatch.register((cudf.Series, cudf.DataFrame))
def get_grouper_cudf(obj):
return cudf.core.groupby.Grouper
except ImportError:
pass |
tysm @galipremsagar ! |
Is it worth mirroring the tests added in dask/dask@e531cd0 ? |
Would it be easier to add an API call test that will go through the dispatch mechanisam? Instead of |
In that case I think it's covered by #10889 , so maybe it's OK for now. |
Cool, then that sounds good to me. |
rerun tests |
@gpucibot merge |
Codecov Report
@@ Coverage Diff @@
## branch-22.08 #10838 +/- ##
===============================================
Coverage ? 86.61%
===============================================
Files ? 144
Lines ? 23511
Branches ? 0
===============================================
Hits ? 20364
Misses ? 3147
Partials ? 0 Continue to review full report at Codecov.
|
@brandon-b-miller could you re-target this to |
…h` (rapidsai#10838) This PR registers uses the (presumably shortly merged) dask `Grouper` dispatch to register `cudf.core.groupby.Grouper` objects to `cudf.DataFrame` objects. This should allow our own Grouper objects to be used in critical places in dask rather than pandas objects. This solution is favorable IMO rather than changing cuDF to handle pandas grouper objects directly. Xref dask/dask#9074 Authors: - https://github.com/brandon-b-miller Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: rapidsai#10838
…h` (#10838) (#10982) Backport of #10838 Authors: - https://github.com/brandon-b-miller Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) - AJ Schmidt (https://github.com/ajschmidt8)
Closes #10296 These _should_ actually just work if the following PRs get merged, after which this diff might be really small: #10815 #10838 dask/dask#9074 Authors: - https://github.com/brandon-b-miller - Charles Blackmon-Luca (https://github.com/charlesbluca) Approvers: - Charles Blackmon-Luca (https://github.com/charlesbluca) URL: #10889
This PR registers uses the (presumably shortly merged) dask
Grouper
dispatch to registercudf.core.groupby.Grouper
objects tocudf.DataFrame
objects. This should allow our own Grouper objects to be used in critical places in dask rather than pandas objects.This solution is favorable IMO rather than changing cuDF to handle pandas grouper objects directly.
Xref dask/dask#9074