Skip to content

chore: remove more kwargs from dask, dont treat ddof as expressifiable arg #2059

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

Merged
MarcoGorelli merged 5 commits into from
Feb 21, 2025
Merged

chore: remove more kwargs from dask, dont treat ddof as expressifiable arg #2059

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ed04d0
chore: remove more `kwargs` from dask, dont treat `ddof` as expressif…
MarcoGorelli Feb 20, 2025
52b80b2
why complicate things when they can be simpler
MarcoGorelli Feb 20, 2025
bd235f3
simplify
MarcoGorelli Feb 21, 2025
568b55b
fixup
MarcoGorelli Feb 21, 2025
565cb8d
Merge branch 'main' into more-kwargs
dangotbanned Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions narwhals/_dask/expr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ def __init__(
alias_output_names: Callable[[Sequence[str]], Sequence[str]] | None,
backend_version: tuple[int, ...],
version: Version,
kwargs: dict[str, Any],
# Kwargs with metadata which we may need in group-by agg
# (e.g. `ddof` for `std` and `var`).
kwargs: dict[str, Any] | None = None,
) -> None:
self._call = call
self._depth = depth
Expand All @@ -57,7 +59,7 @@ def __init__(
self._alias_output_names = alias_output_names
self._backend_version = backend_version
self._version = version
self._kwargs = kwargs
self._kwargs = kwargs or {}

def __call__(self: Self, df: DaskLazyFrame) -> Sequence[dx.Series]:
return self._call(df)
Expand Down Expand Up @@ -110,7 +112,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=None,
backend_version=backend_version,
version=version,
kwargs={},
)

@classmethod
Expand All @@ -133,14 +134,14 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=None,
backend_version=backend_version,
version=version,
kwargs={},
)

def _from_call(
self: Self,
# First argument to `call` should be `dx.Series`
call: Callable[..., dx.Series],
expr_name: str,
kwargs: dict[str, Any] | None = None,

@EdAbati EdAbati Feb 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What if we call this call_kwargs i.e. kwargs for the call function ?

(Just to clarify these kwargs are not passed to _from_call itself)

@dangotbanned dangotbanned Feb 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@EdAbati Couldn't you just pass call as a partial in the cases where this is needed?

  • Avoids needing this parameter entirely
  • "binding" of arguments is done once, instead of on each use
  • Also supports positional arguments

So instead of call, call_kwargs - you'd have call.func, call.keywords.
But the keywords themselves aren't exposed here

@MarcoGorelli MarcoGorelli Feb 21, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did not know that functools.partial stored keywords like that, thanks!

@dangotbanned dangotbanned Feb 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did not know that functools.partial stored keywords like that, thanks!

You might need to use operator.methodcaller instead, if you don't know the class that would be passed.

E.g. partial would work if you could do partial(dx.Series.std, ddof=ddof).
But where it might be dx._groupby.GroupBy.std, using the method name would be more widely accepted

narwhals/narwhals/_dask/group_by.py

Lines 54 to 55 in 4dda548

def std(ddof: int = 1) -> _AggFn:
return partial(_DaskGroupBy.std, ddof=ddof)

@dangotbanned dangotbanned Feb 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if you don't know the class that would be passed.

Is that why you're storing function_name, in addition to the lambda?

@MarcoGorelli MarcoGorelli Feb 21, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yup, that's right

**expressifiable_args: Self | Any,
) -> Self:
def func(df: DaskLazyFrame) -> list[dx.Series]:
Expand All @@ -163,7 +164,7 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=self._alias_output_names,
backend_version=self._backend_version,
version=self._version,
kwargs={**self._kwargs, **expressifiable_args},
kwargs=kwargs,
)

def alias(self: Self, name: str) -> Self:
Expand Down Expand Up @@ -310,12 +311,16 @@ def max(self: Self) -> Self:

def std(self: Self, ddof: int) -> Self:
return self._from_call(
lambda _input, ddof: _input.std(ddof=ddof).to_series(), "std", ddof=ddof
lambda _input: _input.std(ddof=ddof).to_series(),
"std",
kwargs={"ddof": ddof},
)
Comment on lines 312 to 317

@dangotbanned dangotbanned Feb 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Related to #2059 (comment)

and

https://github.com/MarcoGorelli/narwhals/blob/52b80b240f0fc0e7eb069a84f0ee001f218bedaf/narwhals/_dask/expr.py#L51-L53

        # Kwargs with metadata which we may need in group-by agg
        # (e.g. `ddof` for `std` and `var`).
        kwargs: dict[str, Any] | None = None,

I'm not following why kwargs is used at all here:

narwhals/narwhals/_dask/group_by.py

Lines 54 to 55 in 4dda548

def std(ddof: int = 1) -> _AggFn:
return partial(_DaskGroupBy.std, ddof=ddof)

narwhals/narwhals/_dask/group_by.py

Lines 148 to 150 in 4dda548

kwargs: dict[str, Any] = (
{"ddof": expr._kwargs["ddof"]} if function_name in {"std", "var"} else {} # type: ignore[attr-defined]
)

Am I reading this wrong, or are the same kwargs applied twice?


def var(self: Self, ddof: int) -> Self:
return self._from_call(
lambda _input, ddof: _input.var(ddof=ddof).to_series(), "var", ddof=ddof
lambda _input: _input.var(ddof=ddof).to_series(),
"var",
kwargs={"ddof": ddof},
)

def skew(self: Self) -> Self:
Expand Down
19 changes: 2 additions & 17 deletions narwhals/_dask/namespace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=None,
backend_version=self._backend_version,
version=self._version,
kwargs={},
)

def col(self: Self, *column_names: str) -> DaskExpr:
Expand Down Expand Up @@ -93,7 +92,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=None,
backend_version=self._backend_version,
version=self._version,
kwargs={},
)

def len(self: Self) -> DaskExpr:
Expand All @@ -116,7 +114,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=None,
backend_version=self._backend_version,
version=self._version,
kwargs={},
)

def all_horizontal(self: Self, *exprs: DaskExpr) -> DaskExpr:
Expand All @@ -134,7 +131,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=combine_alias_output_names(*exprs),
backend_version=self._backend_version,
version=self._version,
kwargs={"exprs": exprs},
)

def any_horizontal(self: Self, *exprs: DaskExpr) -> DaskExpr:
Expand All @@ -152,7 +148,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=combine_alias_output_names(*exprs),
backend_version=self._backend_version,
version=self._version,
kwargs={"exprs": exprs},
)

def sum_horizontal(self: Self, *exprs: DaskExpr) -> DaskExpr:
Expand All @@ -170,7 +165,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=combine_alias_output_names(*exprs),
backend_version=self._backend_version,
version=self._version,
kwargs={"exprs": exprs},
)

def concat(
Expand Down Expand Up @@ -253,7 +247,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=combine_alias_output_names(*exprs),
backend_version=self._backend_version,
version=self._version,
kwargs={"exprs": exprs},
)

def min_horizontal(self: Self, *exprs: DaskExpr) -> DaskExpr:
Expand All @@ -272,7 +265,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=combine_alias_output_names(*exprs),
backend_version=self._backend_version,
version=self._version,
kwargs={"exprs": exprs},
)

def max_horizontal(self: Self, *exprs: DaskExpr) -> DaskExpr:
Expand All @@ -291,7 +283,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=combine_alias_output_names(*exprs),
backend_version=self._backend_version,
version=self._version,
kwargs={"exprs": exprs},
)

def when(self: Self, predicate: DaskExpr) -> DaskWhen:
Expand Down Expand Up @@ -342,11 +333,6 @@ def func(df: DaskLazyFrame) -> list[dx.Series]:
alias_output_names=getattr(exprs[0], "_alias_output_names", None),
backend_version=self._backend_version,
version=self._version,
kwargs={
"exprs": exprs,
"separator": separator,
"ignore_nulls": ignore_nulls,
},
)


Expand Down Expand Up @@ -401,7 +387,6 @@ def then(self: Self, value: DaskExpr | Any) -> DaskThen:
alias_output_names=getattr(value, "_alias_output_names", None),
backend_version=self._backend_version,
version=self._version,
kwargs={"value": value},
)


Expand All @@ -416,7 +401,7 @@ def __init__(
alias_output_names: Callable[[Sequence[str]], Sequence[str]] | None,
backend_version: tuple[int, ...],
version: Version,
kwargs: dict[str, Any],
kwargs: dict[str, Any] | None = None,
) -> None:
self._backend_version = backend_version
self._version = version
Expand All @@ -425,7 +410,7 @@ def __init__(
self._function_name = function_name
self._evaluate_output_names = evaluate_output_names # pyright: ignore[reportAttributeAccessIssue]
self._alias_output_names = alias_output_names
self._kwargs = kwargs
self._kwargs = kwargs or {}

def otherwise(self: Self, value: DaskExpr | Any) -> DaskExpr:
# type ignore because we are setting the `_call` attribute to a
Expand Down
2 changes: 0 additions & 2 deletions narwhals/_dask/selectors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ def _to_expr(self: Self) -> DaskExpr:
alias_output_names=self._alias_output_names,
backend_version=self._backend_version,
version=self._version,
kwargs={},
)

def __sub__(self: Self, other: DaskSelector | Any) -> DaskSelector | Any:
Expand Down Expand Up @@ -226,5 +225,4 @@ def selector(
alias_output_names=None,
backend_version=context._backend_version,
version=context._version,
kwargs={},
)
6 changes: 2 additions & 4 deletions narwhals/_pandas_like/expr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def __init__(
implementation: Implementation,
backend_version: tuple[int, ...],
version: Version,
kwargs: dict[str, Any],
kwargs: dict[str, Any] | None = None,
) -> None:
self._call = call
self._depth = depth
Expand All @@ -68,7 +68,7 @@ def __init__(
self._implementation = implementation
self._backend_version = backend_version
self._version = version
self._kwargs = kwargs
self._kwargs = kwargs or {}

def __call__(self: Self, df: PandasLikeDataFrame) -> Sequence[PandasLikeSeries]:
return self._call(df)
Expand Down Expand Up @@ -145,7 +145,6 @@ def func(df: PandasLikeDataFrame) -> list[PandasLikeSeries]:
implementation=implementation,
backend_version=backend_version,
version=version,
kwargs={},
)

@classmethod
Expand Down Expand Up @@ -176,7 +175,6 @@ def func(df: PandasLikeDataFrame) -> list[PandasLikeSeries]:
implementation=implementation,
backend_version=backend_version,
version=version,
kwargs={},
)

def cast(self: Self, dtype: DType | type[DType]) -> Self:
Expand Down
4 changes: 0 additions & 4 deletions narwhals/_pandas_like/namespace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ def _create_expr_from_series(self: Self, series: PandasLikeSeries) -> PandasLike
implementation=self._implementation,
backend_version=self._backend_version,
version=self._version,
kwargs={},
)

def _create_compliant_series(self: Self, value: Any) -> PandasLikeSeries:
Expand Down Expand Up @@ -139,7 +138,6 @@ def all(self: Self) -> PandasLikeExpr:
implementation=self._implementation,
backend_version=self._backend_version,
version=self._version,
kwargs={},
)

def lit(self: Self, value: Any, dtype: DType | None) -> PandasLikeExpr:
Expand All @@ -165,7 +163,6 @@ def _lit_pandas_series(df: PandasLikeDataFrame) -> PandasLikeSeries:
implementation=self._implementation,
backend_version=self._backend_version,
version=self._version,
kwargs={},
)

def len(self: Self) -> PandasLikeExpr:
Expand All @@ -187,7 +184,6 @@ def len(self: Self) -> PandasLikeExpr:
implementation=self._implementation,
backend_version=self._backend_version,
version=self._version,
kwargs={},
)

# --- horizontal ---
Expand Down
1 change: 0 additions & 1 deletion narwhals/_pandas_like/selectors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,5 +218,4 @@ def selector(
implementation=context._implementation,
backend_version=context._backend_version,
version=context._version,
kwargs={},
)