fix(typing): Resolve all `mypy` & `pyright` errors for `_arrow` by dangotbanned · Pull Request #2007 · narwhals-dev/narwhals

dangotbanned · 2025-02-13T16:30:56Z

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

Related issues

Closes CI: get mypy passing with pyarrow-stubs installed #1961

Checklist

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

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

~~Planning to finish getting the mypy errors to 0~~, then ~~tidying things up~~

Will close #1961 Still have ~50 errors for `mypy` to review

dangotbanned · 2025-02-13T16:45:24Z

@MarcoGorelli Almost out of the rabbit hole on this!

I've found some more places where (#1657 (comment)) would be pretty helpful:

narwhals/narwhals/_arrow/series.py

Lines 126 to 160 in 1d24dec

    
           def _from_native_series( 
        
               self: ArrowSeries[Any], 
        
               series: pa.Array[_ScalarT_co] 
        
               | pa.ChunkedArray[Any] 
        
               | pa.ChunkedArray[_ScalarT_co] 
        
               | pa.Array[Any], 
        
           ) -> ArrowSeries[_ScalarT_co]: 
        
               return ArrowSeries( 
        
                   chunked_array(series), 
        
                   name=self._name, 
        
                   backend_version=self._backend_version, 
        
                   version=self._version, 
        
               ) 
        
           @classmethod 
        
           def _from_iterable( 
        
               cls: type[Self], 
        
               data: Iterable[_ScalarT_co], 
        
               name: str, 
        
               *, 
        
               backend_version: tuple[int, ...], 
        
               version: Version, 
        
           ) -> ArrowSeries[_ScalarT_co]: 
        
               return cls( 
        
                   chunked_array([data]), 
        
                   name=name, 
        
                   backend_version=backend_version, 
        
                   version=version, 
        
               ) 
        
           def __narwhals_namespace__(self: Self) -> ArrowNamespace: 
        
               from narwhals._arrow.namespace import ArrowNamespace 
        
               return ArrowNamespace( 
        
                   backend_version=self._backend_version, version=self._version

narwhals/narwhals/_arrow/series.py

Lines 549 to 552 in 1d24dec

    
           def cast(self: Self, dtype: DType) -> ArrowSeries[Any]: 
        
               ser = self._native_series 
        
               data_type = narwhals_to_native_dtype(dtype, self._version) 
        
               return self._from_native_series(pc.cast(ser, data_type))

Essentially anywhere that narwhals object would "change" its TypeVar, the current self.__class__(...) route breaks the typing.

So for ArrowSeries[T1], you can't go to ArrowSeries[T2] without a @classmethod that removes T1 from scope.

My brain has fully melted working on this, hope the above made sense 🫠
If not (https://typing.readthedocs.io/en/latest/spec/generics.html#scoping-rules-for-type-variables)

MarcoGorelli · 2025-02-13T16:50:42Z

thanks for working on this

is it necessary to make ArrowSeries generic in the PyArrow type? would it work to just keep that out for now?

narwhals/_arrow/group_by.py

narwhals/_arrow/namespace.py

dangotbanned · 2025-02-13T17:08:40Z

thanks for working on this

is it necessary to make ArrowSeries generic in the PyArrow type? would it work to just keep that out for now?

@MarcoGorelli 100% needed to resolve the issues I'm afraid 😔

Without the TypeVar, most of the @overloads end up matching Expression.
This was the main source of errors, since we don't appear to use Expression anywhere, it just introduces a huge amount of noise

Having read through a lot of the code (but not using pyarrow much personally) I am curious as to why we've not used Expression much/at all?

It seems to be available in our min version (https://arrow.apache.org/docs/11.0/python/generated/pyarrow.dataset.Expression.html)

For some context of the kinds of errors, (#1961 (comment))

https://github.com/narwhals-dev/narwhals/actions/runs/13312428814/job/37178107510?pr=2007

Causing issues in CI, but not locally? https://github.com/narwhals-dev/narwhals/actions/runs/13313655502/job/37182229033?pr=2007

Possibly the source of https://github.com/narwhals-dev/narwhals/actions/runs/13313804295/job/37182729479?pr=2007

dangotbanned · 2025-02-13T17:52:52Z

Started at like 150-300 errors

Down to 32 errors 😁!!!!!!!!!!!
https://github.com/narwhals-dev/narwhals/actions/runs/13313871223/job/37182957093?pr=2007

All remaining errors are in tests/**, finished fixing everything for narwhals/**

Update 5

Only 1 error left (https://github.com/narwhals-dev/narwhals/actions/runs/13328999125/job/37228573119?pr=2007)

Update 6

mypy is satisfied 😅 https://github.com/narwhals-dev/narwhals/actions/runs/13329410417/job/37229803694?pr=2007

Resolves https://github.com/narwhals-dev/narwhals/actions/runs/13313871223/job/37182957093?pr=2007#step:8:15 Doesn't resolve apache/arrow#41017

…) [attr-defined] `__iter__` doesn't seem to be defined in the docs or stubs https://arrow.apache.org/docs/python/generated/pyarrow.Table.html

- The stubs are overly strict for `Table.from_arrays` - It can accept `Sequence[Array[T] | ChunkedArray[T]]`

…s" cannot be "object" [type-var] - Had 2 of these - Used a different path that preserved the generic type

narwhals/_arrow/series.py

dangotbanned · 2025-02-13T21:35:38Z

narwhals/_arrow/typing.py

+Incomplete: TypeAlias = Any  # pragma: no cover
+"""
+Marker for working code that fails on the stubs.
+
+Common issues:
+- Annotated for `Array`, but not `ChunkedArray`
+- Relies on typing information that the stubs don't provide statically
+- Missing attributes
+- Incorrect return types
+- Inconsistent use of generic/concrete types
+- `_clone_signature` used on signatures that are not identical
+"""


Note
See for more context https://github.com/python/typeshed/blob/a410f251ee535950eb4440ad41ae4c00c87e6a67/stdlib/_typeshed/__init__.pyi#L45-L50

I've been sprinkling these in with a comment when all else fails, e.g:

narwhals/narwhals/_arrow/series.py

Lines 486 to 490 in 1b537e7

def diff(self: ArrowSeries[_NumericOrTemporalT]) -> ArrowSeries[_NumericOrTemporalT]:

# NOTE: stub only permits `ChunkedArray[TemporalScalar]`

# (https://github.com/zen-xu/pyarrow-stubs/blob/d97063876720e6a5edda7eb15f4efe07c31b8296/pyarrow-stubs/compute.pyi#L145-L148)

diff: Incomplete = pc.pairwise_diff

return self._from_native_series(diff(self._native_series.combine_chunks()))

If the stub issues get resolved in the future, this will be a lot easier to fix than just using Any directly

Want to start a thread, but havent changed the code here yet

narwhals/_interchange/dataframe.py

Fixes 89672e2#r1955890197

`pyright` doesn't need this, `mypy` infers this as `str` - which is too wide > narwhals/_arrow/namespace.py:372: error: No overload variant of "binary_join_element_wise" matches argument types "Generator[ChunkedArray[StringScalar], None, None]", "str" [call-overload]> narwhals/_arrow/namespace.py:372: note: Possible overload variants:

Related e2940e7

MarcoGorelli · 2025-02-17T11:31:11Z

narwhals/_arrow/utils.py

-    backend_version: tuple[int, ...],
-) -> Any:
+    length: int, other: ArrowSeries, backend_version: tuple[int, ...]
+) -> pa.BooleanArray:


🤔 this doesn't have to be boolean, right? plenty of binary operations don't return booleans?

…ev/narwhals into typing-major-fixing-1

…als into typing-major-fixing-1

dangotbanned · 2025-02-17T11:45:53Z

@MarcoGorelli I'm gonna leave you to it for now to avoid conflicts, but could you give me a shout when everything is ready please?

I'm getting warnings from pyright for some of these changes

MarcoGorelli · 2025-02-17T11:59:27Z

sure, I think those pyright issues should be addressed now (and it would be really good to get pyright running in CI too)

downstream test failures should all be unrelated to this PR

MarcoGorelli

happy to get this in anyway, thanks @dangotbanned , really appreciate it!

dangotbanned · 2025-02-17T12:07:05Z

happy to get this in anyway, thanks @dangotbanned , really appreciate it!

Thanks @MarcoGorelli, both mypy and pyright seem to be good with _arrow now 🎉

Edit

double cat was unintentional but I like it

dangotbanned · 2025-02-17T12:09:44Z

sure, I think those pyright issues should be addressed now (and it would be really good to get pyright running in CI too)

downstream test failures should all be unrelated to this PR

@MarcoGorelli IIRC marimo has pyright in their CI - which could be worth a look

dangotbanned · 2025-02-17T12:19:10Z

narwhals/_arrow/series_dt.py

+    def total_milliseconds(self: Self) -> ArrowSeries:
+        ser: ArrowSeries = self._compliant_series
        arr = ser._native_series
-        unit = ser._type.unit
+        unit = ser._type.unit  # type: ignore[attr-defined]
        unit_to_milli_factor = {
            "s": 1e3,  # seconds
            "ms": 1,  # milli


A bit dissapointed this change reintroduced the need for the #type: ignores that were fixed

Note
Just an observation

#2007 (comment)

For future reference, this commit has a solution using the "public" API 44560d1

dangotbanned · 2025-02-17T12:22:45Z

narwhals/dependencies.py

+def is_pyarrow_chunked_array(
+    ser: Any | ArrowChunkedArray,
+) -> TypeIs[ArrowChunkedArray]:


Could you revert this to keeping the TypeVar?

I don't see a need to erase it, if it can be known here - especially since this is public

59af046 (#2007)

MarcoGorelli · 2025-02-17T12:31:54Z

Going to go ahead and merge this as I think it's a clear improvement, and we can address some smaller items like #2007 (comment) and #2007 (comment) as follow-ups

Thanks again Dan!

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

Discovered during narwhals-dev/narwhals#2007 I worked around this with this, but seemed simple enough to upstream: ```py def _type(self: pa.ChunkedArray[pa.Scalar[_DataType_CoT]]) -> _DataType_CoT: if TYPE_CHECKING: return self[0].type return self.type ```

I noted this in #2007 (comment) Seems `preview` detects this, as I was expecting when I disabled the `pyright` diagnostic

- narwhals-dev/narwhals#2007 (comment) - narwhals-dev/narwhals#2007 (comment)

dangotbanned added 3 commits February 13, 2025 16:23

fix(typing): Resolve all pyright warnings for _arrow

e7f465f

Will close #1961 Still have ~50 errors for `mypy` to review

Merge remote-tracking branch 'upstream/main' into typing-major-fixing-1

0015ac1

chore(typing): add extra ignore

1d24dec

dangotbanned added internal typing labels Feb 13, 2025

dangotbanned commented Feb 13, 2025

View reviewed changes

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

dangotbanned commented Feb 13, 2025

View reviewed changes

narwhals/_arrow/namespace.py Outdated Show resolved Hide resolved

dangotbanned added 4 commits February 13, 2025 17:35

ci: omit _arrow.typing.py from coverage

286beeb

https://github.com/narwhals-dev/narwhals/actions/runs/13312428814/job/37178107510?pr=2007

fix(typing): error: Name "ser" already defined on line 59 [no-redef]

757c6bd

fix(DRAFT): try reordering maybe_extract_py_scalar overloads

b830aa2

Causing issues in CI, but not locally? https://github.com/narwhals-dev/narwhals/actions/runs/13313655502/job/37182229033?pr=2007

ci(typing): re-enable pyarrow-stubs

5cadbed

Possibly the source of https://github.com/narwhals-dev/narwhals/actions/runs/13313804295/job/37182729479?pr=2007

dangotbanned added 6 commits February 13, 2025 18:51

fix: only check for .ordered when type can have property

1e6eb75

Resolves https://github.com/narwhals-dev/narwhals/actions/runs/13313871223/job/37182957093?pr=2007#step:8:15 Doesn't resolve apache/arrow#41017

fix(typing): use utils.chunked_array

51ad255

fix(typing): misc assignment/redef errors

658c963

fix(typing): error: "Table" has no attribute "__iter__" (not iterable…

84033f1

…) [attr-defined] `__iter__` doesn't seem to be defined in the docs or stubs https://arrow.apache.org/docs/python/generated/pyarrow.Table.html

chore(typing): lie about broadcast_series

f35b01e

- The stubs are overly strict for `Table.from_arrays` - It can accept `Sequence[Array[T] | ChunkedArray[T]]`

fix(typing): error: Value of type variable "_ArrayT" of "concat_array…

1b537e7

…s" cannot be "object" [type-var] - Had 2 of these - Used a different path that preserved the generic type

dangotbanned commented Feb 13, 2025

View reviewed changes

narwhals/_arrow/series.py Outdated Show resolved Hide resolved

dangotbanned commented Feb 13, 2025

View reviewed changes

dangotbanned added 2 commits February 14, 2025 10:07

chore(DRAFT): add comment on pyarrow.interchange.from_dataframe

89672e2

Want to start a thread, but havent changed the code here yet

Merge remote-tracking branch 'upstream/main' into typing-major-fixing-1

ce986e1

dangotbanned commented Feb 14, 2025

View reviewed changes

narwhals/_interchange/dataframe.py Show resolved Hide resolved

dangotbanned added 4 commits February 14, 2025 10:20

fix(typing): ambiguous pyarrow.interchange.from_dataframe import

4d4a722

Fixes 89672e2#r1955890197

chore(typing): mark pc.max_element_wise as Incomplete everywhere

c7f23d5

chore(typing): more help mypy match @overload

b7d149d

Related e2940e7

MarcoGorelli reviewed Feb 17, 2025

View reviewed changes

dangotbanned and others added 7 commits February 17, 2025 11:31

refactor: avoid pyarrow.__lib_pxi.types in utils

bc02cdb

Merge branch 'typing-major-fixing-1' of https://github.com/narwhals-d…

8c8dbe7

…ev/narwhals into typing-major-fixing-1

fixup

8ca893c

Merge branch 'typing-major-fixing-1' of https://github.com/narwhals-d…

e6cecbc

…ev/narwhals into typing-major-fixing-1

remove unalive code

aa62a32

Merge branch 'typing-major-fixing-1' of github.com:narwhals-dev/narwh…

9507261

…als into typing-major-fixing-1

last one

3ea9674

MarcoGorelli added 2 commits February 17, 2025 11:52

fixup tests

9bba7aa

fixup typing

1ceda96

MarcoGorelli approved these changes Feb 17, 2025

View reviewed changes

pyright ignore

837f0e6

dangotbanned commented Feb 17, 2025

View reviewed changes

dangotbanned merged commit 2b58ee7 into main Feb 17, 2025
24 of 28 checks passed

dangotbanned deleted the typing-major-fixing-1 branch February 17, 2025 12:51

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

ci(typing): add a basic pyright config

c7f6e2f

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

dangotbanned mentioned this pull request Feb 17, 2025

ci(typing): add pyright to ci #2035

Merged

10 tasks

dangotbanned mentioned this pull request Feb 19, 2025

fix: Preserve generic in ChunkedArray.type zen-xu/pyarrow-stubs#177

Merged

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

refactor: fix (F841) unused-variable

ea97455

I noted this in #2007 (comment) Seems `preview` detects this, as I was expecting when I disabled the `pyright` diagnostic

dangotbanned mentioned this pull request Feb 25, 2025

chore(typing): enable typing checks for pyspark #2051

Merged

10 tasks

dangotbanned added a commit to dangotbanned/pyarrow-stubs that referenced this pull request Mar 1, 2025

fix: widen & simplify max_element_wise

1b9001e

- narwhals-dev/narwhals#2007 (comment) - narwhals-dev/narwhals#2007 (comment)

dangotbanned mentioned this pull request Mar 1, 2025

Tracking: pyarrow-stubs upstream fixes #2124

Open

33 tasks

dangotbanned mentioned this pull request Apr 16, 2025

feat: IbisLazyFrame support #2000

Merged

10 tasks

	def diff(self: ArrowSeries[_NumericOrTemporalT]) -> ArrowSeries[_NumericOrTemporalT]:
	# NOTE: stub only permits `ChunkedArray[TemporalScalar]`
	# (https://github.com/zen-xu/pyarrow-stubs/blob/d97063876720e6a5edda7eb15f4efe07c31b8296/pyarrow-stubs/compute.pyi#L145-L148)
	diff: Incomplete = pc.pairwise_diff
	return self._from_native_series(diff(self._native_series.combine_chunks()))

Conversation

dangotbanned commented Feb 13, 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 commented Feb 13, 2025

Uh oh!

MarcoGorelli commented Feb 13, 2025

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Update 1

Update 2

Update 3

Update 4

Update 5

Update 6

Uh oh!

Uh oh!

dangotbanned Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarcoGorelli Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

dangotbanned commented Feb 17, 2025

Uh oh!

MarcoGorelli commented Feb 17, 2025

Uh oh!

MarcoGorelli left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Edit

Uh oh!

dangotbanned commented Feb 17, 2025

Uh oh!

dangotbanned Feb 17, 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!

dangotbanned Feb 17, 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!

dangotbanned Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

dangotbanned Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli commented Feb 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dangotbanned commented Feb 13, 2025 •

edited

Loading

dangotbanned commented Feb 13, 2025 •

edited

Loading

dangotbanned commented Feb 13, 2025 •

edited

Loading

dangotbanned commented Feb 17, 2025 •

edited

Loading

dangotbanned Feb 17, 2025 •

edited

Loading

dangotbanned Feb 17, 2025 •

edited

Loading