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

Remove legacy Dask-cuDF handling #1417

Open
wants to merge 10 commits into
base: branch-25.02
Choose a base branch
from
Open

Remove legacy Dask-cuDF handling #1417

wants to merge 10 commits into from
+55 −151

Conversation

rjzamora
Copy link
Member

Removes testing/handling for "legacy" Dask cuDF (i.e. DASK_DATAFRAME__QUERY_PLANNING=False).

This PR also adds support for the "explicit-comms" config with query-planning enabled (we used to raise an error telling the user to disable query planning).

This should be merged before rapidsai/cudf#17558 (otherwise Dask-CUDA CI will break).
This PR is marked as "breaking", because it technically breaks the "explicit-comms" config with the "legacy" version of Dask cuDF (which we are about to remove in 25.02 anyway).

@rjzamora rjzamora added 2 - In Progress Currently a work in progress feature request New feature or request breaking Breaking change labels Dec 16, 2024
@rjzamora rjzamora self-assigned this Dec 16, 2024
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Dec 16, 2024

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added python python code needed ci labels Dec 16, 2024
@rjzamora
Copy link
Member Author

/ok to test

@pentschev
Copy link
Member

The build failure is not related to the changes here, it should be fixed by #1418 .

@rjzamora
Copy link
Member Author

Thanks @pentschev - Just a heads up that I do see some pytest failures locally. My Shuffle patching approach seems to have issues. I spent a lot of unsuccessful time debugging yesterday - So, I may need to roll back the "explicit-comms" config support (for now).

@pentschev
Copy link
Member

/ok to test

@rjzamora
Copy link
Member Author

/okay to test

@rjzamora rjzamora marked this pull request as ready for review December 18, 2024 15:07
@rjzamora rjzamora requested review from a team as code owners December 18, 2024 15:07
@rjzamora rjzamora requested a review from AyodeAwe December 18, 2024 15:07
rjzamora
rjzamora commented Dec 18, 2024
View reviewed changes
dask_cuda/__init__.py Outdated
dask.dataframe.shuffle.get_default_shuffle_method = get_default_shuffle_method
dask.dataframe.multi.get_default_shuffle_method = get_default_shuffle_method
dask.bag.core.get_default_shuffle_method = get_default_shuffle_method
patch_shuffle_expression()
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @madsbk (for viz) - Now that we are dropping "legacy" support, we need to do something a bit different to satisfy the "explicit-comms" config. Let me know if you have any thoughts or concerns.

@rjzamora rjzamora changed the title [WIP] Remove legacy Dask-cuDF handling Remove legacy Dask-cuDF handling Dec 18, 2024
@rjzamora rjzamora added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Dec 18, 2024
@rjzamora rjzamora mentioned this pull request Dec 18, 2024
Draft
4 tasks
pentschev
pentschev approved these changes Dec 18, 2024
View reviewed changes
Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

I think this looks good now given tests are also passing. Would be best if @madsbk could have a look too before we merge.

jameslamb
jameslamb approved these changes Dec 18, 2024
View reviewed changes
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Giving you a packaging-codeowners approval. Thanks for removing those warnings filters from pyproject.toml!

@jameslamb jameslamb removed the request for review from AyodeAwe December 18, 2024 23:28
@jakirkham jakirkham requested a review from madsbk December 19, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameslamb jameslamb jameslamb approved these changes

@pentschev pentschev pentschev approved these changes

@madsbk madsbk Awaiting requested review from madsbk

Assignees

@rjzamora rjzamora

Labels
3 - Ready for Review Ready for review by team breaking Breaking change ci feature request New feature or request python python code needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rjzamora @pentschev @jameslamb