Add type annotations

[ccrvlh]

First draft of some type annotations. This will make Mypy complain a lot, but it's a first step on making the cobase leverage type annotations benefits. I'll keep this as a draft for now. One of the decisions to make is whether to add lots of Any for variables with unknown types (or multiple type possibilities), or just to not add the annotation for now.

Ran all tests (both unit and end-to-end with ssh and docker) and everything seems fine (I'm on a macOS with Python 3.10). I also could add tox with the other Python versions to increase test reliability, I don't think I used anything version specific (such as | or TypedDicts and such), so this should be safe.

[Fizzadar]

Thank you for splitting this out @lowercase00! Pushed a few bits and a workflow change so we can start iterating on the mypy warnings.

I'm keen to get this merged sooner than later - to avoid code divergence. I would rather we used Any where types are hard currently to make this process quicker. Then we can revisit all the Any instances one by one off the main branch. Also means the project can benefit from type checking sooner!

[ccrvlh]

@Fizzadar sorry the delay! Sure, just to make sure I understood correctly, should we add Any anywhere Mypy shows warnings to merge asap (making sure there are no mypy warnings at all), and then gradually review each Any, is that correct?

[Fizzadar]

@Fizzadar sorry the delay! Sure, just to make sure I understood correctly, should we add Any anywhere Mypy shows warnings to merge asap (making sure there are no mypy warnings at all), and then gradually review each Any, is that correct?

@lowercase00 nw at all! Yes I think that's reasonable to avoid diverging the branches? Maybe I'm wrong though what are your thoughts?

[ccrvlh]

I would say this is mostly personal preference.

I usually prefer to gradually adopt type annotations leaving the warnings while things are not 100%. There are a few reasons for that: a non-typed repo already has a bunch of warnings (with MyPy on), Any types makes potential errors/bugs silent, and while adopting type annotations some architectural/behavior decisions change (initializing a parameter on the instance vs the class, using property decorators to access more critical attributes and etc), so I tend to prefer to have MyPy warn about potential problems. In the future, if we are working on a branch with MyPy on, it will be inevitable to fix a couple of things here and there (and this would slowly make the codebase reach 100% of annotations), and if we were to ignore MyPy (same effect as using Any), then we might as well turn MyPy off anyways.

Again, I think this is purely personal preferences, and to have Any everywhere and then slowing adding the correct annotation is completely valid as well. I'll be happy to work on this either way!

[Fizzadar]

In the future, if we are working on a branch with MyPy on, it will be inevitable to fix a couple of things here and there (and this would slowly make the codebase reach 100% of annotations), and if we were to ignore MyPy (same effect as using Any), then we might as well turn MyPy off anyways.

Totally agree @lowercase00, let's not go down the Any route! I wonder what's best here for CI - would like to merge this branch in ASAP, maybe we just have mypy generate warnings and pass CI even though it has issues, which gives visibility without blocking any ongoing development.

[ccrvlh]

@Fizzadar I've been thinking about this for a while, here are the options I could think of:

1. Let things as is with MyPy included in the CI Ignore MyPy warnings and leave MyPy on the CI without breaking anything. This wouldn't allow us for a clean CI with MyPy, neither to implement a "no warnings" policy, but would already make MyPy a part of the application lifecycle. There's always a risk to "normalize" issues on the CI, which can be mitigated by actually dealing with types asap.

2. Let things as is and remove MyPy from CI Keep MyPy warnings and remove MyPy from the CI (for now). This would help while developing, wouldn't hurt CI, but also wouldn't make the codebase "typed" and would be harder to enforce typing for new contributions.

3. Adopt Any: This would allow us to have a "no warnings" codebase, add MyPy to the CI flow, and enjoy the benefits of the types we already have. The downside is that it would shadow potential issues, so we would have to actively/manually find places where Any is being used and slowly change it since we would have no warnings.

4. Remove types When looking at incremental adoption of MyPy type annotations, one of the recommendations (MyPy docs) is to add annotations incrementally, making sure everything everything passes (no warnings) with each new added type. The upside is that it would be explicit: things are typed and predictable where we have types, and we still need to work where there aren't any types.

I'm not sure I would go path 4, especially since State/Host/Inventory is used pretty much everywhere, so I'm not sure how feasible it would be.

About 3, thinking about this as I write, I don't have a good experience using Any everywhere. What I've found when I was using it a lot is that it was tricky: I thought I had MyPy as a safety net to warn me about issues, but at the end of the day there were so much Any around the codebase that a whole bunch of issues were just being ignored, so I had a false sense of security. The same goes for # type: ignore. In my last projects I've been pretty strict about typing, and even going the extra mile to make sure things pass (like using if xyz is None: raise even when we know that wouldn't happen), so I'm probably more biased to keep warnings.

Between path 1 and 2, i have to say I'm not totally sold on either. To be honest I've never given much though to MyPy in the CI flow until you mentioned, but i do agree there's value to it. I'm also not a huge fan of having a whole lot of warnings on the CI, but weighting the pros and cons, I think I would rather to have MyPy already on the CI then to push it any further. Wondering if anybody else have experience adopting typing on larger codebases (I don't).

Again, your call, I'll be happy to help regardless of the path taken!

[Fizzadar]

Leaning towards 1 I think. Agree that none of the options are ideal, so suggest we go with 1 and then iterate on fixing up the warnings ASAP.

At least having the warnings, even if not blocking CI, will make it possible to (manually) identify changes that increase the warning / issue count.

[ccrvlh]

@Fizzadar let me know if you want more changes in here before the final review

[simon04]

Hi, based on this PR, I tried to add some types to get_facts – https://github.com/simon04/pyinfra/commit/dd02a7ab62494b38002b90fe4924f9f98208c633

The idea is that the following example snippet can infer the type of linux_distribution and the existence of the key "release_meta"

https://github.com/Fizzadar/pyinfra/blob/2d4792eba3233c77a298d86e9638434688e3a1c5/examples/dnf.py#L18-L19

[codecov[bot]]

Codecov Report

Merging #881 (e0b0203) into 2.x (c4ede08) will decrease coverage by 0.38%. The diff coverage is 87.04%.

@@            Coverage Diff             @@
##              2.x     #881      +/-   ##
==========================================
- Coverage   92.03%   91.65%   -0.39%     
==========================================
  Files         125      125              
  Lines        7911     8025     +114     
==========================================
+ Hits         7281     7355      +74     
- Misses        630      670      +40     

Impacted Files	Coverage Δ
pyinfra/facts/util/distro.py	84.61% <0.00%> (ø)
pyinfra/context.py	92.20% <63.63%> (-4.98%)	:arrow_down:
pyinfra/connectors/chroot.py	94.87% <66.66%> (-3.76%)	:arrow_down:
pyinfra/connectors/docker.py	90.90% <70.00%> (-2.43%)	:arrow_down:
pyinfra/connectors/local.py	87.17% <70.00%> (-3.24%)	:arrow_down:
pyinfra/connectors/dockerssh.py	96.15% <71.42%> (-1.85%)	:arrow_down:
pyinfra/api/operations.py	80.48% <73.33%> (-1.24%)	:arrow_down:
pyinfra/connectors/ansible.py	93.81% <77.77%> (-0.87%)	:arrow_down:
pyinfra/api/connect.py	95.45% <80.00%> (-4.55%)	:arrow_down:
pyinfra/api/operation.py	90.62% <80.00%> (-0.39%)	:arrow_down:
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Fizzadar]

Found a bit of time to sink into this! Have now fixed all the mypy errors, I attempted to add types where possible. There are a few ignore statements in there, some are validish (setting attribute on a function kind of thing), others indicate areas where the code should be rewritten such that there's no reliance on conflating types together.

There's also a bunch of horrible looking nested/union types which would also make excellent candidates for refactoring. Now this is passing it can be merged, and subsequent PRs can iterate on types to expand the coverage. Running mypy in strict mode demonstrates the effort required here (😅): Found 1437 errors in 109 files (checked 120 source files).

Huge THANK YOU @lowercase00 for working on this and iterating through the issues faced, I think this is in a great spot now and am thrilled to be adding type checking to the CI workflow 🎉

[Fizzadar]

@simon04 that change looks fantastic, I'm about to merge this in if you wouldn't mind PR'ing that against 2.x that would be amazing!

[ccrvlh]

Awesome stuff! Happy to help!

MyPy strict is indeed challenging, it would probably require a bit if tweaking with architecture and/or generics to get things right, but I do think is definitely something worth pursuing slowly and gradually.

[Fizzadar]

100% agree, definitely a challenge worth persuing!