add mypy to style checks; rename `spack flake8` to `spack style` by trws · Pull Request #20384 · spack/spack

trws · 2020-12-14T21:38:56Z

I lost my mind a bit after getting the completion stuff working and decided to get Mypy working for spack as well. This adds a .mypy.ini that checks all of the spack and llnl modules, though not yet packages, and fixes all of the identified missing types and type issues for the spack library.

Depends on #20376 because of how I built this up, but could probably rebase that out if it's an issue.

In a related note, how would folks feel about having a Mypy run in CI?

adamjstewart · 2020-12-14T22:19:31Z

It would be nice to rebase out #20376, otherwise we have to wait for it to be merged before this can be reviewed.

I would love to have mypy run in CI! My naive understanding on mypy was that it used Python's new typing system (Python 3.5+), but I guess you can also use it via comments. How does it work if not all variables/function args have an explicitly defined type? I know Sphinx is setup to read type hints to function documentation. Does this work with comment type hints as well? I don't want to have to duplicate everything, but it would be nice to check types on everything.

trws · 2020-12-14T22:42:21Z

Sure, I'll rebase that out shortly.

As to typing, yes that's exactly what it does. That's part of why I have to do the funny try/import/catch dance, since there's no backport of typing to 2.6 but the types have to be imported in the version being checked. For variables, it infers a type based on the type of the first assignment, and for functions it assumes something like Any. It's very permissive until you give it some type annotations, but as you add more annotations it gets progressively more strict. Most of the changes in this PR are actually clarifying types it couldn't infer, or that it inferred incorrectly based on first assignment.

The sphinx thing I'm not sure, I would think the comment type hints would work, but have never tried that TBH.

adamjstewart · 2020-12-14T22:47:33Z

With Mypy, is there a "strict mode" to ensure that all variables and functions are properly typed? We're obviously nowhere near ready for this, but if we ever drop Python 2 support I would love to enforce that for all functions.

trws · 2020-12-14T22:49:58Z

Rebased here, I meant. Should not work on two similar patches quite like this.

Yep, there is definitely a strict mode:

  --strict                  Strict mode; enables the following flags: --warn-unused-configs, --disallow-any-generics, --disallow-subclassing-any, --disallow-untyped-calls, --disallow-untyped-defs, --disallow-incomplete-defs, --check-untyped-defs, --disallow-untyped-decorators, --no-implicit-optional, --warn-redundant-casts, --warn-unused-ignores, --warn-return-any, --no-implicit-reexport, --strict-equality

One upside to the approach they took to gradual typing is that the vast majority of this stuff can be done on a per-module basis, so if some things are ready earlier than others we can make the checking on them stricter without impacting the whole codebase.

adamjstewart · 2020-12-14T22:53:25Z

I've been using Python 3.5+ type hints in all of my research code for a while, so I've wanted them for Spack too. Unfortunately, until we drop Python 2 support, we can't use those directly, only the comments. I would rather not push for the use of comments (other than the minimal changes you made here).

The other complaint I have about Mypy is that it doesn't work unless all of your dependencies also use Mypy. Like, my research code was mostly numpy/pandas, and those libraries don't yet have full type hint support yet, so that was frustrating.

trws · 2020-12-15T06:20:42Z

Generally I'd agree. It would be nice to have some type annotations on high use APIs for scripts and packages, but probably not globally.

The dependency issue is one place where Spack's vendoring policy actually helps a lot. Since all the source is included in the project, rather than installed and accessed through a site-packages or something, the stubs are unnecessary because mypy will follow the import and do type inference through all the externals as well. At the moment, I have it ignoring jinja2 because there's a module in there that needs the async syntax, but every other external has its internal errors ignored but contributes inferred types to checking spack.

trws · 2020-12-15T17:51:45Z

Looks like everything is happy but gitlab, and I don't seem to have access to the results to see why. Should probably figure out how to get access.

I dug into getting Mypy working on packages. There is no good way to do this, at all, because it doesn't support loading multiple modules with the same name in the same invocation and all packages are treated as being named "package", their directories also don't have valid module names. We could make a spack mypy command that would generate a directory of symlinks renamed to module-compatible names to run it on, but that's not a very nice solution. The pyright type checker can type-check all of the packages as they are in a single invocation, but throws a huge number of errors on some spack library code for dynamic fields and naming conventions where Mypy does not. Probably need to put some thought into how to deal with that.

adamjstewart · 2020-12-15T20:12:03Z

I don't think I have access to Gitlab either, I've just been ignoring it cuz it isn't required in order for the PR to be merged.

trws · 2020-12-15T20:16:13Z

Did a bit more digging, it seems like two packages being built in the pipeline that spuriously fail to build failed.

tgamblin

LGTM but 3 change requests:

Integrate mypy checking with spack flake8 (if mypy is available)
Change spack flake8 to spack style and leave the old flake8 command so that it prints out that it's deprecated.
Add a dummy typing module in lib/spack/external/py26/ so that we don't have to catch ImportError for every typing import

adamjstewart · 2020-12-22T18:00:27Z

I support renaming spack flake8 to spack style, especially since we plan to add black someday.

This allows type checking of spack and llnl library code, not yet packages as there isn't an obvious way to deal with the missing pkgkit import. Currently running mypy with this results in 106 errors across 46 files out of the 430 files it scans.

This reduces the spack lib from 106 type missing or mismatch issues to 0. Should be just about good enough to hit CI from here.

Currently aliases flake8 to style, and just runs flake8 as before. Will add mypy support after this.

Allow "import typing" and "from typing import ..." in spack without a try/except dance to support old versions. Doing it this way means we can only reliably use all type hints in python3.7+, so the mypy.ini will be updated to reflect that.

trws · 2020-12-22T21:06:08Z

All checked off. The last one I went a slightly different direction with it and added another directory like py26 for python2, so we can just do import typing and have it do something sane.

Also re-format style.py since it no longer passed the black style check with the new length.

Update the style and docs action to use `spack style`, adding in mypy and black to the action even if it isn't running black right now.

tgamblin

This looks great -- minor change requests.

tgamblin · 2020-12-22T22:21:20Z

lib/spack/spack/cmd/flake8.py

-            changed.add(f)
-
-    return sorted(changed)
+description = spack.cmd.style.description


This should probably say something about deprecation instead of just passing through the spack style description.

tgamblin · 2020-12-22T22:24:08Z

lib/spack/spack/cmd/flake8.py

-        sys.exit(1)
-    else:
-        print("Flake8 checks were clean.")
+    print("=======================================================")


can this stuff be tty.warn? Users will recognize that. Suggest replacing everything with:

tty.warn("`spack flake8` is deprecated", "please use `spack style` to run style checks.")

tgamblin · 2020-12-22T22:28:53Z

lib/spack/spack/package.py

-    _InstallPhase_run_after = {}
+    # These are accessed only through getattr, by name
+    _InstallPhase_run_before = {}  # type: Dict[str, List[Callable]]
+    _InstallPhase_run_after = {}  # type: Dict[str, List[Callable]]


This is actually awesome because previously I had to remember how this was structured.

tgamblin

One more thing. We should update this in CI and update the docs. Specifically:

trws · 2020-12-22T22:58:26Z

Ok, I think that covers the comments. Have a look when you get a chance. Hopefully this will also cover the completion and other issues I was seeing in the CI.

…ck#20384) I lost my mind a bit after getting the completion stuff working and decided to get Mypy working for spack as well. This adds a `.mypy.ini` that checks all of the spack and llnl modules, though not yet packages, and fixes all of the identified missing types and type issues for the spack library. In addition to these changes, this includes: * rename `spack flake8` to `spack style` Aliases flake8 to style, and just runs flake8 as before, but with a warning. The style command runs both `flake8` and `mypy`, in sequence. Added --no-<tool> options to turn off one or the other, they are on by default. Fixed two issues caught by the tools. * stub typing module for python2.x We don't support typing in Spack for python 2.x. To allow 2.x to support `import typing` and `from typing import ...` without a try/except dance to support old versions, this adds a stub module *just* for python 2.x. Doing it this way means we can only reliably use all type hints in python3.7+, and mypi.ini has been updated to reflect that. * add non-default black check to spack style This is a first step to requiring black. It doesn't enforce it by default, but it will check it if requested. Currently enforcing the line length of 79 since that's what flake8 requires, but it's a bit odd for a black formatted project to be quite that narrow. All settings are in the style command since spack has no pyproject.toml and I don't want to add one until more discussion happens. Also re-format `style.py` since it no longer passed the black style check with the new length. * use style check in github action Update the style and docs action to use `spack style`, adding in mypy and black to the action even if it isn't running black right now.

) I lost my mind a bit after getting the completion stuff working and decided to get Mypy working for spack as well. This adds a `.mypy.ini` that checks all of the spack and llnl modules, though not yet packages, and fixes all of the identified missing types and type issues for the spack library. In addition to these changes, this includes: * rename `spack flake8` to `spack style` Aliases flake8 to style, and just runs flake8 as before, but with a warning. The style command runs both `flake8` and `mypy`, in sequence. Added --no-<tool> options to turn off one or the other, they are on by default. Fixed two issues caught by the tools. * stub typing module for python2.x We don't support typing in Spack for python 2.x. To allow 2.x to support `import typing` and `from typing import ...` without a try/except dance to support old versions, this adds a stub module *just* for python 2.x. Doing it this way means we can only reliably use all type hints in python3.7+, and mypi.ini has been updated to reflect that. * add non-default black check to spack style This is a first step to requiring black. It doesn't enforce it by default, but it will check it if requested. Currently enforcing the line length of 79 since that's what flake8 requires, but it's a bit odd for a black formatted project to be quite that narrow. All settings are in the style command since spack has no pyproject.toml and I don't want to add one until more discussion happens. Also re-format `style.py` since it no longer passed the black style check with the new length. * use style check in github action Update the style and docs action to use `spack style`, adding in mypy and black to the action even if it isn't running black right now.

…ck#20384) I lost my mind a bit after getting the completion stuff working and decided to get Mypy working for spack as well. This adds a `.mypy.ini` that checks all of the spack and llnl modules, though not yet packages, and fixes all of the identified missing types and type issues for the spack library. In addition to these changes, this includes: * rename `spack flake8` to `spack style` Aliases flake8 to style, and just runs flake8 as before, but with a warning. The style command runs both `flake8` and `mypy`, in sequence. Added --no-<tool> options to turn off one or the other, they are on by default. Fixed two issues caught by the tools. * stub typing module for python2.x We don't support typing in Spack for python 2.x. To allow 2.x to support `import typing` and `from typing import ...` without a try/except dance to support old versions, this adds a stub module *just* for python 2.x. Doing it this way means we can only reliably use all type hints in python3.7+, and mypi.ini has been updated to reflect that. * add non-default black check to spack style This is a first step to requiring black. It doesn't enforce it by default, but it will check it if requested. Currently enforcing the line length of 79 since that's what flake8 requires, but it's a bit odd for a black formatted project to be quite that narrow. All settings are in the style command since spack has no pyproject.toml and I don't want to add one until more discussion happens. Also re-format `style.py` since it no longer passed the black style check with the new length. * use style check in github action Update the style and docs action to use `spack style`, adding in mypy and black to the action even if it isn't running black right now.

adamjstewart added the mypy label Dec 14, 2020

trws force-pushed the mypy-config branch from 7a4c709 to 8d51d06 Compare December 14, 2020 22:47

tgamblin requested changes Dec 22, 2020

View reviewed changes

tgamblin added flake8 style labels Dec 22, 2020

trws added 10 commits December 22, 2020 10:32

qa/mypy: make spack pass the mypy config

d0e7d91

This reduces the spack lib from 106 type missing or mismatch issues to 0. Should be just about good enough to hit CI from here.

fix imports and missing indirectly referenced members

76e1877

appease vermin

ca430b1

fix comment spacing for type hints

f7a0d83

fix novm comment spacing

3fc4087

clean up flake8 and mypy

9dcdcf2

remove errant debugging print

07661b8

fix module/None mismatch in asp.py

22e55ad

add a newline to make formatting consistent and kick off build pipeline

e9d4a42

trws force-pushed the mypy-config branch from c996a2b to e9d4a42 Compare December 22, 2020 18:35

trws added 4 commits December 22, 2020 10:51

rename spack flake8 to spack style

6772252

Currently aliases flake8 to style, and just runs flake8 as before. Will add mypy support after this.

stub typing module for python2.x

d1eb260

Allow "import typing" and "from typing import ..." in spack without a try/except dance to support old versions. Doing it this way means we can only reliably use all type hints in python3.7+, so the mypy.ini will be updated to reflect that.

update mypy.ini to 3.7 mode and ignore spack.pkg.*

0897b8d

remove now unnecessary import checks for typing

83285c5

trws added 6 commits December 22, 2020 13:10

tell vermine to ignore the typing import in package

afb1a0d

add black appropriate ignores and length in flake8

5401480

Also re-format style.py since it no longer passed the black style check with the new length.

use style check in github action

c90555f

Update the style and docs action to use `spack style`, adding in mypy and black to the action even if it isn't running black right now.

rename stage to refer to style

51a02fe

fix up spack.test.cmd.flake8 to get its definitions from style

eaf37de

more flake8 test fixes

88a9cba

tgamblin requested changes Dec 22, 2020

View reviewed changes

tgamblin changed the title ~~Mypy setup and fixing what falls out~~ style: add mypy to style checks, rename spack flake8 to spack style Dec 22, 2020

tgamblin changed the title ~~style: add mypy to style checks, rename spack flake8 to spack style~~ style: add mypy to checks, rename spack flake8 to spack style Dec 22, 2020

tgamblin added the black label Dec 22, 2020

tgamblin changed the title ~~style: add mypy to checks, rename spack flake8 to spack style~~ add mypy to style checks, rename spack flake8 to spack style Dec 22, 2020

tgamblin changed the title ~~add mypy to style checks, rename spack flake8 to spack style~~ add mypy to style checks; rename spack flake8 to spack style Dec 22, 2020

trws added 4 commits December 22, 2020 14:45

add is_package back, needed for tests

abb0961

update completion with style

f51b889

re-work deprecation warning and description

fffd0a8

doc updates to refer to style and new flake8 capabilities

987fe0e

shorten description of deprecated flake8 command

48b4f53

tgamblin approved these changes Dec 22, 2020

View reviewed changes

install mypy on mac, test flake8 sans mypy

6f4a021

tgamblin merged commit 857749a into spack:develop Dec 23, 2020

tgamblin mentioned this pull request Dec 23, 2020

style: ensure that all packages pass spack style -a #20538

Merged

2 tasks

haampie mentioned this pull request Sep 17, 2021

Backports for 0.16.3 #25991

Merged

11 tasks

alalazo mentioned this pull request Jul 11, 2023

Patching stages relies on side effects from memoization #19670

Closed

3 tasks

Conversation

trws commented Dec 14, 2020

Uh oh!

adamjstewart commented Dec 14, 2020

Uh oh!

trws commented Dec 14, 2020

Uh oh!

adamjstewart commented Dec 14, 2020

Uh oh!

trws commented Dec 14, 2020

Uh oh!

adamjstewart commented Dec 14, 2020

Uh oh!

trws commented Dec 15, 2020

Uh oh!

trws commented Dec 15, 2020

Uh oh!

adamjstewart commented Dec 15, 2020

Uh oh!

trws commented Dec 15, 2020

Uh oh!

tgamblin left a comment • edited by trws Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented Dec 22, 2020

Uh oh!

trws commented Dec 22, 2020

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

tgamblin Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

tgamblin Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

tgamblin Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

tgamblin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trws commented Dec 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgamblin left a comment •

edited by trws

Loading

tgamblin left a comment •

edited

Loading