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

Merged
merged 12 commits into from
Jan 7, 2025
Merged

Remove legacy Dask-cuDF handling #1417

merged 12 commits into from
Jan 7, 2025

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
Merged
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
@rjzamora
Copy link
Member Author

rjzamora commented Jan 6, 2025

@pentschev @madsbk - I plan to merge this tomorrow AM if there are no other review comments.

madsbk
madsbk approved these changes Jan 6, 2025
View reviewed changes
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Thanks @rjzamora, looks good.

@rjzamora rjzamora added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Jan 7, 2025
@rjzamora
Copy link
Member Author

rjzamora commented Jan 7, 2025

/merge

@rapids-bot rapids-bot bot merged commit c19a1a7 into rapidsai:branch-25.02 Jan 7, 2025
32 checks passed
@rjzamora rjzamora deleted the remove-legacy-support branch January 7, 2025 17:04
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Jan 7, 2025
@rjzamora
Remove "legacy" Dask DataFrame support from Dask cuDF (#17558)
30c6caa 
The legacy Dask DataFrame API is deprecated. We should remove it for 25.02 to reduce maintenance burden.

**Blockers**:

- [x] rapidsai/dask-cuda#1417

Authors:
  - Richard (Rick) Zamora (https://github.com/rjzamora)

Approvers:
  - James Lamb (https://github.com/jameslamb)
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Matthew Roeschke (https://github.com/mroeschke)

URL: #17558
@rjzamora rjzamora mentioned this pull request Jan 7, 2025
Merged
rapids-bot bot pushed a commit that referenced this pull request Jan 9, 2025
@rjzamora
Clean up imports for dask>2024.12.1 support (#1424)
afc27f4 
Follow up to #1417

Cleans up some imports (some of which don't work for `dask>2024.12.1`).

Authors:
  - Richard (Rick) Zamora (https://github.com/rjzamora)

Approvers:
  - Mads R. B. Kristensen (https://github.com/madsbk)
  - Peter Andreas Entschev (https://github.com/pentschev)

URL: #1424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madsbk madsbk madsbk approved these changes

@pentschev pentschev pentschev approved these changes

@jameslamb jameslamb jameslamb approved these changes

Assignees

@rjzamora rjzamora

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge 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.
4 participants
@rjzamora @pentschev @madsbk @jameslamb