Add type check to CI - Githubissues

unitaryfund / mitiq

Mitiq is an open source toolkit for implementing error mitigation techniques on most current intermediate-scale quantum computers.

https://mitiq.readthedocs.io

GNU General Public License v3.0

359 stars 157 forks source link

Add type check to CI #489

Closed rmlarose closed 3 years ago

rmlarose commented 3 years ago

We had a PR (#326) to add mypy to CI, but while this PR fixed mypy errors it never actually added a job to CI. Do we want to add this?

rmlarose commented 3 years ago

Agreed to add type check @ mitiq meeting.

crazy4pi314 commented 3 years ago

Mitiq meeting notes: The scope here would be adding the CI check and then squashing any resulting issues one master.

crazy4pi314 commented 3 years ago

So there are 466 errors to fix on master, this is gonna take a while...

nathanshammah commented 3 years ago

Just clarifying that a solution to this issue for UnitaryHACK may build upon #556 by @crazy4pi314.

TripleRD commented 3 years ago

Hi @crazy4pi314 @rmlarose @nathanshammah, I would like to work on this issue. Just for clarification, following the checklist in PR #556 should be enough or do I have to work on anything specific? Thanks!

rmlarose commented 3 years ago

That checklist for generic PRs. (Note you'll see a slightly different one if you open a new PR.) The steps to fix this issue are:

Run make check-types and fix all mypy errors.
Uncomment the type check in CI to make sure we're tracking it in CI.

TripleRD commented 3 years ago

Okay, and does all the code need to be annotated? I went through the discussion in the PR there it was being discussed on ignoring type checking on test files (hence no annotation required for the test files).

TripleRD commented 3 years ago

Okay, and does all the code need to be annotated? I went through the discussion in the PR there it was being discussed on ignoring type checking on test files (hence no annotation required for the test files).

@crazy4pi314 @rmlarose @nathanshammah

crazy4pi314 commented 3 years ago

I think it all should be annotated, but I am not sure what the rest of the team thinks. Also gonna ping @andreamari

rmlarose commented 3 years ago

No need to annotate tests.

andreamari commented 3 years ago

I also think that excluding tests is fine. In this case, it may be necessary to find some trick for telling mypy to exclude tests unless this is automatic.

TripleRD commented 3 years ago

Okay, thanks for clearing the doubts.

TripleRD commented 3 years ago

I have updated some files with type annotations and updated the CI for type checking the non-test files. Should I create a PR or PR them all together after updating the rest of the files?

rmlarose commented 3 years ago

Thanks @TripleR47! It's fine to do separate PRs as you describe (would be good if you're running mypy on the files you added annotations for).

TripleRD commented 3 years ago

Yes, I'm running the mypy on the annotated files. And will it be okay if I push the future changes in the same PR? Thanks!

TripleRD commented 3 years ago

Hey @rmlarose, I created a PR, but it failed the validate and the unit test. I went through the error messages and the source code. But the source code looks fine. Any suggestions on how to work on it? Thanks!

rmlarose commented 3 years ago

Yep! I think just adding mypy to dev_requirements.txt will solve all (most) of the failures. make check-types failed because mypy wasn't installed, and some tests failed I think because of type checking also.

TripleRD commented 3 years ago

Okay, updating the dev_requirements.txt file.