Adding mypy type checking to finn.util scripts#1510
Open
rothej wants to merge 2 commits intoXilinx:devfrom
Open
Adding mypy type checking to finn.util scripts#1510rothej wants to merge 2 commits intoXilinx:devfrom
rothej wants to merge 2 commits intoXilinx:devfrom
Conversation
…e-commit Signed-off-by: Joshua Rothe <joshrothe@gmail.com>
Signed-off-by: Joshua Rothe <joshrothe@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR begins addressing issue #1146 by adding mypy config settings, pre-commit settings, and type annotations. Starting with just scripts in /src/finn/utils/ for now, since this is a lot of work (and a lot of potential checking by reviewers) I'm thinking tackling this in stages will be best. For now, config is set to only look at utils/ and ignore all else for mypy checks.
Motivation
Copied and pasted from the Issue:
Personally I found that when developing in FINN, it was immensely helpful to have type hints added. This makes the projects structure very clear and helps especially newcomers to understand how components play together much quicker. Additionally, when having long iteration cycles due to for example synthesis times, having a static type checker such as mypy can prevent some of the errors before the run has even started, potentially saving quite some time.
Workflow
What I've done here for each file is:
mypy --strict src/finn/util/config.pyto see issues.At the end, ran pre-commit to verify it handles mypy correctly.
Note: mypy is added to requirements.txt but this is only really necessary for devs, as pre-commit runs it's own VM with a mirror of mypy in it. It runs ignoring missing args (and not strict) for all files. This can be changed in the future adding the following to .pre-commit-config.yaml:
but I think it is okay to leave out for now mid-migration, so nothing is broken.
Migration Plan
mypy.ini allows existing code to pass without checking, only the utils/ scripts are checked for now. This file will continue to be updated as needed as mypy types are added. My goal if this PR is accepted/merged is to continue this process.