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 DataFrame support from Dask cuDF #17558

Merged
merged 19 commits into from
Jan 7, 2025
Merged

Remove "legacy" Dask DataFrame support from Dask cuDF #17558

merged 19 commits into from
Jan 7, 2025

Conversation

rjzamora
Copy link
Member

@rjzamora rjzamora commented Dec 9, 2024 •
edited
Loading

Description

The legacy Dask DataFrame API is deprecated. We should remove it for 25.02 to reduce maintenance burden.

Blockers:

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 2 - In Progress Currently a work in progress dask Dask issue improvement Improvement / enhancement to an existing function breaking Breaking change labels Dec 9, 2024
@rjzamora rjzamora self-assigned this Dec 9, 2024
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Dec 9, 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 the Python Affects Python cuDF API. label Dec 9, 2024
@rjzamora
Copy link
Member Author

rjzamora commented Dec 9, 2024

/ok to test

@rjzamora rjzamora marked this pull request as ready for review December 16, 2024 14:38
@rjzamora rjzamora requested review from a team as code owners December 16, 2024 14:38
@rjzamora rjzamora requested a review from jameslamb December 16, 2024 14:38
jameslamb
jameslamb approved these changes Dec 16, 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.

Packaging changes look good to me. Will defer to others on the substance of removing this support.

galipremsagar
galipremsagar approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

LGTM

python/dask_cudf/dask_cudf/__init__.py
Comment on lines +16 to +17
"The legacy DataFrame API is not supported in dask_cudf>24.12. "
"Please enable query-planning, or downgrade to dask_cudf<=24.12"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"The legacy DataFrame API is not supported in dask_cudf>24.12. "
"Please enable query-planning, or downgrade to dask_cudf<=24.12"
"The legacy DataFrame API is not supported in dask_cudf>24.12."
"Please enable query-planning, or downgrade to dask_cudf<=24.12"

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't we want a space between the period and the first word of the next sentence?

@rjzamora rjzamora marked this pull request as draft December 16, 2024 18:56
This was referenced Dec 16, 2024
Merged
Merged
@rjzamora
Copy link
Member Author

Update: This is blocked by rapidsai/dask-cuda#1417 (which is now ready for review)

@rjzamora
Copy link
Member Author

Update: This PR now removes 2,000+ lines of code that would be essentially "dead" outside the legacy API.

@rjzamora
Copy link
Member Author

/okay to test

@rjzamora
Copy link
Member Author

rjzamora commented Jan 6, 2025

/okay to test

rapids-bot bot pushed a commit to rapidsai/dask-cuda that referenced this pull request Jan 7, 2025
@rjzamora
Remove legacy Dask-cuDF handling (#1417)
c19a1a7 
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).

Authors:
  - Richard (Rick) Zamora (https://github.com/rjzamora)
  - Peter Andreas Entschev (https://github.com/pentschev)

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

URL: #1417
@rjzamora rjzamora marked this pull request as ready for review January 7, 2025 17:33
mroeschke
mroeschke approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Looks like python/dask_cudf/dask_cudf/io/tests/test_csv.py needs a copyright update otherwise LGTM.

(I think after the dask-cuda update to remove legacy dask, this PR will be necessary to unblock other PRs? e.g. https://github.com/rapidsai/cudf/actions/runs/12643976310/job/35232330877?pr=17686)

@rjzamora
Copy link
Member Author

rjzamora commented Jan 7, 2025

I think after the dask-cuda update to remove legacy dask, this PR will be necessary to unblock other PRs?

Ah! I can't believe I forgot that we run distributed tests in dask_cudf - Yeah, we can get this in soon then and I can do some cleanup in a follow-up PR (also need some cleanup in dask-cuda before we can unpin Dask).

@rjzamora rjzamora added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Jan 7, 2025
@rjzamora
Copy link
Member Author

rjzamora commented Jan 7, 2025

/merge

@rapids-bot rapids-bot bot merged commit 30c6caa into rapidsai:branch-25.02 Jan 7, 2025
110 checks passed
@rjzamora rjzamora deleted the remove-legacy-dataframe branch January 7, 2025 20:15
@rjzamora rjzamora mentioned this pull request Jan 9, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroeschke mroeschke mroeschke approved these changes

@galipremsagar galipremsagar galipremsagar approved these changes

@jameslamb jameslamb jameslamb approved these changes

Assignees

@rjzamora rjzamora

Labels
2 - In Progress Currently a work in progress 5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change dask Dask issue improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.
Projects
cuDF Python
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rjzamora @jameslamb @mroeschke @galipremsagar