feat: Dask Support Implementation #484

benrutter · 2024-07-10T16:46:15Z

What type of PR is this? (check all applicable)

Related issues

Related issue [Enh]: Add support for dask #396
Closes [Enh]: Add support for dask #396

Checklist

Code follows style guide (ruff)
Tests added
Documented the changes (I don't think so, is there a changelog to add to?)

If you have comments or can explain your changes, please do so below.

Hello,

This is still a draft PR (although everything is in a working state, there's a very unperformance step in it I've put in that I do need to take out) but I'd love to get some feedback around making integration as sleek as possible!

I've implemented a bunch of logic with something along the lines of:

if self._integration == "dask":
    return do_some_bespoke_thing()
return the_happy_path()

Which I think for the most part is fine because that's generally how other implementations like modin etc are done? Not sure if there's any guidance around another way of doing this? (or maybe I should just put in some friendly comments where that happens to explain why dask is implemented differently). I actually found it pretty clear to follow along, but the alternative I could think of woud be subclassing or something.

The other thing I've done (mentioned over on the Narwhals discord) is implement .collect() to .compute() a dask dataframe, which seems like good behaviour. Dask dataframes are a little like polars lazyframes, and computing them forces all data into a single noded dataframe.

There are also a few things where something just isn't possible with dask, for instance cross-joins. Which dask doesn't support natively. Theoretically you could hack something in by doing something like adding two columns that are always true, joining on that, then dropping the column, but I thought it was a better move to maintain the implementations of dask as mark as "not implemented" (particularly as my understanding is dask intentionally doesn't implement cross joins for performance reasons).

I was thinking it might be nice to put in a decorator along the lines of:

@not_implemented_in(["dask", "modin"])
def self.some_method(x: Any) -> Any:
   ...

To nicely raise a NotImplementedError if that method gets called with one of those backends? Any thoughts?

It's my first time working with the Narwhal's codebase - I've had a blast running through it! I'm probably not familiar with all the conventions etc, so please do let me know if I've missed something major.

MarcoGorelli

Awesome effort, thanks @benrutter !

a few comments:

I think a subclass or a decorator should be fine. I think there's not that many if/then statements you had to add? maybe a decorator is easier then. for shape we can unify the implementations under len(self._native_series)
merge conflicts: there are some merge conflicts, but it's mostly down to some things having been renamed:
- PandasDataFrame -> PandasLikeDataFrame
- ._series -> ._native_series
- implementation == "dask" => implementation is Implementation.DASK

The major refactors are over (for pandas-like), I promise not to do this again 😄 😳 Fancy fixing up the merge conflicts?

narwhals/_pandas_like/series.py

narwhals/_pandas_like/dataframe.py

benrutter · 2024-07-11T10:13:40Z

@MarcoGorelli absolutely! I'll do some "tidy up" changes and then work through the merge conflicts.

for more information, see https://pre-commit.ci

benrutter · 2024-07-12T16:18:52Z

Ok, I thought I'd sorted out the merge conflicts, and at least test_common is working for both. But looks like I've introduced some kind of syntax error somewhere else or something. There's a lot of failing tests, oh dear. . .

I'll have a bit more of a look some time soon.

MarcoGorelli · 2024-07-12T16:26:47Z

awesome stuff! love the decorator

a lot of narwhals/_pandas_like/utils.py was moved to narwhals/_expression_parsing.py, as it's reused between pandas-like APIs and pyarrow

benrutter · 2024-07-12T16:56:25Z

Ah thanks! That probably explains it then- I might just need to implement a few small bits over there then.

for more information, see https://pre-commit.ci

benrutter · 2024-07-15T15:50:50Z

Ok, all tests passing bar one which I'll mention in a sec (and sorted out the latest conflicts).

Looks like one of the tests which checks the API docs for stable.v1 is passing, but I'm not 100% sure of the intention behind it so don't want to mess too much without checking.

Is the idea that I should go in and update the documentation so that v1 matches elsewhere?

MarcoGorelli · 2024-07-15T15:57:24Z

Is the idea that I should go in and update the documentation so that v1 matches elsewhere?

yup, if you update the main namespace docs, you should also update the v1 docs

benrutter · 2024-07-15T18:29:10Z

Awesome - thanks @MarcoGorelli for all the guidance! Looks like all the tests are passing now (I spotted they're failing in 3.8 python but that just looks like the old type hinting API not being supported rather than anything new here)

MarcoGorelli

thanks @benrutter !

left some comments, there's some pre-commit checks to fixup too, but awesome work here

narwhals/_pandas_like/dataframe.py

narwhals/_pandas_like/group_by.py

MarcoGorelli · 2024-07-15T18:32:08Z

narwhals/_pandas_like/group_by.py

+        dataframe_is_empty = (
+            self._df._native_dataframe.empty
+            if self._df._implementation != Implementation.DASK
+            else len(self._df._native_dataframe) == 0
+        )


can we just use self._df.is_empty()?

narwhals/_pandas_like/series.py

MarcoGorelli · 2024-07-15T18:34:01Z

narwhals/_pandas_like/utils.py

-        if other._native_series.index is not index:
+        if (
+            other._native_series.index is not index
+            and other._implementation != Implementation.DASK
+        ):


what if other._native_series.index is not index and other._implementation is Implementation.DASK? I think we need to raise an error message in that case

narwhals/_pandas_like/utils.py

MarcoGorelli · 2024-07-15T18:35:42Z

narwhals/_pandas_like/utils.py

@@ -218,6 +298,9 @@ def set_axis(
        kwargs["copy"] = False
    else:  # pragma: no cover
        pass
+    if implementation == "dask":


MarcoGorelli · 2024-07-15T18:35:47Z

narwhals/_pandas_like/utils.py

@@ -449,6 +532,8 @@ def to_datetime(implementation: Implementation) -> Any:
        return get_modin().to_datetime
    if implementation is Implementation.CUDF:
        return get_cudf().to_datetime
+    if implementation == "dask":


narwhals/dependencies.py

MarcoGorelli · 2024-07-15T18:37:13Z

tests/utils.py

@@ -51,6 +57,17 @@ def maybe_get_modin_df(df_pandas: pd.DataFrame) -> Any:
            return mpd.DataFrame(df_pandas.to_dict(orient="list"))


+def maybe_get_dask_df(df_pandas: pd.DataFrame) -> Any:


where is this used?

benrutter · 2024-07-15T19:26:58Z

thanks @benrutter !

left some comments, there's some pre-commit checks to fixup too, but awesome work here

Thanks! Yeah just spotted precommit and it turned up a bunch of issues, I'll work through them plus your comments

for more information, see https://pre-commit.ci

benrutter · 2024-07-16T12:10:58Z

Ah quick update on this, I actually spotted that "get_dask()" (and get_modin()) obviously only imports dask if it's already imported. Which makes total sense, but also meant that dask wasn't being included in the new tests at all, since I hadn't added an initial import statement anywhere.

I've put one + an ImportError suppression in the conftest which has worked nicely (yaaay! 🙌) but that's also turned up a whole bunch of errors in the test.

I suspect its a mix of:

Stuff that's not supported in dask so the test just needs to be skippped
Stuff where I've not implemented something correctly / at all yet and just hadn't realised till now
Stuff where I've borked the implementation in the merge conflict

I'll work through, and the failing tests actually give me better confidence that the final integration should be reliable and nicely tested. Might take a little time though, let me know if it'd be easier for me to close off the PR while I work on it and reopen another later or not.

MarcoGorelli · 2024-07-16T12:29:13Z

thanks @benrutter for the update! up to you if you want to keep this PR or open a new one, whichever's easiest for you

MarcoGorelli · 2024-07-19T19:29:23Z

i'll work on updating this, would quite like to see it, but I think it should be back lazyframe, not dataframe?

EDIT ok, turns out this is going to be really complicated. putting it off for now, gonna see if I can catch up with a dask maintainer first

benrutter · 2024-07-20T07:06:00Z

Oh yikes! Thanks for taking a look, I think it might look worse than it is because some of the testing stuff like .to_dict isn't supported. Maybe I'm being naively optimistic 😅

I haven't taken a look at this for a little but but I'm hoping to spend some time hacking on it again soon.

for more information, see https://pre-commit.ci

benrutter · 2024-07-22T05:38:54Z

Closing off for now so that dask functionality can be implemented in line with #566 later.

I'll keep playing around with the fork though.

benrutter added 3 commits July 8, 2024 13:19

initial dask integration for test_common

eeaea89

series support added in

a05ee06

individual workarounds for dask differences

70a4144

MarcoGorelli reviewed Jul 11, 2024

View reviewed changes

narwhals/_pandas_like/series.py Outdated Show resolved Hide resolved

MarcoGorelli reviewed Jul 11, 2024

View reviewed changes

narwhals/_pandas_like/dataframe.py Outdated Show resolved Hide resolved

benrutter and others added 5 commits July 12, 2024 16:00

initial tidy up

638cfee

fixes

1d349ef

Merge branch 'main' into main

1d6ccab

[pre-commit.ci] auto fixes from pre-commit.com hooks

b2ab110

for more information, see https://pre-commit.ci

some fixes

f10c279

DeaMariaLeon changed the title ~~Dask Support Implementation (still draft, don't merge yet!)~~ feat: Dask Support Implementation (still draft, don't merge yet!) Jul 14, 2024

github-actions bot added the enhancement New feature or request label Jul 14, 2024

benrutter and others added 3 commits July 15, 2024 15:00

compute avoidance plus conflict-resolve-issue fix

dd15289

Merge branch 'main' into main

d3affbc

test logic updates post merge fix

7dac812

benrutter changed the title ~~feat: Dask Support Implementation (still draft, don't merge yet!)~~ feat: Dask Support Implementation Jul 15, 2024

[pre-commit.ci] auto fixes from pre-commit.com hooks

c727d1c

for more information, see https://pre-commit.ci

docstring update for stablev1

94b9dc7

MarcoGorelli reviewed Jul 15, 2024

View reviewed changes

type fixes

8eff3a4

benrutter and others added 2 commits July 15, 2024 21:47

more type fixes

30450ca

[pre-commit.ci] auto fixes from pre-commit.com hooks

fcd3253

for more information, see https://pre-commit.ci

benrutter added 2 commits July 16, 2024 11:12

type fixes and linting

87bea6d

test inclusion and some fixes

362f283

MarcoGorelli added 2 commits July 19, 2024 20:11

Merge remote-tracking branch 'upstream/main' into dask

26cbff2

fixup

2dc41f5

benrutter and others added 2 commits July 20, 2024 20:40

additional functionality for dask around exprs

c5f63a2

[pre-commit.ci] auto fixes from pre-commit.com hooks

c38311e

for more information, see https://pre-commit.ci

benrutter closed this Jul 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Dask Support Implementation #484

feat: Dask Support Implementation #484

benrutter commented Jul 10, 2024 •

edited

Loading

MarcoGorelli left a comment

benrutter commented Jul 11, 2024

benrutter commented Jul 12, 2024

MarcoGorelli commented Jul 12, 2024

benrutter commented Jul 12, 2024

benrutter commented Jul 15, 2024

MarcoGorelli commented Jul 15, 2024

benrutter commented Jul 15, 2024

MarcoGorelli left a comment

MarcoGorelli Jul 15, 2024

MarcoGorelli Jul 15, 2024

MarcoGorelli Jul 15, 2024

MarcoGorelli Jul 15, 2024

MarcoGorelli Jul 15, 2024

benrutter commented Jul 15, 2024

benrutter commented Jul 16, 2024

MarcoGorelli commented Jul 16, 2024

MarcoGorelli commented Jul 19, 2024 •

edited

Loading

benrutter commented Jul 20, 2024

benrutter commented Jul 22, 2024

		@@ -51,6 +57,17 @@ def maybe_get_modin_df(df_pandas: pd.DataFrame) -> Any:
		return mpd.DataFrame(df_pandas.to_dict(orient="list"))


		def maybe_get_dask_df(df_pandas: pd.DataFrame) -> Any:

feat: Dask Support Implementation #484

feat: Dask Support Implementation #484

Conversation

benrutter commented Jul 10, 2024 • edited Loading

What type of PR is this? (check all applicable)

Related issues

Checklist

If you have comments or can explain your changes, please do so below.

MarcoGorelli left a comment

Choose a reason for hiding this comment

benrutter commented Jul 11, 2024

benrutter commented Jul 12, 2024

MarcoGorelli commented Jul 12, 2024

benrutter commented Jul 12, 2024

benrutter commented Jul 15, 2024

MarcoGorelli commented Jul 15, 2024

benrutter commented Jul 15, 2024

MarcoGorelli left a comment

Choose a reason for hiding this comment

MarcoGorelli Jul 15, 2024

Choose a reason for hiding this comment

MarcoGorelli Jul 15, 2024

Choose a reason for hiding this comment

MarcoGorelli Jul 15, 2024

Choose a reason for hiding this comment

MarcoGorelli Jul 15, 2024

Choose a reason for hiding this comment

MarcoGorelli Jul 15, 2024

Choose a reason for hiding this comment

benrutter commented Jul 15, 2024

benrutter commented Jul 16, 2024

MarcoGorelli commented Jul 16, 2024

MarcoGorelli commented Jul 19, 2024 • edited Loading

benrutter commented Jul 20, 2024

benrutter commented Jul 22, 2024

benrutter commented Jul 10, 2024 •

edited

Loading

MarcoGorelli commented Jul 19, 2024 •

edited

Loading