chore: Support `@requires.backend_version` in namespaces by dangotbanned · Pull Request #3127 · narwhals-dev/narwhals

dangotbanned · 2025-09-12T22:12:04Z

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

Related issues

Checklist

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

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

- Towards #3116 (comment) - Typing is easy to fix

want `some.nope`, but got `nope` for now

#3116 (comment)

Yoinks from polars util

shorter in most cases

narwhals/_polars/typing.py

FBruzzesi

Thanks @dangotbanned - let's prioritize this.

There are a couple of relevant comments in _utils.py:

One regarding what to check to make sure something is an accessor
The other is a suggestion for how to split the _unwrap_context responsibilities

narwhals/_arrow/series_list.py

narwhals/_compliant/typing.py

narwhals/_utils.py

FBruzzesi · 2025-09-13T20:47:20Z

narwhals/_utils.py

+    def _unwrap_context(self, instance: _IntoContext) -> _FullContext:
+        if is_namespace_accessor(instance):
+            # NOTE: Should only need to do this once per class (the first time the method is called)
+            if "." not in self._wrapped_name:
+                self._wrapped_name = f"{instance._accessor}.{self._wrapped_name}"
+            return instance.compliant
+        return instance


I am not the biggest fan of this method. It tries to achieve two things:

qualify the method name

return either the instance or the instance.compliant, depending if we are in the accessor namespace or not.

I would keep the name clean up, but for how to access an element that has _backend_version and _implementation, I would prefer to move that in:

class NamespaceAccessor(_StoresCompliant[CompliantT_co], Protocol[CompliantT_co]): _accessor: ClassVar[Accessor] + @property + def _backend_version(self): + return self.compliant._backend_version + + @property + def _implementation(self): + return self.compliant._implementation

i.e. by simply forwarding attributes as properties

ooohh nice idea! 😍

It's pretty funny how I managed to do almost the exact same thing a while back

narwhals/narwhals/_compliant/series.py

Lines 269 to 286 in 1471cbb

class _SeriesNamespace( # type: ignore[misc]

_StoresCompliant[CompliantSeriesT_co],

_StoresNative[NativeSeriesT_co],

Protocol[CompliantSeriesT_co, NativeSeriesT_co],

):

_compliant_series: CompliantSeriesT_co

@property

def compliant(self) -> CompliantSeriesT_co:

return self._compliant_series

@property

def implementation(self) -> Implementation:

return self.compliant._implementation

@property

def backend_version(self) -> tuple[int, ...]:

return self.implementation._backend_version()

but seemed to fumble on this one 😂

i.e. by simply forwarding attributes as properties

@FBruzzesi oh how I wish that were true 😂

I split that into 2 commits:

refactor: NamespaceAccessor typing/protocol changes

refactor: @requires runtime changes

(#3131) is playing a part in the complexity. I chose to keep the properties shown in (#3127 (comment)) - as updating those would've created an even bigger diff 😳

But also CompliantT_co not having a bound=... means that self.compliant.<anything> is unsafe:

narwhals/narwhals/_utils.py

Line 165 in 1471cbb

CompliantT_co = TypeVar("CompliantT_co", covariant=True)

So I traded that out to reuse Implementation, which is still technically unsafe (#3132):

narwhals/narwhals/_utils.py

Line 166 in 6cc8ed0

CompliantT_co = TypeVar("CompliantT_co", bound="_StoresImplementation", covariant=True)

... but the alternative is adding _backend_version as a requirement to CompliantColumn - which I don't want to do

Sooooooo, while I agree with what you've said in (#3127 (comment)) - I'm finding it tricky to justify what it took to end up with (6cc8ed0) 🤔

WDYT?

Oh boy! I see the issue is #3131 not helping at all.

But if that's fixed, isn't _FullContext what we are looking for to bound CompliantT_co i.e. to implement both _implementation and _backend_version?

Also I would aim to decouple _unwrap_context into something like:

@@ -1936,23 +1936,19 @@ class requires: # noqa: N801 def _unparse_version(backend_version: tuple[int, ...], /) -> str: return ".".join(f"{d}" for d in backend_version) - def _unwrap_context( - self, instance: _IntoContext - ) -> tuple[Implementation, tuple[int, ...]]: - if is_namespace_accessor(instance): + def _qualify_wrapped_name(self, instance: _IntoContext) -> None: + if is_namespace_accessor(instance) and "." not in self._wrapped_name: # NOTE: Should only need to do this once per class (the first time the method is called) - if "." not in self._wrapped_name: - self._wrapped_name = f"{instance._accessor}.{self._wrapped_name}" - return instance.implementation, instance.backend_version - return instance._implementation, instance._backend_version + self._wrapped_name = f"{instance._accessor}.{self._wrapped_name}" def _ensure_version(self, instance: _IntoContext, /) -> None: - backend, version = self._unwrap_context(instance) - if version >= self._min_version: + self._qualify_wrapped_name(instance) + if (version := instance._backend_version) >= self._min_version: return method = self._wrapped_name minimum = self._unparse_version(self._min_version) found = self._unparse_version(version) + backend = instance._implementation

I have a commit almost ready, I will push that and see how it looks like

Nevermind, I guess this is another main blocker for you?!

.. but the alternative is adding _backend_version as a requirement to CompliantColumn - which I don't want to do

ah yeah, this issue is relevant since it traces-back to where I removed _backend_version 😅

Transition to single-Implementation classes #3059

But I guess the short version is _backend_version is not something we need an extender of narwhals to provide.
We won't be branching on it - so requiring it didn't make sense to me

@FBruzzesi I think this is as close as I can get to (#3127 (comment))

(refactor: try a different way)

... while avoiding the bigger changes for now 😉

I just wanted to help out with #3116

Hey @dangotbanned thanks for bearing with me, I wanted to get back to you earlier but I couldn't.

Another thought I had was something you also mentioned: namely all we need is _StoreImplementation since we can do:

impl = self._implementation version = impl.backend_version()

I have mixed feelings at the moment for both solutions, but my understanding was that you eventually wanted to move towards this latter suggestion (I hope I didn't get that wrong).

thanks for bearing with me, I wanted to get back to you earlier but I couldn't.

No worries @FBruzzesi, I took longer to respond 😉

Another thought I had was something you also mentioned: namely all we need is _StoreImplementation since we can do:

Yeah that was the direction I went in with 4a8764#diff
and is what I've been trying to move towards recently e.g:

narwhals/narwhals/_utils.py

Lines 2097 to 2098 in 7b271eb

class Compliant(

_StoresNative[NativeT_co], _StoresImplementation, Protocol[NativeT_co]

narwhals/narwhals/_utils.py

Lines 241 to 250 in 7b271eb

class ValidateBackendVersion(_StoresImplementation, Protocol):

"""Ensure the target `Implementation` is on a supported version."""

def _validate_backend_version(self) -> None:

"""Raise if installed version below `nw._utils.MIN_VERSIONS`.

**Only use this when moving between backends.**

Otherwise, the validation will have taken place already.

"""

_ = self._implementation._backend_version()

Both of the *Context protocols also depend on it

narwhals/narwhals/_utils.py

Line 225 in 7b271eb

class _LimitedContext(_StoresImplementation, _StoresVersion, Protocol):

narwhals/narwhals/_utils.py

Line 233 in 7b271eb

class _FullContext(_StoresImplementation, _StoresBackendVersion, Protocol):

but my understanding was that you eventually wanted to move towards this latter suggestion (I hope I didn't get that wrong).

Absolutely right! 😄

To clarify (#3127 (comment)) some more ...

I want to tackle it holistically as part of this meta issue:

Tracking: Making Narwhals extensible #2785

Currently, there are a few related issues and I don't wanna make a decision here for the benefit of only #3116

Show screenshot

Everything **but** the runtime change to `requires._unwrap_context` #3127 (comment)

dangotbanned · 2025-09-14T14:39:48Z

narwhals/_compliant/any_namespace.py

    "DateTimeNamespace",
    "ListNamespace",
    "NameNamespace",
+    "NamespaceAccessor",


Forgot to mention!

I chose to invert the naming because we've kinda overloaded the term Namespace

from narwhals._arrow.namespace import ArrowNamespace from narwhals._compliant.any_namespace import ( NameNamespace, # AccessorNamespace, NamespaceAccessor, StringNamespace, StructNamespace, ) from narwhals._compliant.expr import EagerExprCatNamespace from narwhals._compliant.namespace import CompliantNamespace from narwhals._namespace import Namespace

This reverts commit 6cc8ed0.

This reverts commit 4a87646.

Along the lines of (#3127 (comment))

narwhals/_polars/expr.py

`Any` was the wrong choice (explanation to follow!)

Inlined `_hasattr_static` to avoid creating lots of sentinels

FBruzzesi · 2025-09-18T09:09:32Z

@dangotbanned I think this is ready to merge - I opened a PR (#3136) to pin duckdb when we also run ibis, and get confidence from tests back.

dangotbanned · 2025-09-18T11:11:28Z

@dangotbanned I think this is ready to merge - I opened a PR (#3136) to pin duckdb when we also run ibis, and get confidence from tests back.

Thanks @FBruzzesi!

Feel free to merge that and then this one whenever you're ready 🥳

FBruzzesi

Thanks for bearing with me on this one @dangotbanned - I am also looking forward to getting rid of some of the tech debt here

dangotbanned added 6 commits September 12, 2025 19:52

test: Add failing tests

8a0b9e0

- Towards #3116 (comment) - Typing is easy to fix

feat: Everything but the method name working

f80a83c

want `some.nope`, but got `nope` for now

refactor: Remove no-longer-required _version

fbbe1b1

#3116 (comment)

feat: Add _accessor class var

3009d8e

Yoinks from polars util

refactor: Do the same thing as pandas_like

c926ca8

shorter in most cases

feat: prefix method name

ccdd735

dangotbanned added error reporting developer tools labels Sep 12, 2025

dangotbanned mentioned this pull request Sep 12, 2025

feat: Add support for {Expr,Series}.str.to_titlecase #3116

Merged

10 tasks

dangotbanned commented Sep 12, 2025

View reviewed changes

narwhals/_polars/typing.py Show resolved Hide resolved

dangotbanned marked this pull request as ready for review September 12, 2025 22:19

dangotbanned requested a review from FBruzzesi September 12, 2025 22:19

Merge branch 'main' into @requires-on-accessors

4b79c04

FBruzzesi reviewed Sep 13, 2025

View reviewed changes

This was referenced Sep 14, 2025

Rename _implementation, _backend_version -> implementation, backend_version #3131

Open

Make Implementation.UNKNOWN._backend_version() safe #3132

Closed

dangotbanned added 2 commits September 14, 2025 13:53

refactor: NamespaceAccessor typing/protocol changes

4a87646

Everything **but** the runtime change to `requires._unwrap_context` #3127 (comment)

refactor: @requires runtime changes

6cc8ed0

dangotbanned commented Sep 14, 2025

View reviewed changes

dangotbanned and others added 6 commits September 15, 2025 15:44

revert "refactor: @requires runtime changes"

6854043

This reverts commit 6cc8ed0.

revert "refactor: NamespaceAccessor typing/protocol changes"

cfea56b

This reverts commit 4a87646.

refactor: try a different way

7b271eb

Along the lines of (#3127 (comment))

Merge branch 'main' into @requires-on-accessors

456545f

use it for PolarsExpr.str.zfill

00af1a4

change check in is_namespace_accessor

88e9e9f

dangotbanned commented Sep 17, 2025

View reviewed changes

narwhals/_polars/expr.py Show resolved Hide resolved

dangotbanned added 4 commits September 17, 2025 11:46

chore(typing): Narrow what can be passed to is_namespace_accessor

a52beb2

`Any` was the wrong choice (explanation to follow!)

refactor: Make super private

72a6e0f

chore: make the runtime check safer

32a211b

Inlined `_hasattr_static` to avoid creating lots of sentinels

perf: Move optimization into _hasattr_static

02c4984

docs: Leave a trail for "why so strict dude?"

7906b15

Merge branch 'main' into @requires-on-accessors

75da19e

FBruzzesi approved these changes Sep 18, 2025

View reviewed changes

dangotbanned merged commit e3133de into main Sep 18, 2025
29 of 31 checks passed

dangotbanned deleted the @requires-on-accessors branch September 18, 2025 14:06

	class _SeriesNamespace( # type: ignore[misc]
	_StoresCompliant[CompliantSeriesT_co],
	_StoresNative[NativeSeriesT_co],
	Protocol[CompliantSeriesT_co, NativeSeriesT_co],
	):
	_compliant_series: CompliantSeriesT_co

	@property
	def compliant(self) -> CompliantSeriesT_co:
	return self._compliant_series

	@property
	def implementation(self) -> Implementation:
	return self.compliant._implementation

	@property
	def backend_version(self) -> tuple[int, ...]:
	return self.implementation._backend_version()

	class Compliant(
	_StoresNative[NativeT_co], _StoresImplementation, Protocol[NativeT_co]

	class ValidateBackendVersion(_StoresImplementation, Protocol):
	"""Ensure the target `Implementation` is on a supported version."""

	def _validate_backend_version(self) -> None:
	"""Raise if installed version below `nw._utils.MIN_VERSIONS`.

	Only use this when moving between backends.
	Otherwise, the validation will have taken place already.
	"""
	_ = self._implementation._backend_version()

Conversation

dangotbanned commented Sep 12, 2025

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

Uh oh!

Uh oh!

FBruzzesi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangotbanned Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangotbanned Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FBruzzesi commented Sep 18, 2025

Uh oh!

dangotbanned commented Sep 18, 2025

Uh oh!

FBruzzesi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dangotbanned Sep 15, 2025 •

edited

Loading

dangotbanned Sep 16, 2025 •

edited

Loading