Refactor Dask cuDF legacy code #17205

rjzamora · 2024-10-29T21:49:51Z

Description

The "legacy" DataFrame API is now deprecated (dask/dask#11437). The main purpose of this PR is to start isolating legacy code in Dask cuDF.

Old layout:

dask_cudf/
├── expr/
│   ├── _collection.py
│   ├── _expr.py
│   ├── _groupby.py
├── io/
│   ├── tests/
│   ├── ...
│   ├── parquet.py
│   ├── ...
├── tests/
├── accessors.py
├── backends.py
├── core.py
├── groupby.py
├── sorting.py

New layout:

dask_cudf/
├── _expr/
│   ├── accessors.py
│   ├── collection.py
│   ├── expr.py
│   ├── groupby.py
├── _legacy/
│   ├── io/
│   ├── core.py
│   ├── groupby.py
│   ├── sorting.py
├── io/
│   ├── tests/
│   ├── ...
│   ├── parquet.py
│   ├── ...
├── tests/
├── backends.py
├── core.py

Notes

This PR adds some backward compatibility to the expr-based API that was previously missing: The user can now import collection classes from dask_cudf.core (previously led to a "silent" bug when query-planning was enabled).
The user can also import various IO functions from dask_cudf.io (and sub-modules like dask_cudf.io.parquet), but they will typically get a deprecation warning.
This PR is still technically "breaking" in the sense that the user can no longer import some functions/classes from dask_cudf.io.*. Also, the groupby, sorting, and accessors modules have simply moved. It should be uncommon for down-stream code to import from these modules. It's also worth noting that query-planning was already causing problems for these users if they were doing this.

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 · 2024-10-29T23:33:49Z

python/dask_cudf/dask_cudf/__init__.py

-# For dask>2024.2.0, we can silence the loud deprecation
-# warning before importing `dask.dataframe` (this won't
-# do anything for dask==2024.2.0)
-config.set({"dataframe.query-planning-warning": False})


No longer relevant.

…sk-cudf

…factor-dask-cudf

rjzamora · 2024-10-29T23:48:30Z

python/dask_cudf/dask_cudf/_expr.py

+_PATCHED = False
+
+
+def _patch_dask_expr():


Most of this PR is just moving around existing code. This change is actually new - We used to implicitly "patch" dask-expr when the expr module was imported. I'm trying to make the patching a bit more explicit (but I only want to allow this function to do its thing once).

madsbk · 2024-10-30T08:00:28Z

If _accessors.py & co. are all private, would it make sense to move them to a sub-folder and avoid the _ prefix?

wence- · 2024-10-30T11:10:33Z

Question: is there any user-visible change to this refactoring, such that we must warn users about the deprecation? Or is it that we can just swap out the implementation from under them?

rjzamora · 2024-10-30T13:39:43Z

If _accessors.py & co. are all private, would it make sense to move them to a sub-folder and avoid the _ prefix?

I'm happy to do whatever others prefer. I just want to choose some convention that helps us move in the direction of making everything other than dask_cudf.<function-or-class> private. Dask-expr uses a flat directory of .py files with the _ prefix. We can certainly put everything in a _-prefixed directory instead.

Question: is there any user-visible change to this refactoring, such that we must warn users about the deprecation? Or is it that we can just swap out the implementation from under them?

There may be some code that people were importing directly from dask_cudf.io (e.g. dask_cudf.io.parquet.create_metadata_file). If I understand correctly, everything else is already broken when query-planning is enabled, so we would only cause "new" problems for legacy-API users (and the legacy API will definitely be gone for 25.02).

With that said, I'll try to make some changes to better-warn people who try to import things from "deprecated" paths.

madsbk · 2024-10-31T14:43:38Z

@rjzamora agree, the important thing is to mark them private.
What about something like:

dask_cudf/
├── _expr/
│   ├── collection.py
│   ├── expr.py
│   ├── groupby.py
│   ├── accessors.py

…sk-cudf

rjzamora · 2024-11-01T17:33:10Z

What about something like:

Sounds good @madsbk - I made this change.

…sk-cudf

rjzamora · 2024-11-01T23:47:09Z

python/dask_cudf/dask_cudf/__init__.py

-def raise_not_implemented_error(attr_name):
+def _deprecated_api(old_api, new_api=None, rec=None):
    def inner_func(*args, **kwargs):
+        if new_api:
+            # Use alternative
+            msg = f"{old_api} is now deprecated. "
+            msg += rec or f"Please use {new_api} instead."
+            warnings.warn(msg, FutureWarning)
+            new_attr = new_api.split(".")
+            module = import_module(".".join(new_attr[:-1]))
+            return getattr(module, new_attr[-1])(*args, **kwargs)
+
+        # No alternative - raise an error
        raise NotImplementedError(
-            f"Top-level {attr_name} API is not available for dask-expr."
+            f"{old_api} is no longer supported. " + (rec or "")
        )

    return inner_func


Note: I decided to expand this utility to help deal with the many dask_cudf.io APIs that are no longer directly accessible with query-planning on.

rjzamora · 2024-11-01T23:49:22Z

python/dask_cudf/dask_cudf/__init__.py

+
+to_orc = _deprecated_api(
+    "dask_cudf.to_orc",
+    new_api="dask_cudf._legacy.io.to_orc",


The legacy to_orc API is actually "fine" (it's compatible with query-planning stuff), but we might as well discourage this kind of usage.

madsbk

Looks good to me, thanks @rjzamora

…sk-cudf

rjzamora · 2024-11-04T21:06:30Z

/merge

rjzamora added 4 commits October 28, 2024 19:23

initial refactor

da9b6fb

rename _collection.py to core.py

196c0c0

keep _collection name

5be712b

move legacy io

957299b

rjzamora added 2 - In Progress Currently a work in progress dask Dask issue non-breaking Non-breaking change labels Oct 29, 2024

rjzamora self-assigned this Oct 29, 2024

rjzamora requested a review from a team as a code owner October 29, 2024 21:49

github-actions bot added the Python Affects Python cuDF API. label Oct 29, 2024

rjzamora added improvement Improvement / enhancement to an existing function breaking Breaking change and removed non-breaking Non-breaking change labels Oct 29, 2024

rjzamora added 3 commits October 29, 2024 16:21

split out _expr.py logic

86b3fa6

fix patching

60853f6

Merge branch 'branch-24.12' into refactor-dask-cudf

56644e8

rjzamora commented Oct 29, 2024

View reviewed changes

rjzamora added 3 commits October 29, 2024 16:41

tweak _patch_dask_expr

db4556f

Merge remote-tracking branch 'upstream/branch-24.12' into refactor-da…

8dec940

…sk-cudf

Merge branch 'refactor-dask-cudf' of github.com:rjzamora/cudf into re…

e54807f

…factor-dask-cudf

rjzamora commented Oct 29, 2024

View reviewed changes

rjzamora added 3 commits November 1, 2024 06:57

copy upstream changes

ec139d2

Merge remote-tracking branch 'upstream/branch-24.12' into refactor-da…

b5f92c9

…sk-cudf

use Mads' suggestion

8fb4657

remove core - not worth the tech debt

1d7c84e

rjzamora added 3 commits November 1, 2024 10:37

Merge remote-tracking branch 'upstream/branch-24.12' into refactor-da…

ab56534

…sk-cudf

add back core.py

788cb24

add deprecation warnings for various IO methods

1f54219

rjzamora commented Nov 1, 2024

View reviewed changes

madsbk approved these changes Nov 4, 2024

View reviewed changes

rjzamora added 2 commits November 4, 2024 06:24

Merge remote-tracking branch 'upstream/branch-24.12' into refactor-da…

95a6697

…sk-cudf

add test coverage for _deprecated_api usage

1b36183

rjzamora added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Nov 4, 2024

rjzamora changed the title ~~[WIP] Refactor Dask cuDF legacy code~~ Refactor Dask cuDF legacy code Nov 4, 2024

rjzamora added 2 commits November 4, 2024 08:20

fix parquet test mistake

bc9897b

Merge remote-tracking branch 'upstream/branch-24.12' into refactor-da…

a84128a

…sk-cudf

rjzamora added 5 - Ready to Merge Testing and reviews complete, ready to merge 3 - Ready for Review Ready for review by team and removed 3 - Ready for Review Ready for review by team 5 - Ready to Merge Testing and reviews complete, ready to merge labels Nov 4, 2024

Merge branch 'branch-24.12' into refactor-dask-cudf

ba3032a

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 Nov 4, 2024

rapids-bot bot merged commit 45563b3 into rapidsai:branch-24.12 Nov 4, 2024
102 checks passed

rjzamora deleted the refactor-dask-cudf branch November 4, 2024 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Dask cuDF legacy code #17205

Refactor Dask cuDF legacy code #17205

rjzamora commented Oct 29, 2024 •

edited

Loading

rjzamora Oct 29, 2024

rjzamora Oct 29, 2024

madsbk commented Oct 30, 2024

wence- commented Oct 30, 2024

rjzamora commented Oct 30, 2024

madsbk commented Oct 31, 2024

rjzamora commented Nov 1, 2024

rjzamora Nov 1, 2024

rjzamora Nov 1, 2024

madsbk left a comment

rjzamora commented Nov 4, 2024

Refactor Dask cuDF legacy code #17205

Refactor Dask cuDF legacy code #17205

Conversation

rjzamora commented Oct 29, 2024 • edited Loading

Description

Checklist

rjzamora Oct 29, 2024

Choose a reason for hiding this comment

rjzamora Oct 29, 2024

Choose a reason for hiding this comment

madsbk commented Oct 30, 2024

wence- commented Oct 30, 2024

rjzamora commented Oct 30, 2024

madsbk commented Oct 31, 2024

rjzamora commented Nov 1, 2024

rjzamora Nov 1, 2024

Choose a reason for hiding this comment

rjzamora Nov 1, 2024

Choose a reason for hiding this comment

madsbk left a comment

Choose a reason for hiding this comment

rjzamora commented Nov 4, 2024

rjzamora commented Oct 29, 2024 •

edited

Loading