ml-explore / mlx-examples

Examples in the MLX framework
MIT License
5.5k stars 791 forks source link

Proposal: Add mypy to .pre-commit-config.yml #829

Open wangkuiyi opened 3 weeks ago

wangkuiyi commented 3 weeks ago

So far, the.pre-commit-config.yml file only calls black and isort. I unintentionally used https://github.pie.apple.com/aiml-oss/ml-recurrent-drafter/blob/main/.pre-commit-config.yaml and discovered the issues addressed in #827 and #828. It may be worthwhile to include mypy and other tools in the above pre-commit configuration.

awni commented 3 weeks ago

I think its a good idea, especially if we are going to use type annotations (which we do).

wangkuiyi commented 3 weeks ago

I ran the following command to generate the above list of checkable items:

mypy \
  --explicit-package-bases \
  --ignore-missing-imports \
  . | cut -f 1 -d ':' | sort | uniq | sed 's|^|- [ ] |'

I need --explicit-package-base; otherwise, mypy would complain an error like the following and stop working on the rest files in the project:

❯ mypy .
mistral/convert.py: error: Duplicate module named "convert" (also at "./llama/convert.py")
mistral/convert.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
mistral/convert.py: note: Common resolutions include: a) using `--exclude` to avoid checking one of them, b) adding `__init__.py` somewhere, c) using `--explicit-package-bases` or adjusting MYPYPATH
Found 1 error in 1 file (errors prevented further checking)

I need --ignore-missing-imports because even if I pip-install mlx and both mlx/ and mlx/nn/ are in conda's site directory, mypy still complains that it couldn't find type informaiton of these packages.