fix: use mypy pre-commit in local environment

[EdAbati]

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

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

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]

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]

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]

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

[EdAbati]

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

[MarcoGorelli]

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

[EdAbati]

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.

[dangotbanned]

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

Update

Resolved in (fd192f2)

This code provides the narrowing at runtime.
I tried out adding @overload(s) in Series, v1.Series, PandasLikeSeries but they end up invalidating eachother

narwhals/narwhals/series.py

Lines 2591 to 2601 in 60e5705

	if bins is not None and bin_count is not None:
	msg = "can only provide one of `bin_count` or `bins`"
	raise ComputeError(msg)
	if bins is None and bin_count is None:
	bin_count = 10 # polars (v1.20) sets bin=10 if neither are provided.
	if bins is not None:
	for i in range(1, len(bins)):
	if bins[i - 1] >= bins[i]:
	msg = "bins must increase monotonically"
	raise ComputeError(msg)

[dangotbanned]

@MarcoGorelli I'm reasonably happy with where the typing is at after (#1966 (commits))

Of these modules, narwhals._arrow.series is the only one left with major issues:

pyright shouting at me

Will try to look into those after this and #1979 have merged

[MarcoGorelli]

thanks both! sure, happy to move forwards and address the rest incrementally, much appreciate your efforts here!

[camriddell]

@dangotbanned seems like you resolved everything there, thanks!

If it is useful to you in a post-hoc sense

narwhals/narwhals/series.py

Lines 2591 to 2601 in 60e5705

	if bins is not None and bin_count is not None:
	msg = "can only provide one of `bin_count` or `bins`"
	raise ComputeError(msg)
	if bins is None and bin_count is None:
	bin_count = 10 # polars (v1.20) sets bin=10 if neither are provided.
	if bins is not None:
	for i in range(1, len(bins)):
	if bins[i - 1] >= bins[i]:
	msg = "bins must increase monotonically"
	raise ComputeError(msg)

The high-level narwhals.Series entrypoint controls over uses runtime determination of bins or bin_count to determine the behavior of the function. The following rules should be respected:

1. One cannot pass both bins and bin_count in as it leads to ambiguous behavior
2. If one is passed in, then it should be treated as its non-None type; the other should be set to None.
3. If neither are passed it, then it is treated as bin_count=10 and bins=None

I decided to split broad smoke-tests into 2 separate functions:

• bins=... is defined, bin_count=None

  narwhals/tests/series_only/hist_test.py

  Line 84 in 4b1d778

  def test_hist_bin(

• bins=None, bin_count=... is defined

  narwhals/tests/series_only/hist_test.py

  Line 166 in 4b1d778

  def test_hist_count(

---

Thanks for resolving all of these typing issue. In the future, it could be worth refactoring these two behaviors into separate functions for each backend and dispatch the correct call from the narwhals.Series. I think this would make the internal code make a lot more sense as well.