-
Notifications
You must be signed in to change notification settings - Fork 900
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
Revise shuffle
deprecation to align with dask/dask
#14762
Revise shuffle
deprecation to align with dask/dask
#14762
Conversation
cc @pentschev (Thanks for tackling the original fix! Let me know if you have any thoughts on this small update) |
CI failures seem unrelated |
I think this would probably be solved by just re-running the failed jobs:
Looks like temporary outage in services from
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock so we can fix CI
/merge |
Thanks all! 🙏 Please feel free to submit follow up work if needed. Just want to unblock other PRs |
Sorry I missed this yesterday. Changes look fine to me nevertheless, thanks @rjzamora ! |
Description
It seems that #14708 was a bit too aggressive in updating the
shuffle
kwarg toshuffle_method
, and gpu-CI is currently failing in dask/dask.This PR aligns dask-cudf with
dask.dataframe
by warning the user when theshuffle
argument is used (rather than raising an error). I definitely don't think it makes sense for RAPIDS to be more strict than dask/dask about this argument.This also fixes a bug in
groupby.aggregate()
(where we were still usingshuffle=
)Checklist