Skip to content

chore: @requires for backend method constraints #2371

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.

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Apr 11, 2025

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

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

  • Related issue #<issue number>
  • Closes #<issue number>

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

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

Adds a method decorator for consistent error messages on unsupported versions

Main use case is for enforcing a minimum version, e.g.:

class PolarsExpr:
    @requires(min_version=(1,))
    def rolling_var(
        self: Self, window_size: int, *, min_samples: int, center: bool, ddof: int
    ) -> Self: ...

Still have some other cases I wanna explore:

  • Providing a hint (PolarsNamespace.nth)

Possible follow-ups

PandasLike*

.pivot is a good example of multiple cases dependent on more than just backend_version:

def pivot(
self: Self,
on: list[str],
*,
index: list[str] | None,
values: list[str] | None,
aggregate_function: Any | None,
sort_columns: bool,
separator: str,
) -> Self:
if self._implementation is Implementation.PANDAS and (
self._backend_version < (1, 1)
): # pragma: no cover
msg = "pivot is only supported for pandas>=1.1"
raise NotImplementedError(msg)
if self._implementation is Implementation.MODIN:
msg = "pivot is not supported for Modin backend due to https://github.com/modin-project/modin/issues/7409."
raise NotImplementedError(msg)

Parameter combinations

I'd only want to include the version-based parameter restrictions on @requires:

  • DuckDBLazyFrame.join(how="cross")
  • DuckDBExpr.broadcast(kind=ExprKind.AGGREGATION)
  • PolarsDataFrame.__array__(copy=...)
  • PolarsExpr.over(order_by=...)

It seems there are more blanket parameter restrictions, but they don't make sense as a @requires:

  • DaskExpr.cum_*(reverse=True)
  • DaskExpr.rolling_*(ddof=2) (or not 1)
  • (Dask|DuckDB)Expr.quantile(interpolation="(nearest|higher|lower|midpoint)")
  • DuckDBLazyFrame.join_asof(strategy="nearest")
  • (DuckDB|SparkLike)Expr.fill_null(strategy=...)
  • (DuckDB|SparkLike)Expr.rank(method="(average|max|ordinal)")
  • DuckDBExprStringNamespace.to_datetime(format=None)

How to document this?

Like with not_implemented, we could mark the methods with runtime metadata. Then consume that in a script for something like https://narwhals-dev.github.io/narwhals/api-completeness/

@dangotbanned
Copy link
Member Author

dangotbanned commented Apr 13, 2025

Been think about an alternative spelling which could be more easily adapted for other requirements.

Imports

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from typing_extensions import Self

    from narwhals._polars.expr import PolarsExpr

Current

Decorator constructor

class requires:  # noqa: N801
    def __init__(self, *, min_version: tuple[int, ...], hint: str = "") -> None:
        self._min_version: tuple[int, ...] = min_version
        self._hint: str = hint
        self._wrapped_name: str

Usage

class DuckDBExpr:
    @requires(min_version=(1, 3))
    def is_first_distinct(self) -> Self: ...

class PolarsNamespace:
    @requires(
        min_version=(1, 0, 0), hint="Please use `col` for columns selection instead."
    )
    def nth(self, *indices: int) -> PolarsExpr: ...

Alternative

Would be pretty easy to add support for multi-implementation cases this way - without it affecting the more common usage.

I also like that we can stay aligned with the backend_version name, while still indicating it is a minimum

Decorator constructor

See (651cf2f)

Code block

class requires:  # noqa: N801
    @classmethod
    def backend_version(cls, min_version: tuple[int, ...], /, hint: str = "") -> Self:
        obj = cls.__new__(cls)
        obj._min_version = min_version
        obj._hint = hint
        return obj

Usage

class DuckDBExpr:
    @requires.backend_version((1, 3))
    def is_first_distinct(self) -> Self: ...

class PolarsNamespace:
    @requires.backend_version(
        (1, 0, 0), "Please use `col` for columns selection instead."
    )
    def nth(self, *indices: int) -> PolarsExpr: ...

@dangotbanned dangotbanned mentioned this pull request Apr 16, 2025
10 tasks
@dangotbanned dangotbanned changed the title chore(DRAFT): @requires for backend method constraints chore: @requires for backend method constraints Apr 16, 2025
I want to move all uses over to this style, but open to feedback in review πŸ™‚
#2371 (comment)
Comment on lines +1828 to +1842
@classmethod
def backend_version(cls, minimum: tuple[int, ...], /, hint: str = "") -> Self:
"""Method decorator for raising below a minimum `_backend_version`.
Arguments:
minimum: Minimum backend version.
hint: Optional suggested alternative.
Returns:
An exception-raising decorator.
"""
obj = cls.__new__(cls)
obj._min_version = minimum
obj._hint = hint
return obj
Copy link
Member Author

@dangotbanned dangotbanned Apr 16, 2025

Choose a reason for hiding this comment

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

I want to switch everything over to this style, see for comparison (#2371 (comment))

Also the one current use for coverage (0ac9595)

@dangotbanned dangotbanned marked this pull request as ready for review April 16, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant