Add minimal 'mypy' run to the pre-commit hooks. #4176

greschd · 2020-06-15T14:40:55Z

Currently, only two files are checked: aiida/tools/groups/paths.py and aiida/engine/processes/calcjobs/calcjob.py. The option follow_imports is set to skip, because there are a lot of issues in other files, mostly because these lack type hints.
Even on the two checked files, the strictness level of mypy is minimal (see the commented options in mypy.ini).

From here, we can continue expanding the type hints one file at
a time:

add the file to files in the mypy pre-commit hook
fix all issues that mypy reports

@chrisjsewell I had to change some of the type hints in groups/paths.py - please check that these are correct.

chrisjsewell · 2020-06-15T14:46:56Z

Yeh but you know we can't add these proper type annotations, until we drop python 3.5 support in October: https://devguide.python.org/#status-of-python-branches

sphuber · 2020-06-15T14:49:36Z

Yeh but you know we can't add these proper type annotations, until we drop python 3.5 support in October: https://devguide.python.org/#status-of-python-branches

These are only method argument and return value annotations, which is supported by Python 3.5. We just cannot add variable annotations because that was only added in Python 3.6.

Anyway, I really cannot wait for pyproject.toml to catch on and we can do away with the hundreds of different configuration files in the repo root for all the different tools we run...

greschd · 2020-06-15T14:50:16Z

Only variable annotations and lazy annotations are not supported, right? Need to fix the lazy thing, though..

chrisjsewell · 2020-06-15T14:51:53Z

We just cannot add variable annotations because that was only added in Python 3.6

Ah fair enough brilliant, for some reason I thought it was 3.5, otherwise I would have already added them!

chrisjsewell · 2020-06-15T15:01:52Z

Only variable annotations and lazy annotations are not supported, right?

Well the other big thing in the pipeline is TypedDict (introduced in 3.8). Having worked with interfaces in TypeScript, which are similar, these will be so helpful.

Other than that LGTM 👍

greschd · 2020-06-15T15:05:32Z

@chrisjsewell just changed something more: get_or_create_group returns Tuple[orm.Group, bool], not Tuple['GroupPath', bool], right?

greschd · 2020-06-15T15:08:15Z

Ah, and

def walk_nodes(self, filters=None, node_class=None, query_batch=None) -> Iterable[WalkNodeResult]:

to

def walk_nodes(self, filters=None, node_class=None, query_batch=None) -> Iterator[WalkNodeResult]:

aiida/tools/groups/paths.py

codecov · 2020-06-15T15:19:02Z

Codecov Report

Merging #4176 into develop will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4176      +/-   ##
===========================================
+ Coverage    78.94%   78.94%   +0.01%     
===========================================
  Files          467      467              
  Lines        34501    34510       +9     
===========================================
+ Hits         27232    27242      +10     
+ Misses        7269     7268       -1

Flag	Coverage Δ
#django	`70.87% <100.00%> (+0.02%)`	⬆️
#sqlalchemy	`71.75% <100.00%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
aiida/tools/groups/paths.py	`91.62% <100.00%> (+0.48%)`	⬆️
aiida/transports/plugins/local.py	`80.47% <0.00%> (+0.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41f5d5d...23e1eb8. Read the comment docs.

greschd · 2020-06-15T16:09:09Z

 sqlalchemy.exc.TimeoutError: QueuePool limit of size 5 overflow 0 reached, connection timed out, timeout 1 (Background on this error at: http://sqlalche.me/e/3o7r)

Is this a known flaky test?

aiida/tools/groups/paths.py

sphuber · 2020-06-16T06:05:07Z

 sqlalchemy.exc.TimeoutError: QueuePool limit of size 5 overflow 0 reached, connection timed out, timeout 1 (Background on this error at: http://sqlalche.me/e/3o7r)

Is this a known flaky test?

I haven't seen it fail yet, but it is relatively new. This looks like it comes from a test that was put in to check the REST API cleanly closing the SqlAlchemy sessions so we don't get a timeout because the queuepool is exhausted. Clearly this was triggered here where we run with reduced pool size and overflow, but don't see how your changes should have affected it. So I would just chalk this up to bad luck for now and ignore it until we start to see it more often

setup.json

sphuber · 2020-06-16T06:53:52Z

BTW, I have just been reading up on a 2-year long discussion on mypy adding support for pyproject.toml. Seems it might be getting close to getting there: python/mypy#5208
pytest also just merged it and should be releasing support with v6.0

greschd · 2020-06-16T07:45:06Z

FYI, there is a potential for confusion because our orm and the stdlib typing share some names, specifically Dict and List.

In my own packages I solved this by

import typing as ty
<...>
ty.Dict

I'm open for better suggestions, but since this is a problem that's bound to happen in many places in our code base, it's probably a good idea to have a consistent style.

Should probably be changed in paths.py once we agree on the style.

Currently, only two files are checked: aiida/tools/groups/paths.py and aiida/engine/processes/calcjobs/calcjob.py. The option 'follow_imports' is set to 'skip', because there are a lot of issues in other files, mostly because these lack type hints. From here, we can continue expanding the type hints one file at a time: - add the file to 'files' in the 'mypy' pre-commit hook - fix all issues that mypy reports Co-authored-by: Chris Sewell <[email protected]>

sphuber · 2020-06-18T21:48:42Z

mypy.ini

+; Strictness settings
+; disallow_any_unimported = True
+; disallow_any_expr = True
+; disallow_any_decorated = True
+; disallow_any_explicit = True
+; disallow_any_generics = True
+; disallow_subclassing_any = True
+
+; disallow_untyped_calls = True
+; disallow_untyped_defs = True
+; disallow_incomplete_defs = True
+; disallow_untyped_decorators = True
+
+; no_implicit_optional = True
+; no_strict_optional = False
+
+; Enable all warnings
+; warn_redundant_casts = True
+; warn_unused_ignores = True
+; warn_return_any = True
+; warn_unreachable = True
+
+; allow_untyped_globals = False
+; strict_equality = True


can we get rid of all these if we are not using them?

Yeah, although it might be nice seeing them listed here since (I think) the goal would be to slowly enable some of them.

sphuber · 2020-06-18T21:49:26Z

mypy.ini

+ignore_missing_imports = True
+
+[mypy-plumpy.*]
+ignore_missing_imports = True


Is this similar to pylint issuing missing import warnings if it is not being run in the "system" environment? If so, would it make sense to also run mypy under system?

Mypy.. has its own "import system" - the problem here is that when dependencies don't have type annotations, that will be reported as a failure.

For now I'm not sure this makes a difference since we also use skip for the follow_imports option, but we definitely want to get rid of that one eventually.

The import mechanism is described here: https://mypy.readthedocs.io/en/stable/running_mypy.html#how-mypy-handles-imports

sphuber · 2020-06-20T10:59:05Z

Thanks @greschd

greschd requested review from chrisjsewell and sphuber June 15, 2020 14:40

greschd force-pushed the add_minimal_mypy_run branch 2 times, most recently from 75f85e1 to eb7701d Compare June 15, 2020 15:00

chrisjsewell suggested changes Jun 15, 2020

View reviewed changes

aiida/tools/groups/paths.py Outdated Show resolved Hide resolved

aiida/tools/groups/paths.py Outdated Show resolved Hide resolved

aiida/tools/groups/paths.py Outdated Show resolved Hide resolved

greschd force-pushed the add_minimal_mypy_run branch from 1e96327 to 2e1451b Compare June 15, 2020 15:16

greschd force-pushed the add_minimal_mypy_run branch from 2e1451b to a27f94c Compare June 15, 2020 15:34

greschd requested a review from chrisjsewell June 15, 2020 16:09

chrisjsewell suggested changes Jun 15, 2020

View reviewed changes

aiida/tools/groups/paths.py Outdated Show resolved Hide resolved

chrisjsewell previously approved these changes Jun 15, 2020

View reviewed changes

greschd dismissed chrisjsewell’s stale review via 73af1c7 June 15, 2020 19:32

greschd force-pushed the add_minimal_mypy_run branch 2 times, most recently from 73af1c7 to 1024b59 Compare June 15, 2020 19:40

greschd requested a review from chrisjsewell June 15, 2020 20:11

chrisjsewell previously approved these changes Jun 15, 2020

View reviewed changes

sphuber reviewed Jun 16, 2020

View reviewed changes

setup.json Outdated Show resolved Hide resolved

greschd dismissed chrisjsewell’s stale review via ab8b494 June 16, 2020 07:21

greschd force-pushed the add_minimal_mypy_run branch from 1024b59 to ab8b494 Compare June 16, 2020 07:21

greschd requested review from sphuber and chrisjsewell June 16, 2020 08:00

chrisjsewell previously approved these changes Jun 16, 2020

View reviewed changes

greschd dismissed chrisjsewell’s stale review via a896fe3 June 16, 2020 18:09

greschd force-pushed the add_minimal_mypy_run branch from ab8b494 to a896fe3 Compare June 16, 2020 18:09

sphuber reviewed Jun 18, 2020

View reviewed changes

Merge branch 'develop' into add_minimal_mypy_run

23e1eb8

sphuber approved these changes Jun 20, 2020

View reviewed changes

sphuber merged commit e9d3153 into aiidateam:develop Jun 20, 2020

sphuber deleted the add_minimal_mypy_run branch June 20, 2020 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add minimal 'mypy' run to the pre-commit hooks. #4176

Add minimal 'mypy' run to the pre-commit hooks. #4176

greschd commented Jun 15, 2020

chrisjsewell commented Jun 15, 2020

sphuber commented Jun 15, 2020

greschd commented Jun 15, 2020

chrisjsewell commented Jun 15, 2020

chrisjsewell commented Jun 15, 2020

greschd commented Jun 15, 2020

greschd commented Jun 15, 2020

codecov bot commented Jun 15, 2020 •

edited

Loading

greschd commented Jun 15, 2020 •

edited

Loading

sphuber commented Jun 16, 2020

sphuber commented Jun 16, 2020

greschd commented Jun 16, 2020 •

edited

Loading

sphuber Jun 18, 2020

greschd Jun 18, 2020

sphuber Jun 18, 2020

greschd Jun 18, 2020

sphuber commented Jun 20, 2020

Add minimal 'mypy' run to the pre-commit hooks. #4176

Add minimal 'mypy' run to the pre-commit hooks. #4176

Conversation

greschd commented Jun 15, 2020

chrisjsewell commented Jun 15, 2020

sphuber commented Jun 15, 2020

greschd commented Jun 15, 2020

chrisjsewell commented Jun 15, 2020

chrisjsewell commented Jun 15, 2020

greschd commented Jun 15, 2020

greschd commented Jun 15, 2020

codecov bot commented Jun 15, 2020 • edited Loading

Codecov Report

greschd commented Jun 15, 2020 • edited Loading

sphuber commented Jun 16, 2020

sphuber commented Jun 16, 2020

greschd commented Jun 16, 2020 • edited Loading

sphuber Jun 18, 2020

Choose a reason for hiding this comment

greschd Jun 18, 2020

Choose a reason for hiding this comment

sphuber Jun 18, 2020

Choose a reason for hiding this comment

greschd Jun 18, 2020

Choose a reason for hiding this comment

sphuber commented Jun 20, 2020

codecov bot commented Jun 15, 2020 •

edited

Loading

greschd commented Jun 15, 2020 •

edited

Loading

greschd commented Jun 16, 2020 •

edited

Loading