fix(typing): Narrow `TypeVar` used in `Series` by dangotbanned · Pull Request #2347 · narwhals-dev/narwhals

dangotbanned · 2025-04-04T18:34:53Z

Will close #2343

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

Related issues

Closes Narrow TypeVar used in Series #2343

Checklist

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

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

Changing IntoSeries gets us most of the way there
Settled on a bit of a creative solution

Part of (#2343)

Still need to fix `from_native`

dangotbanned · 2025-04-04T18:38:46Z

tests/translate/from_native_test.py

+        # TODO @dangotbanned: Fix this one
+        # Current:
+        #   `unstable_nw.Series[unstable_nw.Series[pl.Series]]``
+        # Goal:
+        #   `unstable_nw.Series[pl.Series]`
+        assert_type(nw_series_passthrough, unstable_nw.Series[pl.Series])


NOTE

What we need is for from_native to return the same type on these branches

narwhals/narwhals/translate.py

Lines 387 to 391 in d2a9f42

# Early returns

if isinstance(native_object, (DataFrame, LazyFrame)) and not series_only:

return native_object

if isinstance(native_object, Series) and (series_only or allow_series):

return native_object

Right now, this introduces a level of nesting that gets silly quickly

Type of "nw_series_nest_2" is "narwhals.series.Series[narwhals.series.Series[narwhals.series.Series[polars.series.series.Series]]]"

Ignore mypy (https://github.com/narwhals-dev/narwhals/actions/runs/14271920405/job/40006873171?pr=2347)

I'm working on resolving this one

(6bd47b4) is a very heavy-handed fix.

My thinking is

Passing a narwhals object to from_native really should never have been supported

None of these are native types: nw.Series | nw.DataFrame | nw.LazyFrame

Removing that support would likely break someones workflow

Ignoring all flags requires only adding 1 new @overload per-class

None of those need to deal with recursion

Was expecting the v1 backport (253da29) would surface some downstream issues - but all seems fine 🤔

dangotbanned · 2025-04-04T19:13:59Z

narwhals/typing.py

    class NativeLazyFrame(NativeFrame, Protocol):
        def explain(self, *args: Any, **kwargs: Any) -> Any: ...

+    # TODO @dangotbanned: `nw.Series` **cannot** be allowed to match this!!!


NativeSeries is now what is used for IntoSeries.

An issue - that's new to me - is that Series also matches NativeSeries 🤦‍♂️

So for the original alias:

IntoSeries: TypeAlias = Union["Series[Any]", "NativeSeries"]

nw.Series can be a match in 3 positions

IntoSeries: TypeAlias = Union["Series[Any]", "NativeSeries"] # ^--1--^ ^2^ ^-----3-----^

@dangotbanned literally an hour ago (#2347)

Should be the easiest to fix in (#2239)

🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️

dangotbanned · 2025-04-04T21:26:33Z

narwhals/translate.py

+@overload
+def from_native(native_object: SeriesT, **kwds: Any) -> SeriesT: ...


But why does this work?

By having an unconditional @overload when a nw.Series is passed - it ensures no other @overload(s) are checked.

If we were to spell out all the valid cases, they would overlap with the NativeSeries cases (see #2347 (comment))

narwhals.from_native(native_series, allow_series=True) narwhals.from_native(native_series, series_only=True)

Needing to have **kwds is unfortunate - but

practicality beats purity

I'm open to any other solutions that can pass the new tests 🙂

shall we raise at runtime if kwds?

shall we raise at runtime if kwds?

Yeah I was thinking about that as well!

I wasn't sure

what the message should be?

could that logic be shared for both v1 and main?

@MarcoGorelli resolved in:

(124239a)

(8f6d90a)

(fix(typing): Narrow TypeVar used in Series #2347 (comment))

MarcoGorelli

thanks @dangotbanned ! just one comment

MarcoGorelli · 2025-04-05T08:00:20Z

narwhals/translate.py

+@overload
+def from_native(native_object: SeriesT, **kwds: Any) -> SeriesT: ...


shall we raise at runtime if kwds?

#2347 (comment)

dangotbanned · 2025-04-05T10:51:52Z

narwhals/translate.py

+    if kwds:
+        msg = f"from_native() got an unexpected keyword argument {next(iter(kwds))!r}"
+        raise TypeError(msg)


Important
This matches the runtime behavior of python.

import polars as pl import narwhals as nw df = pl.DataFrame({"a": ["A", "B", "A"]}) nw_df = nw.from_native(df) nw.to_native(nw_df, keyword_1="a", keyword_2="b")

Only the first bad argument is ever included:

--------------------------------------------------------------------------- TypeError Traceback (most recent call last) Cell In[5], line 39 37 df = pl.DataFrame({"a": ["A", "B", "A"]}) 38 nw_df = nw.from_native(df) ---> 39 nw.to_native(nw_df, keyword_1="a", keyword_2="b") TypeError: to_native() got an unexpected keyword argument 'keyword_1'

MarcoGorelli

thanks @dangotbanned !

dangotbanned added 2 commits April 4, 2025 19:30

fix(typing): Make IntoSeries non-recursive

74a03a8

Part of (#2343)

test: Add test_series_recursive

ff841d1

Still need to fix `from_native`

dangotbanned added fix typing labels Apr 4, 2025

dangotbanned commented Apr 4, 2025

View reviewed changes

chore(DRAFT): Add todo

37549cc

dangotbanned commented Apr 4, 2025

View reviewed changes

dangotbanned added 4 commits April 4, 2025 20:41

fix(typing): Try a more aggressive fix

6bd47b4

test: tidy up

50e4d13

fix(typing): v1 backport

253da29

remove todo

51aaa8d

dangotbanned commented Apr 4, 2025

View reviewed changes

dangotbanned marked this pull request as ready for review April 4, 2025 21:27

MarcoGorelli reviewed Apr 5, 2025

View reviewed changes

dangotbanned added 3 commits April 5, 2025 10:56

Merge remote-tracking branch 'upstream/main' into narrow-series-type-var

77565ed

fix: Add runtime error for unknown **kwds

124239a

#2347 (comment)

test: Add test_from_native_invalid_keywords

8f6d90a

dangotbanned commented Apr 5, 2025

View reviewed changes

This was referenced Apr 5, 2025

API: io functions for v2 #2116

Open

test(typing): Narrow Constructor, ConstructorEager aliases #2349

Merged

Merge branch 'main' into narrow-series-type-var

83ca244

dangotbanned requested a review from MarcoGorelli April 6, 2025 21:15

MarcoGorelli approved these changes Apr 7, 2025

View reviewed changes

MarcoGorelli merged commit e565223 into main Apr 7, 2025
27 of 28 checks passed

MarcoGorelli deleted the narrow-series-type-var branch April 7, 2025 10:55

	# Early returns
	if isinstance(native_object, (DataFrame, LazyFrame)) and not series_only:
	return native_object
	if isinstance(native_object, Series) and (series_only or allow_series):
	return native_object

		@overload
		def from_native(native_object: SeriesT, **kwds: Any) -> SeriesT: ...

Conversation

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

Uh oh!

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!

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

Uh oh!

Choose a reason for hiding this comment

NOTE

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

But why does this work?

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!

MarcoGorelli left a comment

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!

MarcoGorelli 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 commented Apr 4, 2025 •

edited

Loading

dangotbanned Apr 4, 2025 •

edited

Loading