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

Revisit our mypy setup #2156

Closed
idanov opened this issue Dec 22, 2022 · 4 comments
Closed

Revisit our mypy setup #2156

idanov opened this issue Dec 22, 2022 · 4 comments
Assignees
Labels
Issue: Feature Request New feature or improvement to existing feature

Comments

@idanov
Copy link
Member

idanov commented Dec 22, 2022

Description

Currently we are using mypy only in our pre-commit hook and we even don't have it as an explicit test_requirements.txt dependency. If you are using pyright as a language server to explore Kedro's codebase, you will see a lot of type errors and warnings that you wouldn't catch through our pre-commit hook. Also Python's type annotations have evolved since we first started using it and we should catch up with these developments, e.g. the change of PEP 484 which disallows now inexplicit optional definitions:

session_id: str = None

now has to become:

session_id: Optional[str] = None

The goal of this is to end up as close as possible to be able to run mypy --strict on our codebase without any complains.

Context

To improve the quality of our codebase and possibly uncover some bugs hidden bugs.

Possible Implementation

  • Ensure pre-commit and running mypy locally produce the same output
  • Bump up to the latest mypy version in dev requirements (test_requirements.txt)
  • Run mypy --strict and fix as much errors as possible
  • Configure mypy to ignore the errors we cannot fix, e.g. the ones introduced to us by third-party packages

Note: don't run mypy on kedro.extras because we're removing the datasets in 0.19.0

Possible Alternatives

Another option is to move to pyright entirely and use it as a type checker instead of mypy, or maybe even use both?

@noklam
Copy link
Contributor

noklam commented Jul 6, 2023

Second to that, PyLance (which is better than PyRight) have built-in extension for these type hints which is better. Our mypy is very old (v0.961). Simply bumping it to 1.4.1 I get 267 errors. Not all of the errors are important but I think it helps a lot when we get these type correct for the functions that nested in many layers. It will also give us better insight to refactor.

kedro/ipython/__init__.py:78: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
Found 283 errors in 57 files (checked 268 source files)

@astrojuanlu
Copy link
Member

Echoing a comment I made somewhere else about MyPy and pre-commit, see https://jaredkhan.com/blog/mypy-pre-commit

An alternative is to use it as a pytest plugin rather than a linter https://github.com/realpython/pytest-mypy or just call it separately.

@astrojuanlu
Copy link
Member

And I agree upgrading to a recent version of MyPy is warranted. We can probably learn some things from kedro-org/kedro-viz#1419

@merelcht
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Feature Request New feature or improvement to existing feature
Projects
None yet
Development

No branches or pull requests

4 participants