RFC, fix: use `mypy` pre-commit in local environment #1966

EdAbati · 2025-02-08T08:33:07Z

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

Related issues

Related issues CI: get mypy passing with pyarrow-stubs installed #1961
Closes CI / typing: migrate away from pre-commit? #1962

Checklist

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

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

I did some digging, looking for how other projects run typing and general best practices with mypy.
One thing people seem to agree with is that mirrors-mypy has its gotchas and may not be ideal for every project.
In our case we would need to add every backend dependencies as additional-dependencies. In this case we would have to maintain a script that keep those dependencies up to date.

One alternative would be to run mypy in the local environment where all the dependencies are already installed. This is what this PR is trying to do.

~~I have still some open questions, making this draft to test the CI too.~~
~I am expecting to see ~~300 errors from mypy 👀~~

Update: I propose to move to this solution where we:

keep using pre-commit for linting but move mypy to run in the local env -> this is to avoid having to add many additional-dependencies
Add Makefile to add 1 consistent command to run typing checks: make typing
Run mypy in a separate CI pipeline on the entire code

MarcoGorelli · 2025-02-08T09:11:27Z

i like the look of this, thanks for doing it

it looks like this fails in pre-commit-ci, so we may need to find another way of running it in ci? (i'm not averse to removing pre-commit-ci)

EdAbati · 2025-02-08T13:43:53Z

Thanks @MarcoGorelli ! ❤️

This could be a possible approach that doesn't change super much what we currently have:

we add mypy in the dev dependencies
mypy runs in pre-commit locally
we use pyproject.toml to configure mypy's behaviour
we only remove mypy from the pre-commit.ci (since the local env with dependencies will not be created there)
we add a new GH action for typing
the Makefile is to create a command that works the same way in Linux/macOS and Windows (make typing)

The errors are now showing up in the CI!
The dependencies we install in the Typing CI will influence which backends we will check for typing error. (just for this example I only added core)

Happy to hear your thoughts (also @FBruzzesi and @dangotbanned )

MarcoGorelli · 2025-02-08T13:48:30Z

nice, happy to go with this - if we can get it green and then gradually address things, happy to get this in

dangotbanned · 2025-02-08T19:30:49Z

#1966 (comment)

@MarcoGorelli sounds reasonable

Maybe this is a dumb question, but what is the issue with running mypy without pre-commit?
To me it seems the simplest way to get the same errors we'd see locally

MarcoGorelli · 2025-02-08T19:33:21Z

that's fine as well. seeing as we're using pre-commit for other checks, i don't it hurt to have a make typing so that the workflow for anyone using pre-commit isn't affected

EdAbati · 2025-02-09T17:07:04Z

Seriously though, what is the goal here?

My goal here was trying to find a better and more consistent way to run typing tests without disrupting too much the current workflow.
Here we keep mypy in pre-commit and we run mypy on all the relevant code in CI (or manually using make) without pre-commit

Actually if we removed pyarrow-stubs for a bit we'd get waaaay less errors 👀 b301356

Here I meant that it seems that most of typing errors (we are not already ignoring) come from the pyarrow related part of the code. Of course not suggesting to remove this stubs forever 😅
My plan/suggestion is to make the CI green with at least our core dependencies installed and then start reintroducing stubs and removing exclusions in follow-up PRs

Still testing a couple of things, hope to have this PR properly ready for review later today 🤞

narwhals/_arrow/series.py

EdAbati · 2025-02-09T21:18:28Z

narwhals/_duckdb/expr.py

@@ -33,7 +33,7 @@
    from narwhals.utils import Version


-class DuckDBExpr(CompliantExpr["duckdb.Expression"]):
+class DuckDBExpr(CompliantExpr["duckdb.Expression"]):  # type: ignore[type-var]


This ignore should actually be solved. duckdb.Expression is not a CompatibleSeries

Is it ok for a follow-up?

yup definitely

EdAbati · 2025-02-09T21:19:31Z

narwhals/_duckdb/dataframe.py

@@ -159,7 +159,7 @@ def select(
        ):
            return self._from_native_frame(
                self._native_frame.aggregate(
-                    [val.alias(col) for col, val in new_columns_map.items()]
+                    [val.alias(col) for col, val in new_columns_map.items()]  # type: ignore[arg-type]


based on the duckdb stubs, only str are allowed in .aggregate. 🤔

looks wrong, it should be str | list[Expression], right? want to make a PR to duckdb?

EdAbati · 2025-02-09T21:34:10Z

pyproject.toml

+  "narwhals[tests]",
+  "narwhals[typing]",


I think we should separate the dev dependencies into tests and typing.

The latest pinned mypy is available for python >=3.9 and if we simply include it in dev our python 3.8 tests will fail.

narwhals/_duckdb/expr_dt.py

dangotbanned · 2025-02-09T23:03:18Z

narwhals/_pandas_like/dataframe.py

@@ -58,7 +58,7 @@
 from narwhals.typing import CompliantDataFrame
 from narwhals.typing import CompliantLazyFrame

-CLASSICAL_NUMPY_DTYPES = frozenset(
+CLASSICAL_NUMPY_DTYPES: frozenset[np.dtype] = frozenset(


You might be able to get away with this?

Possibly an issue for 3.8 though

Suggested change

CLASSICAL_NUMPY_DTYPES: frozenset[np.dtype] = frozenset(

CLASSICAL_NUMPY_DTYPES = frozenset["np.dtype"](

narwhals/_pandas_like/dataframe.py

`DuckDBPyRelation.cross` isn't in the stubs

MarcoGorelli · 2025-02-10T10:45:09Z

narwhals/_duckdb/expr_dt.py

@@ -60,23 +60,23 @@ def second(self: Self) -> DuckDBExpr:
    def millisecond(self: Self) -> DuckDBExpr:
        return self._compliant_expr._from_call(
            lambda _input: FunctionExpression("millisecond", _input)
-            - FunctionExpression("second", _input) * 1_000,


I've got a feeling they want to nudge us towards using * ConstantExpression(1000), then mypy accepts that

Yeah that was what I went for in (bf9e0d9)

@MarcoGorelli to clarify, that was only on _duckdb.expr (not this module _duckdb.expr_dt)

dangotbanned · 2025-02-10T10:53:11Z

narwhals/_duckdb/expr.py

+            return (
+                FunctionExpression("var_pop", _input)
+                * n_samples
+                / (n_samples - ConstantExpression(ddof))
+            )


I was just fixing a pyright warning here.

If anyone has a meaningful name for the parts of this expression, I think it'd be more readable with an assignment first

Like:

n_samples = n_samples = FunctionExpression("count", _input) meaningful = (n_samples - ConstantExpression(ddof)) # don't use this name! return FunctionExpression("var_pop", _input) * n_samples / meaningful

Resolves 2/3 errors in https://github.com/narwhals-dev/narwhals/actions/runs/13240136620/job/36953460841?pr=1966

dangotbanned · 2025-02-10T12:08:10Z

The last warning seems legit to me https://github.com/narwhals-dev/narwhals/actions/runs/13240255809/job/36953842381?pr=1966

Both uses of bin_count pass None as arguments to functions that don't accept None:

narwhals/narwhals/_pandas_like/series.py

Lines 1057 to 1067 in 4b1d778

    
           elif self._native_series.count() < 1: 
        
               if bins is not None: 
        
                   data = { 
        
                       "breakpoint": bins[1:], 
        
                       "count": zeros(shape=len(bins) - 1), 
        
                   } 
        
               else: 
        
                   data = { 
        
                       "breakpoint": linspace(0, 1, bin_count), 
        
                       "count": zeros(shape=bin_count), 
        
                   }

I don't see any tests covering Series.hist(..., bin_count=None):
https://github.com/narwhals-dev/narwhals/blob/4b1d778beeab20c3b5565dba629e320eb23804c8/tests/series_only/hist_test.py

The one mypy caught looks like it would cause an error at runtime:
https://github.com/numpy/numpy/blob/06f987b7b77453aae7fa11de15f728567fccd407/numpy/_core/function_base.py#L121-L125

@camriddell just wanted to run this by you following #1859 - feel like I must be missing something

EdAbati added 8 commits February 5, 2025 13:02

add hypothesis and control mypy in pyproject.toml

8be02ed

mypy in local

eb70be2

get_item tidy up draft

8e7990c

Merge remote-tracking branch 'upstream/main' into mypy-fixes

6ae4436

declaring relative path

6017193

pyproject fix

86c23ed

restore precommit

13745b4

add makefile

faa477b

EdAbati added 10 commits February 8, 2025 12:08

skip mypy from precommit ci

b7aebab

new typing CI

75f474d

add forgotten setup-python

5f1aea6

where is mypy?

2144196

trying to create a venv

8664672

fix

3aaf983

trying again with PATH

a030f61

always activate

9dbd938

one more activate

a4f6ae2

remove redundant which mypy

46fade5

EdAbati added 2 commits February 8, 2025 14:51

force color?

6bcea6b

force_color

ce3eae9

This comment was marked as outdated.

Sign in to view

EdAbati added 6 commits February 8, 2025 15:51

Merge branch 'main' into mypy-fixes

4e09713

Merge branch 'main' into mypy-fixes

0ee5312

env var check

07532ba

just mypy?

57adae7

colors are not showing, giving up

5689858

add polars only in mypy dependencies

a6892e2

dangotbanned mentioned this pull request Feb 8, 2025

add np.ndarray alias(es) #1972

Closed

Merge branch 'main' into mypy-fixes

9a2d9ff

dangotbanned mentioned this pull request Feb 9, 2025

chore(typing): remove unused ignores #1979

Draft

10 tasks

fix mypy version

b9612cc

dangotbanned reviewed Feb 9, 2025

View reviewed changes

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

EdAbati added 7 commits February 9, 2025 18:59

organise optional dependencies

8177933

add typing_extensions to tests

4456c33

Merge remote-tracking branch 'upstream/main' into mypy-fixes

5dcdc1a

fix downstream tests

0dfed72

fix last types

58ed0bf

add tests reqs

99a6690

Merge branch 'main' into mypy-fixes

81ee931

EdAbati commented Feb 9, 2025

View reviewed changes

narwhals/_duckdb/expr_dt.py Show resolved Hide resolved

EdAbati marked this pull request as ready for review February 9, 2025 21:36

dangotbanned reviewed Feb 9, 2025

View reviewed changes

narwhals/_pandas_like/dataframe.py Show resolved Hide resolved

dangotbanned added 2 commits February 10, 2025 10:34

fix(typing): resolve some new _duckdb.expr warnings

bf9e0d9

chore(typing): add stub ignore

338962c

`DuckDBPyRelation.cross` isn't in the stubs

This comment was marked as resolved.

Sign in to view

MarcoGorelli reviewed Feb 10, 2025

View reviewed changes

dangotbanned reviewed Feb 10, 2025

View reviewed changes

dangotbanned added 2 commits February 10, 2025 11:47

Merge remote-tracking branch 'upstream/main' into pr/EdAbati/1966

9abdd34

fix(typing): widen annotation to inclide _1DArray

54a29c7

Resolves 2/3 errors in https://github.com/narwhals-dev/narwhals/actions/runs/13240136620/job/36953460841?pr=1966

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC, fix: use `mypy` pre-commit in local environment #1966

RFC, fix: use `mypy` pre-commit in local environment #1966

EdAbati commented Feb 8, 2025 •

edited

Loading

MarcoGorelli commented Feb 8, 2025

EdAbati commented Feb 8, 2025 •

edited

Loading

MarcoGorelli commented Feb 8, 2025

This comment was marked as outdated.

dangotbanned commented Feb 8, 2025

MarcoGorelli commented Feb 8, 2025

EdAbati commented Feb 9, 2025

EdAbati Feb 9, 2025

MarcoGorelli Feb 10, 2025

EdAbati Feb 9, 2025

MarcoGorelli Feb 10, 2025

EdAbati Feb 9, 2025

dangotbanned Feb 9, 2025

This comment was marked as resolved.

MarcoGorelli Feb 10, 2025

dangotbanned Feb 10, 2025

dangotbanned Feb 10, 2025 •

edited

Loading

dangotbanned Feb 10, 2025

dangotbanned Feb 10, 2025 •

edited

Loading

dangotbanned commented Feb 10, 2025 •

edited

Loading

	CLASSICAL_NUMPY_DTYPES: frozenset[np.dtype] = frozenset(
	CLASSICAL_NUMPY_DTYPES = frozenset["np.dtype"](

RFC, fix: use mypy pre-commit in local environment #1966

Are you sure you want to change the base?

RFC, fix: use mypy pre-commit in local environment #1966

Conversation

EdAbati commented Feb 8, 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

MarcoGorelli commented Feb 8, 2025

EdAbati commented Feb 8, 2025 • edited Loading

MarcoGorelli commented Feb 8, 2025

This comment was marked as outdated.

dangotbanned commented Feb 8, 2025

MarcoGorelli commented Feb 8, 2025

EdAbati commented Feb 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangotbanned Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangotbanned Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

dangotbanned commented Feb 10, 2025 • edited Loading

RFC, fix: use `mypy` pre-commit in local environment #1966

RFC, fix: use `mypy` pre-commit in local environment #1966

EdAbati commented Feb 8, 2025 •

edited

Loading

EdAbati commented Feb 8, 2025 •

edited

Loading

dangotbanned Feb 10, 2025 •

edited

Loading

dangotbanned Feb 10, 2025 •

edited

Loading

dangotbanned commented Feb 10, 2025 •

edited

Loading