ci(typing): add `pyright` to ci #2035

dangotbanned · 2025-02-17T19:42:32Z

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

Related issues

Related fix(typing): Resolve all mypy & pyright errors for _arrow #2007 (comment)

Checklist

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

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

Had a look at (https://github.com/e10v/tea-tasting/blob/main/.github/workflows/ci.yml), which seemed to just run pyright as a command.
I've pulled out some topics into threads
Commit messages also have some more detail

Pretty sure `pyarrow-stubs` is going to need an upstream fix for the imports #2007 (comment)

for more information, see https://pre-commit.ci

https://github.com/narwhals-dev/narwhals/actions/runs/13377879586/job/37360851272?pr=2035

dangotbanned · 2025-02-17T20:16:36Z

.github/workflows/typing.yml

      - name: Run pyright
-        run: pyright
+        run: |
+          source .venv/bin/activate
+          pyright


Expecting to get:

32 errors, 4 warnings, 0 informations

Update 1

Actually got:

25 errors, 4 warnings, 0 informations

close enough

@MarcoGorelli that was easier than expected https://github.com/narwhals-dev/narwhals/actions/runs/13377926539/job/37360986379?pr=2035

Do you want me to fix these or merge with errors?

The ones that look like this would just need to be ignored, for now

Update 2

https://github.com/narwhals-dev/narwhals/actions/runs/13379548648/job/37365557071?pr=2035

11 errors, 0 warnings, 0 informations

Update 3

https://github.com/narwhals-dev/narwhals/actions/runs/13389097323/job/37392499933?pr=2035

8 errors, 0 warnings, 0 informations

All of these seems to be roughly the same issue mentioned in (#2035 (comment)).

Will take a look at if there are any small changes that could be made on the protocol typing.

It pretty much boils down to defining that it should accept lazy and eager, but then later constraining to only eager or only lazy

narwhals/narwhals/typing.py

Lines 78 to 83 in 9571ad6

class CompliantExpr(Protocol, Generic[CompliantSeriesT_co]):

_implementation: Implementation

_backend_version: tuple[int, ...]

_evaluate_output_names: Callable[

[CompliantDataFrame | CompliantLazyFrame], Sequence[str]

]

Edit

https://github.com/narwhals-dev/narwhals/actions/runs/13389346860/job/37393273341?pr=2035

See commit message: revert(typing): reintroduce arrow errors evaluate_output_names

10 errors, 0 warnings, 0 informations

@MarcoGorelli I think this is the missing piece in having all the typing tie in together nicely.

I've been playing around with this as a solution:

narwhals.typing diff

diff --git a/narwhals/typing.py b/narwhals/typing.py index aa18a999..ca0a0607 100644 --- a/narwhals/typing.py +++ b/narwhals/typing.py @@ -70,24 +70,29 @@ class CompliantLazyFrame(Protocol): # (so, no broadcasting is necessary). +CompliantFrameT_contra = TypeVar( + "CompliantFrameT_contra", + bound="CompliantDataFrame | CompliantLazyFrame", + contravariant=True, +) CompliantSeriesT_co = TypeVar( "CompliantSeriesT_co", bound=CompliantSeries, covariant=True ) -class CompliantExpr(Protocol, Generic[CompliantSeriesT_co]): +class CompliantExpr(Protocol, Generic[CompliantSeriesT_co, CompliantFrameT_contra]): _implementation: Implementation _backend_version: tuple[int, ...] - _evaluate_output_names: Callable[ - [CompliantDataFrame | CompliantLazyFrame], Sequence[str] - ] + _evaluate_output_names: Callable[[CompliantFrameT_contra], Sequence[str]] _alias_output_names: Callable[[Sequence[str]], Sequence[str]] | None _depth: int _function_name: str def __call__(self, df: Any) -> Sequence[CompliantSeriesT_co]: ... def __narwhals_expr__(self) -> None: ... - def __narwhals_namespace__(self) -> CompliantNamespace[CompliantSeriesT_co]: ... + def __narwhals_namespace__( + self, + ) -> CompliantNamespace[CompliantSeriesT_co, CompliantFrameT_contra]: ... def is_null(self) -> Self: ... def alias(self, name: str) -> Self: ... def cast(self, dtype: DType) -> Self: ... @@ -102,11 +107,13 @@ class CompliantExpr(Protocol, Generic[CompliantSeriesT_co]): def __pow__(self, other: Any) -> Self: ... -class CompliantNamespace(Protocol, Generic[CompliantSeriesT_co]): - def col(self, *column_names: str) -> CompliantExpr[CompliantSeriesT_co]: ... +class CompliantNamespace(Protocol, Generic[CompliantSeriesT_co, CompliantFrameT_contra]): + def col( + self, *column_names: str + ) -> CompliantExpr[CompliantSeriesT_co, CompliantFrameT_contra]: ... def lit( self, value: Any, dtype: DType | None - ) -> CompliantExpr[CompliantSeriesT_co]: ... + ) -> CompliantExpr[CompliantSeriesT_co, CompliantFrameT_contra]: ... class SupportsNativeNamespace(Protocol): @@ -306,7 +313,7 @@ if TYPE_CHECKING: # This one needs to be in TYPE_CHECKING to pass on 3.9, # and can only be defined after CompliantExpr has been defined IntoCompliantExpr: TypeAlias = ( - CompliantExpr[CompliantSeriesT_co] | CompliantSeriesT_co + CompliantExpr[CompliantSeriesT_co, CompliantFrameT_contra] | CompliantSeriesT_co )

This checks out, but would require lots of small changes everywhere that CompliantNamespace and CompliantExpr are used.

Where?

FYI, I'm just exploring here, not planning to include this in the PR 🙂

The reason I say "missing piece", is because it would fill in the only non-generic hole in https://github.com/narwhals-dev/narwhals/blob/9571ad67e0da05d753e3595363b1537a3cef40bf/narwhals/_expression_parsing.py
I think you then might be able to start moving some more of the methods up to the Compliant*-level.
Or at-least their signatures?

Note
Those changes are over on (https://github.com/narwhals-dev/narwhals/tree/dev-pyright-eval-out-names)

Which is just +1 commit on top of (#2035)

chore(typing): make _evaluate_output_names generic

Passes pyright and mypy fully for me locally

Fixes: ```py to_py_scalar_test.py:46:39 - error: Second argument to "isinstance" must be a class or tuple of classes Generic type with type arguments not allowed for instance or class checks ```

…peForm]

docs state `str` is allowed

https://github.com/narwhals-dev/narwhals/actions/runs/13378808755/job/37363499208?pr=2035

Shape typing is an ongoing issue numpy/numpy#28076

narwhals/_dask/group_by.py

dangotbanned · 2025-02-17T22:36:47Z

tests/frame/with_columns_sequence_test.py

@@ -15,7 +15,7 @@
 def test_with_columns(constructor_eager: ConstructorEager) -> None:
    result = (
        nw.from_native(constructor_eager(data))
-        .with_columns(d=np.array([4, 5]))
+        .with_columns(d=np.array([4, 5]))  # pyright: ignore[reportArgumentType]


See (numpy/numpy#28076)

Not that happy about this.
Might need to change _(1|2)DArray to just both alias _AnyDArray

narwhals/narwhals/typing.py

Lines 270 to 273 in 9571ad6

_NDArray: TypeAlias = "np.ndarray[_ShapeT, Any]"

_1DArray: TypeAlias = "_NDArray[tuple[int]]" # noqa: PYI042, PYI047

_2DArray: TypeAlias = "_NDArray[tuple[int, int]]" # noqa: PYI042, PYI047

_AnyDArray: TypeAlias = "_NDArray[tuple[int, ...]]" # noqa: PYI047

Maybe with a doc linking back to the numpy issue and a description of what we actually mean?

The problem stems from most functions returning shape: tuple[int, ...] - which fails both _(1|2)DArray.

- Now passes both type checkers, without ignore comments - Pass `polars` an iterator directly - `aggs` is only iterated over **once** on the `polars` side, rather than **twice** in `narwhals` + again in `polars` #2035 (comment)

`pyright` doesn't recognise `_expr`, but can see: https://github.com/dask/dask-expr/blob/42e7a8958ba286a4c7b2f199d143a70b0b479489/dask_expr/_collection.py#L77 via: https://github.com/dask/dask-expr/blob/42e7a8958ba286a4c7b2f199d143a70b0b479489/dask_expr/__init__.py#L4

To demonstrate that this appears on every use of `CompliantExpr` #2035 (comment)

Related #2035 (comment)

- Will always be the same result on every call to the inner `evaluate_output_names` - Unsure how frequent it would be called though, but noticed in (#2035 (comment))

no runtime change, but now we get syntax highlighting

`dask_expr` already imports this for `wraps` (and loads of dependencies), so we didn't even get a performance boost

MarcoGorelli · 2025-02-18T16:02:32Z

thanks - generally looks good, if we can get it green i think we can merge!

dangotbanned · 2025-02-18T16:25:35Z

thanks - generally looks good, if we can get it green i think we can merge!

Thanks @MarcoGorelli, did you see this? (#2035 (comment))

That gets everything green, but it requires a bigger change - which I split out into another branch for comparison

dangotbanned · 2025-02-20T09:05:36Z

thanks - generally looks good, if we can get it green i think we can merge!

Thanks @MarcoGorelli, did you see this? (#2035 (comment))

That gets everything green, but it requires a bigger change - which I split out into another branch for comparison

@MarcoGorelli could we merge this if I just add this ignore (b26c94c) for all the current errors?

We can solve these with either (or maybe both?):

But I'd rather get this part in first because it doesn't redefine any protocols/typevars

MarcoGorelli · 2025-02-20T09:55:37Z

sure, happy to do things incrementally 👍

MarcoGorelli

feel free to ship it once ignores are in and CI is green

dangotbanned · 2025-02-20T10:08:47Z

sure, happy to do things incrementally 👍

Thanks @MarcoGorelli

Resolves #2035 (comment)

dangotbanned added 3 commits February 17, 2025 19:28

ci(typing): add a basic pyright config

c7f6e2f

Pretty sure `pyarrow-stubs` is going to need an upstream fix for the imports #2007 (comment)

ci: add pyright to [typing]

0a4c7c4

ci: match same files as mypy

9c74ceb

dangotbanned added ci typing labels Feb 17, 2025

pre-commit-ci bot and others added 3 commits February 17, 2025 19:44

[pre-commit.ci] auto fixes from pre-commit.com hooks

68cef04

for more information, see https://pre-commit.ci

ci(typing): add "Run pyright" step

c9e3a31

try activating first?

af0677e

https://github.com/narwhals-dev/narwhals/actions/runs/13377879586/job/37360851272?pr=2035

dangotbanned commented Feb 17, 2025

View reviewed changes

dangotbanned added 6 commits February 17, 2025 21:23

fix(typing): use class instead of generic alias

6f2b75e

Fixes: ```py to_py_scalar_test.py:46:39 - error: Second argument to "isinstance" must be a class or tuple of classes Generic type with type arguments not allowed for instance or class checks ```

ci(typing): disable [reportUnusedExpression]

cf1fb46

fix: Variable not allowed in type expression Pylance [reportInvalidTy…

4a8dd5d

…peForm]

ignore incorrect stub

8a4db22

docs state `str` is allowed

fix(typing): resolve some _dask.group_by

b3d8d53

https://github.com/narwhals-dev/narwhals/actions/runs/13378808755/job/37363499208?pr=2035

ignore numpy test shape

65e7ff0

Shape typing is an ongoing issue numpy/numpy#28076

dangotbanned commented Feb 17, 2025

View reviewed changes

narwhals/_dask/group_by.py Outdated Show resolved Hide resolved

dangotbanned commented Feb 17, 2025

View reviewed changes

dangotbanned added 4 commits February 18, 2025 10:08

Merge remote-tracking branch 'upstream/main' into dev-pyright

d87705c

chore: fix typing, simplify _polars.group_by

e182c9c

- Now passes both type checkers, without ignore comments - Pass `polars` an iterator directly - `aggs` is only iterated over **once** on the `polars` side, rather than **twice** in `narwhals` + again in `polars` #2035 (comment)

revert(typing): reintroduce arrow errors evaluate_output_names

b26c94c

To demonstrate that this appears on every use of `CompliantExpr` #2035 (comment)

dangotbanned changed the title ~~ci(DRAFT): add pyright to ci?~~ ci(typing): add pyright to ci Feb 18, 2025

dangotbanned marked this pull request as ready for review February 18, 2025 13:15

dangotbanned added a commit that referenced this pull request Feb 18, 2025

chore(typing): make _evaluate_output_names generic

a468674

Related #2035 (comment)

dangotbanned mentioned this pull request Feb 18, 2025

chore(perf): move combine_evaluate_output_names check #2040

Merged

10 tasks

dangotbanned added 3 commits February 18, 2025 14:57

fix(typing): resolve dask group by

1acc660

no runtime change, but now we get syntax highlighting

refactor: move functools import

bde040e

`dask_expr` already imports this for `wraps` (and loads of dependencies), so we didn't even get a performance boost

Merge remote-tracking branch 'upstream/main' into dev-pyright

cd8352f

dangotbanned mentioned this pull request Feb 19, 2025

fix TypeVar used in (SparkLike|DuckDB)Expr #2044

Open

Merge branch 'main' into dev-pyright

6f54657

MarcoGorelli approved these changes Feb 20, 2025

View reviewed changes

chore(typing): ignore all _evaluate_output_names errors

dc49d5f

Resolves #2035 (comment)

dangotbanned merged commit 2abed29 into main Feb 20, 2025
28 checks passed

dangotbanned deleted the dev-pyright branch February 20, 2025 10:21

dangotbanned mentioned this pull request Feb 20, 2025

refactor(typing): make _evaluate_output_names generic #2053

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(typing): add `pyright` to ci #2035

ci(typing): add `pyright` to ci #2035

dangotbanned commented Feb 17, 2025 •

edited

Loading

dangotbanned Feb 17, 2025 •

edited

Loading

dangotbanned Feb 17, 2025

dangotbanned Feb 17, 2025

dangotbanned Feb 17, 2025 •

edited

Loading

dangotbanned Feb 18, 2025 •

edited

Loading

dangotbanned Feb 18, 2025 •

edited

Loading

dangotbanned Feb 18, 2025 •

edited

Loading

dangotbanned Feb 17, 2025

dangotbanned Feb 18, 2025

MarcoGorelli commented Feb 18, 2025

dangotbanned commented Feb 18, 2025 •

edited

Loading

dangotbanned commented Feb 20, 2025

MarcoGorelli commented Feb 20, 2025

MarcoGorelli left a comment

dangotbanned commented Feb 20, 2025

	class CompliantExpr(Protocol, Generic[CompliantSeriesT_co]):
	_implementation: Implementation
	_backend_version: tuple[int, ...]
	_evaluate_output_names: Callable[
	[CompliantDataFrame \| CompliantLazyFrame], Sequence[str]
	]

	_NDArray: TypeAlias = "np.ndarray[_ShapeT, Any]"
	_1DArray: TypeAlias = "_NDArray[tuple[int]]" # noqa: PYI042, PYI047
	_2DArray: TypeAlias = "_NDArray[tuple[int, int]]" # noqa: PYI042, PYI047
	_AnyDArray: TypeAlias = "_NDArray[tuple[int, ...]]" # noqa: PYI047

ci(typing): add pyright to ci #2035

ci(typing): add pyright to ci #2035

Conversation

dangotbanned commented Feb 17, 2025 • edited Loading

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

dangotbanned Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Update 1

dangotbanned Feb 17, 2025

Choose a reason for hiding this comment

dangotbanned Feb 17, 2025

Choose a reason for hiding this comment

dangotbanned Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Update 2

dangotbanned Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Update 3

Edit

dangotbanned Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

dangotbanned Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

dangotbanned Feb 17, 2025

Choose a reason for hiding this comment

dangotbanned Feb 18, 2025

Choose a reason for hiding this comment

MarcoGorelli commented Feb 18, 2025

dangotbanned commented Feb 18, 2025 • edited Loading

dangotbanned commented Feb 20, 2025

MarcoGorelli commented Feb 20, 2025

MarcoGorelli left a comment

Choose a reason for hiding this comment

dangotbanned commented Feb 20, 2025

ci(typing): add `pyright` to ci #2035

ci(typing): add `pyright` to ci #2035

dangotbanned commented Feb 17, 2025 •

edited

Loading

dangotbanned Feb 17, 2025 •

edited

Loading

dangotbanned Feb 17, 2025 •

edited

Loading

dangotbanned Feb 18, 2025 •

edited

Loading

dangotbanned Feb 18, 2025 •

edited

Loading

dangotbanned Feb 18, 2025 •

edited

Loading

dangotbanned commented Feb 18, 2025 •

edited

Loading