mlx-examples icon indicating copy to clipboard operation
mlx-examples copied to clipboard

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

Open wangkuiyi opened this issue 1 year ago • 12 comments

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.

wangkuiyi avatar Jun 10 '24 20:06 wangkuiyi

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

awni avatar Jun 10 '24 22:06 awni

  • [ ] gguf_llm/generate.py
  • [ ] gguf_llm/models.py
  • [ ] llama/convert.py
  • [ ] mistral/convert.py
  • [ ] mixtral/convert.py
  • [ ] mixtral/mixtral.py
  • [ ] mlx_lm/lora.py
  • [ ] mlx_lm/merge.py
  • [ ] mlx_lm/models/cohere.py
  • [ ] mlx_lm/models/dbrx.py
  • [ ] mlx_lm/models/gemma.py
  • [ ] mlx_lm/models/gpt2.py
  • [ ] mlx_lm/models/gpt_bigcode.py
  • [ ] mlx_lm/models/internlm2.py
  • [ ] mlx_lm/models/llama.py
  • [ ] mlx_lm/models/minicpm.py
  • [ ] mlx_lm/models/mixtral.py
  • [ ] mlx_lm/models/olmo.py
  • [ ] mlx_lm/models/openelm.py
  • [x] mlx_lm/models/phi3.py https://github.com/ml-explore/mlx-examples/pull/833
  • [x] mlx_lm/models/phi3small.py https://github.com/ml-explore/mlx-examples/pull/833
  • [ ] mlx_lm/models/plamo.py
  • [ ] mlx_lm/models/qwen2.py https://github.com/ml-explore/mlx-examples/pull/835
  • [ ] mlx_lm/models/qwen2_moe.py https://github.com/ml-explore/mlx-examples/pull/835
  • [ ] mlx_lm/models/starcoder2.py https://github.com/ml-explore/mlx-examples/pull/835
  • [ ] mlx_lm/server.py
  • [ ] mlx_lm/tuner/trainer.py
  • [ ] mlx_lm/tuner/utils.py
  • [ ] mlx_lm/utils.py
  • [ ] speculative_decoding/decoder.py
  • [ ] speculative_decoding/model.py
  • [ ] tests/test_server.py

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.

wangkuiyi avatar Jun 11 '24 17:06 wangkuiyi

@awni May I take on this issue ??

ParamThakkar123 avatar Mar 30 '25 13:03 ParamThakkar123

How many of these are done ?

ParamThakkar123 avatar Mar 30 '25 13:03 ParamThakkar123

Sure! Just FYI mlx_lm has moved to a new repo https://github.com/ml-explore/mlx-lm

I don't know how many are done.. not too many It hink.

awni avatar Mar 30 '25 13:03 awni

Sure! Just FYI mlx_lm has moved to a new repo https://github.com/ml-explore/mlx-lm

I don't know how many are done.. not too many It hink.

Okay, I will start working on this. Can you please assign it to me ??

ParamThakkar123 avatar Mar 30 '25 13:03 ParamThakkar123

@awni I started a pull request for OpenELM models at https://github.com/ml-explore/mlx-lm/pull/61 . Can you please review it ?? I will start for the rest of the models if that looks fine.

ParamThakkar123 avatar Mar 30 '25 15:03 ParamThakkar123

Updated list :

  • [x] mlx_lm/models/gpt2.py
  • [x] mlx_lm/models/gpt_bigcode.py
  • [x] mlx_lm/models/internlm2.py
  • [x] mlx_lm/models/llama.py
  • [x] mlx_lm/models/minicpm.py
  • [x] mlx_lm/models/qwen2.py
  • [x] gguf_llm/generate.py
  • [x] gguf_llm/models.py
  • [x] llama/convert.py
  • [x] mistral/convert.py
  • [x] mistral/convert.py
  • [x] mixtral/convert.py
  • [x] mixtral/mixtral.py
  • [x] mlx_lm/lora.py
  • [x] mlx_lm/merge.py
  • [x] mlx_lm/server.py
  • [x] mlx_lm/tuner/trainer.py
  • [x] mlx_lm/tuner/utils.py
  • [x] mlx_lm/utils.py
  • [x] speculative_decoding/decoder.py
  • [x] speculative_decoding/model.py
  • [x] tests/test_server.py

ParamThakkar123 avatar Mar 30 '25 15:03 ParamThakkar123

gguf_llm/generate.py

gguf_llm/models.py

llama/convert.py

mistral/convert.py

mistral/convert.py

mixtral/convert.py

mixtral/mixtral.py

speculative_decoding/decoder.py

speculative_decoding/model.py

tests/test_server.py

@awni where are the following files present in the mlx-lm repo ?? I didn't find any of them

ParamThakkar123 avatar Apr 01 '25 05:04 ParamThakkar123

Some of those files are still in mlx-examples. The files in tests/ should be in mlx-lm now.

awni avatar Apr 02 '25 15:04 awni

@awni can you please review my PR https://github.com/ml-explore/mlx-lm/pull/61. It's almost done and needs your review to proceed and merge

ParamThakkar123 avatar Apr 08 '25 19:04 ParamThakkar123

@awni all files done. Need your review to proceed

ParamThakkar123 avatar Apr 09 '25 17:04 ParamThakkar123