Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Excluding a protobuf directory from mypy in pyproject.toml & pre-commit-config.yaml #16403

Open
BenWolfaardt opened this issue Nov 4, 2023 · 17 comments

Comments

@BenWolfaardt
Copy link

Hey all, thanks for the great product, my code quality has significantly improved since I started using mypy.

I've, however, been struggling all day trying to get mypy to ignore the automatically generated protobuf files - ending with <filename>_pb2.py and <filename>_pb2_grpc.py due to the fact that these files are auto-generated and that we shouldn't be editing them as per: # Generated by the protocol buffer compiler. DO NOT EDIT!

I would like to exclude them. This in and of itself is difficult, let alone trying to get it to happen when running mpy in your CLI as well as in you pre-commit... I've read everything I can find:

My current config:

Python 3.11.4

pre-commit-config.yaml

exclude: ^proto/zkp_auth_pb2_grpc.py$|^proto/zkp_auth_pb2.py$

repos:
  - repo: https://github.com/PyCQA/bandit
  - repo: https://github.com/psf/black
  - repo: https://github.com/pycqa/flake8
  - repo: https://github.com/timothycrosley/isort

  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v1.6.1
    hooks:
      - id: mypy
        args: [--config-file=pyproject.toml]

  - repo: https://github.com/asottile/pyupgrade

pyproject.toml

[tool.mypy]
python_version = "3.11"

check_untyped_defs = true
disallow_untyped_defs = true
incremental = false
ignore_errors = false
pretty = true
show_error_context = true
show_traceback = true
strict_optional = true
warn_incomplete_stub = true
warn_no_return = true
warn_redundant_casts = true
warn_return_any = true
warn_unreachable = true
warn_unused_configs = true
warn_unused_ignores = true

[[tool.mypy.overrides]]
module = [
    "google.protobuf.*",
    "grpc.*",
]
ignore_missing_imports = true

The structure of my project is:

.
├── ci
│   ├── Dockerfile-client
│   └── Dockerfile-server
├── docker-compose.yml
├── poetry.lock
├── proto
│   ├── __init__.py
│   ├── zkp_auth_pb2_grpc.py
│   ├── zkp_auth_pb2.py
│   ├── zkp_auth_pb2.pyi
│   └── zkp_auth.proto
├── pyproject.toml
├── README.md
├── scripts
│   └── setup.sh
├── src
│   ├── client.py
│   ├── __init__.py
│   ├── __main__.py
│   └── server.py
└── tests

A few of the things I tried (excuse my use of regex I'm still very new to it) in my pyproject.toml are:

exclude = '(^|/)proto/)'

exclude = "(^|/)proto/)"

exclude = "(proto/zkp_auth_pb2_grpc.py)"

exclude = "(proto/zkp_auth_pb2_grpc.py|proto/zkp_auth_pb2.py)"

exclude = "(zkp_auth_pb2_grpc.py|zkp_auth_pb2.py)/$"

exclude = """
(?x)(
    zkp_auth_pb2_grpc.py$
)
"""

exclude = "./proto"

exclude = '^proto/'

exclude = [
    '^proto/',
]

exclude = [
    '^proto/zkp_auth_pb2_grpc.py$',
    '^proto/zkp_auth_pb2.py$'
]

exclude = [
    "^proto/zkp_auth_pb2_grpc.py$",
    "^proto/zkp_auth_pb2.py$"
]

[tool.mypy.exclude]
files = [
  '^proto/zkp_auth_pb2_grpc.py$',
  '^proto/zkp_auth_pb2.py$'
]

[tool.mypy.exclude]
files = [
  "^proto/zkp_auth_pb2_grpc.py$",
  "^proto/zkp_auth_pb2.py$"
]

[tool.mypy.files]
exclude = '^(?:proto/|/proto/)'

[mypy.myproject.generated.*]

[tool.mypy.ignore_errors]
module = [
    "proto.*",
]

Furthermore I tried:

  • Using a mypy.ini file instead
  • Fiddling with the pre-commit-config.yaml with regards to the arguments being parsed to it
    • Adding pass_filenames: false
  • Running pypy manually

The part I also don't get is that I was able to commit the auto-generated files even if they had issues, think I had exclude: proto/ in my pre-commit that time, but then that the other files importing these auto-generated protobuf files would result in issues when committing, example:

src/client.py:5: note: In module imported here:
proto/zkp_auth_pb2_grpc.py:

If anyone has any ideas/tips/insights in how I can exclude these files from mypy that would be great appreciated?

Do I potentially need to use: https://github.com/nipunn1313/mypy-protobuf?

@posita I hope you don't mind me tagging you as p[er your post here

@posita
Copy link
Contributor

posita commented Nov 4, 2023

It might help if you can pare down a smaller sample that reproduces the issue?

@erictraut
Copy link

Do I potentially need to use: https://github.com/nipunn1313/mypy-protobuf?

Yes, you should generate type-compatible stubs using the mypy-protobuf protoc extension. The resulting ".pyi" files can be checked in next to the corresponding _pb2.py files. Mypy will use the stubs for type information.

@BenWolfaardt
Copy link
Author

Thanks @erictraut, I've now done so, not sure it's correct, but I used:

  • python -m grpc_tools.protoc -I=. --python_out=. --grpc_python_out=. ./proto/zkp_auth.proto
    • To generate zkp_auth_pb2.py and zkp_auth_pb2_grpc.py
  • protoc --mypy_out=. proto/zkp_auth.proto
    • To generate zkp_auth_pb2.pyi

Surely this could be done with one tool?

@BenWolfaardt
Copy link
Author

BenWolfaardt commented Nov 4, 2023

@posita I'll try and simplify it, I have:

├── proto
│   ├── __init__.py
│   ├── zkp_auth_pb2_grpc.py
│   ├── zkp_auth_pb2.py
│   ├── zkp_auth_pb2.pyi
│   └── zkp_auth.proto
├── src
│   ├── __init__.py
│   ├── __main__.py
│   ├── client.py
│   └── server.py
├── poetry.lock
└── pyproject.toml

And when running mypy either with pre-commit run mypy --all-files or via my pre-commit hook it complains about the proto/zkp_auth_pb2_grpc.py file specifically.

I would thus like to exclude that particular file from having mypy run on it as this file is automatically generated and should something change and it needs to be regenerated I'd like the exclude to be persisted to improve the developer experience.

I tried setting this in both my pyprojects.toml file as well as in the pre-commit-config hook. With no success. I've outlined a couple of my approaches in the last code block in the above issue.

Could you please kindly advise how one can go about correctly ignoring either the specific file mentioned, or the whole ./proto/ directory?

@posita
Copy link
Contributor

posita commented Nov 4, 2023

@BenWolfaardt, I meant four, very brief files:

  1. A minimalist pre-commit-config.yaml containing only the Mypy hook and only enough to repro your issue.
  2. A minimalist pyproject.toml containing only enough of the Mypy config to repro your issue.
  3. A single .py file with a typing error (perhaps num: int = "zero") at a path intended to be checked by Mypy.
  4. A single .py file with a typing error at a path intended to be excluded from Mypy checking.

I assume, given the above that the expected result of running pre-commit --all-files with the above would be that the error in the .py file at no. 3 would be surfaced, but the .py file at no. 4 would not.

@jsimonlane
Copy link

jsimonlane commented Nov 8, 2023

I think we can extend this to mypy.ini exclude regex's don't work, at least in the non regex form.

On top of this, even the simple example in the docs isn't working for me

[mypy]
exclude = (?x)(
    ^one\.py$    # files named "one.py"
    | two\.pyi$  # or files ending with "two.pyi"
    | ^three\.   # or files starting with "three."
  )
➜ 1 backend git:(main) ✗ poetry run mypy . --config-file mypy.ini
mypy.ini: Source contains parsing errors: 'mypy.ini'
	[line  4]: '| two\\.pyi$"""\n'
Success: no issues found in 17 source files

^ weirdly it returns success, but nothing is actually linted.

However, the below worked (in the pyproject.toml)

[tool.mypy]
strict = true
exclude = '''(?x)(
    app/base_queries/*
)'''

@pedropaulofb
Copy link

Configure your pre-commit hook like this:

  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v1.7.0
    hooks:
      - id: mypy
        language: system
        pass_filenames: false
        args: ['.']

Now you're able to correctly set all configurations in your pyproject.toml file. E.g., mine is:

[tool.mypy]
exclude= '^my_project/tests'
strict = true
plugins = "pydantic.mypy"

@posita
Copy link
Contributor

posita commented Nov 15, 2023

I think we can extend this to mypy.ini exclude regex's don't work, at least in the non regex form.

@jsimonlane, what does "in the non regex form" mean in this context? If the doc's examples aren't working, that sounds like a regression (see, e.g., this test). Do we need to file another issue? @BenWolfaardt's original issue was about calling Mypy from pre-commit using pyproject.toml.

UPDATE: The mypy.ini example from the docs works for me as expected:

% \ls
mypy.ini  one.py  tea_4_two.py  this_is_fine.py  three.pyi  typing_error.py
% cat mypy.ini
[mypy]
exclude = (?x)(
    ^one\.py$    # files named "one.py"
    | two\.pyi$  # or files ending with "two.pyi"
    | ^three\.   # or files starting with "three."
  )
% cat one.py
# one.py
This isn't a valid Python file!
% cat tea_4_two.py
# tea_4_two.py
This isn't a valid Python file!
% cat three.pyi
# three.pyi
This isn't a valid Python file!
% cat this_is_fine.py
# this_is_fine.py
i: int = 0
% cat typing_error.py
# typing_error.py
one: int = "1"
% mypy --version
mypy 1.7.0 (compiled: yes)
% mypy .
tea_4_two.py:1: error: unterminated string literal (detected at line 1)  [syntax]
Found 1 error in 1 file (errors prevented further checking)

@posita
Copy link
Contributor

posita commented Nov 15, 2023

@BenWolfaardt, I think I might see the issue. mypy does not honor its own exclude config for paths explicitly passed as arguments:

% \ls
code.py  pyproject.toml  test.py
% cat pyproject.toml
[tool.mypy]
exclude = '''(?x)(
    (^|/)test[^/]*\.py$    # files named "test*.py"
  )'''
% cat code.py
# code.py
i: int = 0
% cat test.py
# test.py
This isn't a valid Python file!
% mypy .
Success: no issues found in 1 source file
% mypy test.py
test.py:2: error: unterminated string literal (detected at line 2)  [syntax]
Found 1 error in 1 file (errors prevented further checking)

I think this is by design, but haven't researched it. I'm also guessing pre-commit passes the paths of changed files directly to mypy as arguments, meaning those will get checked. If my hunch is correct, you might have to represent your desired excludes in pre-commit-config.yaml as well (probably via a top-level exclude) so that pre-commit never passes those paths in the first place?

I see you have one already, but I'm not sure this is a Mypy bug if pre-commit is passing arguments to Mypy that it should be excluding? What happens if you try the following in your pre-commit-config.yaml instead?

# (hopefully) ignore all python files in (sub)directories named "proto"
exclude: (^|/)proto/.*\.py$

@pedropaulofb
Copy link

@BenWolfaardt, I think I might see the issue. mypy does not honor its own exclude config for paths explicitly passed as arguments:

% \ls
code.py  pyproject.toml  test.py
% cat pyproject.toml
[tool.mypy]
exclude = '''(?x)(
    (^|/)test[^/]*\.py$    # files named "test*.py"
  )'''
% cat code.py
# code.py
i: int = 0
% cat test.py
# test.py
This isn't a valid Python file!
% mypy .
Success: no issues found in 1 source file
% mypy test.py
test.py:2: error: unterminated string literal (detected at line 2)  [syntax]
Found 1 error in 1 file (errors prevented further checking)

I think this is by design, but haven't researched it. I'm also guessing pre-commit passes the paths of changed files directly to mypy as arguments, meaning those will get checked. If my hunch is correct, you might have to represent your desired excludes in pre-commit-config.yaml as well (probably via a top-level exclude) so that pre-commit never passes those paths in the first place?

I see you have one already, but I'm not sure this is a Mypy bug if pre-commit is passing arguments to Mypy that it should be excluding? What happens if you try the following in your pre-commit-config.yaml instead?

# (hopefully) ignore all python files in (sub)directories named "proto"
exclude: (^|/)proto/.*\.py$

Hi @posita , please take a look on this answer: #16403 (comment)

@posita
Copy link
Contributor

posita commented Nov 15, 2023

Hi @posita , please take a look on this answer: #16403 (comment)

I saw that. Does that mean this issue should be closed?

As an aside, I think that work-around works because it (in effect) tells pre-commit to no longer pass just the changed files to sub-commands/hooks, in which case it's basically equivalent to calling mypy . (instead of mypy first-changed-file second-changed-file ...). This is probably fine for small code-bases. My guess is that the default behavior for pre-commit passing only changed files is by design for at least two reasons: first, by only focusing on changed files, it probably reduces noise associated with any secondary effects and allows the developer to hone in on root causes more easily (most of the time); and second, for large code bases, this might speed up execution time dramatically.

@jsimonlane
Copy link

@posita, looking further it seems that the parser actually works correctly... though it is confusing, as I'm seeing text mypy.ini: Source contains parsing errors: 'mypy.ini'.

So it looks like the functionality works... but odd I'm seeing the error.

Basically, when I run mypy with a multi-line regex, I get the error: mypy.ini: Source contains parsing errors: 'mypy.ini'.

When I use a regular (single-line) regex, excluding works.

I also tried this on version 1.70 just now, same issue.

➜  backend git:(main) ✗ poetry run mypy . 
mypy.ini: Source contains parsing errors: 'mypy.ini'
	[line  6]: ")'''\n"
Success: no issues found in 14 source files
➜  backend git:(main) ✗ poetry run mypy --version 
mypy 1.6.1 (compiled: yes)
➜  backend git:(main) ✗ cat mypy.ini
[mypy]
strict = true
exclude = '''(?x)(
    app/base_queries/*
    | two\.pyi$  # or files ending with "two.pyi"
)'''

@posita
Copy link
Contributor

posita commented Nov 15, 2023

@jsimonlane, ini files don't support triple quotes (not sure what was wrong with your original example, though). There is a call-out of some subtle differences between the two formats (note the note about TOML in the docs), but please feel free to suggest how to make that clearer. I think we're getting away from the original issue, though?

@pedropaulofb
Copy link

I agree with @posita. Could @BenWolfaardt solve his problem with the configuration suggested in this thread?

@ZeN220
Copy link

ZeN220 commented Dec 14, 2023

I also had the same problem. The solution #16403 (comment) is not the best for me, because as said before, it affects all files in the project, which is not always necessary

@mahenzon
Copy link

Hey there. I did it this way:

mypy config in pyproject.toml

[tool.mypy]
# ...
exclude = [
    "tests",
]

This works when running mypy manually:

mypy ./

And for pre-commit I excluded tests files again. .pre-commit-config.yaml file:

repos:
  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v1.9.0
    hooks:
      - id: mypy
        exclude: tests

@hauntsaninja
Copy link
Collaborator

See also: #13916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants