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

RFC: begin using python 3 type hints in codebase #3034

Closed
macintoshpie opened this issue Dec 13, 2021 · 5 comments
Closed

RFC: begin using python 3 type hints in codebase #3034

macintoshpie opened this issue Dec 13, 2021 · 5 comments

Comments

@macintoshpie
Copy link
Contributor

macintoshpie commented Dec 13, 2021

Proposal

SEED's python codebase is somewhat complex in some areas. In particular, some functions return semi-complex data structures.
A newer feature of python is using "type hints" to document and validate functions and variables. Having these annotations provide documentation and can be statically checked to verify typing consistency. Types are statically checked (i.e. not at runtime), and can be progressively added to a codebase. See some examples here.

This proposal is for SEED to begin integrating type hints into its python codebase. Below I've listed some of the Pros and Cons.

Pros/Cons

cons

  • could result in more "chore" work
  • might require some refactoring as we discover typing inconsistencies

pros

  • avoids some types of errors
  • better document code
  • improved IDE experience

other

UPDATE: see google's notes on type hints as well: https://google.github.io/styleguide/pyguide.html#221-type-annotated-code

2.21.2 Pros
Type annotations improve the readability and maintainability of your code. The type checker will convert many runtime errors to build-time errors, and reduce your ability to use Power Features.

2.21.3 Cons
You will have to keep the type declarations up to date. You might see type errors that you think are valid code. Use of a type checker may reduce your ability to use Power Features.

2.21.4 Decision
You are strongly encouraged to enable Python type analysis when updating code. When adding or modifying public APIs, include type annotations and enable checking via pytype in the build system. As static analysis is relatively new to Python, we acknowledge that undesired side-effects (such as wrongly inferred types) may prevent adoption by some projects. In those situations, authors are encouraged to add a comment with a TODO or link to a bug describing the issue(s) currently preventing type annotation adoption in the BUILD file or in the code itself as appropriate.

UPDATE 2: see Django's DEP for adding type hints here: django/deps#65
TLDR; some concerns about new contributions (b/c of upfront cost of learning/understanding how types work), some concerns about code bloat and chore work such as representing complex types, some praise b/c of confidence refactoring, some praise b/c of much better IDE experience (less code digging)

Integration process

  • Developers should add type hints whenever they create new code or touch existing code. E.g. if adding the function foo which calls an existing function bar the developer should add type hints to both functions.
  • We will use a static type checker in CI. There are a few different tools which provide this, maybe we start with google's pytype since it has better type inferencing ability than mypy, but we could definitely switch around if it's not working for us.
  • For IDEs, VSCode can use microsoft's pyright
@Ryoken
Copy link
Contributor

Ryoken commented Dec 13, 2021

I like this. I would even consider "discovering typing inconsistencies" a pro rather than a con as it reveals to us where things are not functioning as we expect.

@macintoshpie
Copy link
Contributor Author

Just ran pytype (which has type inferencing) on the current develop (didn't modify code before running), see output below. It seems like it's catching issues in old code which we should probably remove. Looks like we found a pytype bug (line 166), which would be worked around with an ignore comment...

https://gist.github.com/macintoshpie/78a6c811ae8de0d6b41ba0bdd8c4fb57

@nllong
Copy link
Member

nllong commented Dec 14, 2021

I am for this. I just want to make sure we can lazily apply these updates. I also want to make sure that we focus on the performance work first.

@macintoshpie
Copy link
Contributor Author

@nllong yeah I agree, if we "lazily" add type hints on things we touch when working on tickets it should be a pretty good way to get things rolling. I'll have to look into this a bit more, but mypy is probably going to be our better option considering we want more progressive addition of types (rather than having it check our entire codebase for implicit issues).

I ran mypy on current develop and got these results, like I said I need to spend a sec looking into why these errors are happening
https://gist.github.com/macintoshpie/a3f217e8834c462e55dcc61ccfe554d0

@macintoshpie
Copy link
Contributor Author

TLDR; we decided to progressively include type hints where it's not too much work and could have some payoff.

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

No branches or pull requests

3 participants