-
Notifications
You must be signed in to change notification settings - Fork 134
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
feat(typing): Backport generic Series
to v1
#2110
Conversation
That now would trigger **15** `[var-annotated]` warnings in our tests: ```py tests/utils_test.py:39: error: Need type annotation for "s" [var-annotated] s = nw.from_native(pd.Series([1, 2, 3], index=[2, 1, 0]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/utils_test.py:65: error: Need type annotation for "s" [var-annotated] s = nw.from_native(pd.Series([1, 2, 3], index=[2, 2, 0]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/utils_test.py:72: error: Need type annotation for "s" [var-annotated] s = nw.from_native(pl.Series([1, 2, 3]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/utils_test.py:171: error: Need type annotation for "index" [var-annotated] index = nw.from_native(pd.Series([0, 1, 2]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/utils_test.py:198: error: Need type annotation for "series" [var-annotated] series = nw.from_native(pl.Series([1, 2, 3]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/utils_test.py:215: error: Need type annotation for "pandas_series" [var-annotated] pandas_series = nw.from_native( ^ tests/utils_test.py:232: error: Need type annotation for "series" [var-annotated] series = nw.from_native(pl.Series([1, 2, 3]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/stable_api_test.py:136: error: Need type annotation for "stable_df" [var-annotated] stable_df = nw_v1.from_native(pl.Series(), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/dtypes_test.py:152: error: Need type annotation for "result" [var-annotated] result = nw.from_native(s, series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/dtypes_test.py:182: error: Need type annotation for "snw" [var-annotated] snw = nw.from_native(s, series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/translate/from_native_test.py:140: error: Need type annotation for "res" [var-annotated] res = nw.from_native(obj, series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/series_only/hist_test.py:277: error: Need type annotation for "s" [var-annotated] s = nw.from_native(pl.Series([1, 2, 3]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/series_only/arrow_c_stream_test.py:18: error: Need type annotation for "s" [var-annotated] s = nw.from_native(pl.Series([1, 2, 3]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/series_only/arrow_c_stream_test.py:31: error: Need type annotation for "s" [var-annotated] s = nw.from_native(pl.Series([1, 2, 3]), series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tests/expr_and_series/cast_test.py:172: error: Need type annotation for "s" [var-annotated] s = nw.from_native(s_pd, series_only=True) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Found 15 errors in 7 files (checked 345 source files) ```
with pytest.raises(TypeError, match="Cannot only use `series_only`"): | ||
nw.from_native(df, series_only=True) | ||
nw.from_native(df, series_only=True) # pyright: ignore[reportArgumentType, reportCallIssue] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needing this ignore makes me think I'm onto something with (daa4a17)
I can just fix this on the Pretty sure it would be working if I change Update 1Following (vega/altair#3811), this warning for But I can't add an ignore in altair/utils/schemapi.py:497: error: No overload variant of "from_native"
matches argument types "Iterable[Any]", "bool" [call-overload]
ser = nw.from_native(obj, series_only=True)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
altair/utils/schemapi.py:497: note: Possible overload variants: If anything - this is demonstrating the def _from_array_like(obj: Iterable[Any], /) -> list[Any]:
try:
ser = nw.from_native(obj, series_only=True)
return ser.to_list()
except TypeError:
return list(obj) Update 2See #2110 (comment) |
https://github.com/narwhals-dev/narwhals/actions/runs/13567213313/job/37922929104?pr=2110
|
Spotted during an upstream integration test - narwhals-dev/narwhals#2110 (comment) IIRC, I added `strict` before it was [deprecated](https://narwhals-dev.github.io/narwhals/backcompat/#breaking-changes-carried-out-so-far) to help match an `@overload`. Since then, the `@overload` definitions in `narwhals` have improved - meaning the default **doesn't** need to be passed in for us to get it typing correctly # Related - #3693 - narwhals-dev/narwhals#2110
Thanks - I haven't looked at the shiny failure closely, but if that and the altair one are easy fixes then I think we can do this |
Great thanks @MarcoGorelli - the shiny one should be solved by (#2109) - or at least most of it |
IntoSeriesT = TypeVar("IntoSeriesT", bound="IntoSeries", default=Any) | ||
T = TypeVar("T", default=Any) | ||
else: | ||
from typing import TypeVar | ||
|
||
IntoSeriesT = TypeVar("IntoSeriesT", bound="IntoSeries") | ||
T = TypeVar("T") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using PEP 696 is intended to limit the impact of the change downstream.
Code that currently uses v1.Series
should fall back to the default v1.Series[Any]
.
Instead of appearing as v1.Series[Unknown]
.
I'm not sure if that was a problem in #1412, since the logs have expired https://github.com/narwhals-dev/narwhals/actions/runs/11932852152/job/33258672137?pr=1412
Hey @schloerke 👋! I saw you were active on #1412 (comment) and have recently made a typing PR (posit-dev/py-shiny#1875) Anyways, I wanted to check in and see if (#2110 (comment)) sounds like a reasonable assessment of this integration failure run UpdateAll looking good in (https://github.com/narwhals-dev/narwhals/actions/runs/13575555103/job/37950799431?pr=2110) Seems like (#2109) was the only issue for |
@MarcoGorelli I believe we're all good now 😁, just |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a better understanding *now* of what the issue is in (https://github.com/narwhals-dev/narwhals/actions/runs/13577097131/job/37955718895). - This patch is *functionally* the same - but removes the need for ignoring a (typing) warning following (narwhals-dev/narwhals@62f8a1f). - Should also mean we don't need to bump our `narwhals` minimum [again](#3807 (comment)). - **Bonus**: reduced overhead for `<3.11` (python/cpython#84403) ## Related - narwhals-dev/narwhals#2110 - #3811 - narwhals-dev/narwhals#2110 (comment) - narwhals-dev/narwhals#2110 (comment) - narwhals-dev/narwhals#2111
Thank you @dangotbanned ! Very exciting to have access to |
Q: Is Shiny the only package holding back on typing updates for v1 stable? It's a slower process compared to your fast releases... but in a few weeks, I could take a look to see if Shiny can get typing updates for Narwhals v1 (not stable) and bump the min version for Narwhals. |
Tracking
DataFrame.sort_values
overload match #2109nw.from_native(..., strict=True)
vega/altair#3811What type of PR is this? (check all applicable)
Related issues
Series
generic #1412LazyFrame
has a uniqueTypeVar
#1930Checklist
If you have comments or can explain your changes, please do so below
Series
generic #1412 (comment))LazyFrame
has a uniqueTypeVar
#1930)v1.(Data|Lazy)Frame
because they use a differentTypeVar
to their parent