Skip to content
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

Merged
merged 5 commits into from
Feb 27, 2025

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Feb 27, 2025

Tracking

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

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

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

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)
```
@dangotbanned dangotbanned added enhancement New feature or request typing labels Feb 27, 2025
Comment on lines 243 to +244
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]
Copy link
Member Author

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)

@dangotbanned
Copy link
Member Author

dangotbanned commented Feb 27, 2025

I can just fix this on the altair side? https://github.com/narwhals-dev/narwhals/actions/runs/13567213313/job/37922925743?pr=2110

Pretty sure it would be working if I change strict=True -> pass_through=False.
Similar to #2110 (comment)

Update 1

Following (vega/altair#3811), this warning for altair is valid.

But I can't add an ignore in altair until this is merged

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:

https://github.com/vega/altair/blob/c0956c5ba7de45e1a6b67b2051a10f9477c7afe0/altair/utils/schemapi.py#L495-L500

If anything - this is demonstrating the @overload(s) are now working as intended.
The fact I'm catching a TypeError should require the need of a type: ignore:

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 2

See #2110 (comment)

@dangotbanned
Copy link
Member Author

dangotbanned commented Feb 27, 2025

https://github.com/narwhals-dev/narwhals/actions/runs/13567213313/job/37922929104?pr=2110

shiny is only failing because it is running our test suite and failing #2109

dangotbanned added a commit to vega/altair that referenced this pull request Feb 27, 2025
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
@dangotbanned dangotbanned marked this pull request as ready for review February 27, 2025 14:53
@MarcoGorelli
Copy link
Member

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

@dangotbanned
Copy link
Member Author

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

Comment on lines +98 to +104
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")
Copy link
Member Author

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

@dangotbanned
Copy link
Member Author

dangotbanned commented Feb 27, 2025

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

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

Update

All looking good in (https://github.com/narwhals-dev/narwhals/actions/runs/13575555103/job/37950799431?pr=2110)

Seems like (#2109) was the only issue for shiny

@dangotbanned
Copy link
Member Author

@MarcoGorelli I believe we're all good now 😁, just altair reporting a true positive (https://github.com/narwhals-dev/narwhals/actions/runs/13575555103/job/37950799078?pr=2110)

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

legend mate

@MarcoGorelli MarcoGorelli merged commit 62f8a1f into main Feb 27, 2025
27 of 29 checks passed
@MarcoGorelli MarcoGorelli deleted the v1-series-generic-backport branch February 27, 2025 21:55
dangotbanned added a commit to vega/altair that referenced this pull request Feb 28, 2025
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
@schloerke
Copy link
Contributor

Thank you @dangotbanned ! Very exciting to have access to IntoSeriesT 😃

@schloerke
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants