Turn on static type checks in the CI (and have them pass)

[andreww]

A reasonable amount to this, but it only (a) enabled type checking in our CI workflow and (b) makes this pass. It's a rebase of a previous PR with commits reorganised.

With the changes to get_runtime_config's return types, I think I agree with @tlestang that this is one of those places where type checking isn't really worth it. But if we don't do anything about it we cannot really do type checking elsewhere. I think (as I say in fb01f50dcc327b7c830b4cf07142dff16a76cc2d) that fact that we check args.footprint in two places isn't ideal (what if we rename the argument) but I wouldn't worry too much about that unless we actually want to further refactor. Anyway, I kept all of that in a single commit.

The other changes are (I think) pretty straightforward. I've incorporated the changes suggested by @abhidg in the previous PR.

[tlestang]

Thanks for the work. The benefit of enforcing 100% compliance with mypy's static type checks is not clear to me. I feel this way for most projects big and small, but this becomes especially plain for a small simple projects like cats. Enforcing 100% will inevitably lead to code that aims at satisfying type checks at the expense of Python's expressivity/understandability.

To me, this is demonstrated by the fact you felt the need to add a lengthy comment justifying why configure.get_runtime_config does not set PUEand jobinfo to None. I think None have different meaning than 0.0 and that's important. You also mention similar issues in your commit messages:

This is arguably one of those places where static type checking and python do not play particularly well together, but this does allow mypy to report all is well.

While None being passed into sys.exit isn't an error in python, it does break static type checking.

While this isn't an error in python, it does break static type checking.

Don't get me wrong: i'm not against type hints, they can really help navigate the code. I'm not against static type checking either, it can help identify issues not caught by tests, and it did in you case. But I do have a problem with (blindly?) enforcing 100% compliance with static type checks, for... what reason(s)?

[andreww]

Thanks - I don't disagree about the balance between dynamic typing and including types, but would ask two questions. What is the point of annotating the types if we don't use that information? Is it sensible to include incorrect type annotations?

I would argue that if we are adding typing information to functions it really ought to be correct. Running the code through mypy in the CI helps us make sure that the typing is correct, and adds some value to including the typing information (because we finds bugs that are not caught by our existing test suite). Otherwise we should probably strip out the type annotations.

Which isn't to say that the way I've made the static type checking pass is the best or only approach. Having said that, some context:

While None being passed into sys.exit isn't an error in python, it does break static type checking.

We were using the returned value from a function without a return statement and feeding that into sys.exit. This is not an error in python but I'm not sure it's a sensible thing to do.

While this isn't an error in python, it does break static type checking.

The variable name output was referring to to two different objects in the same place in the code. While this isn't an error in python, I think that it is potentially confusing.

For configure.get_runtime_config, I think we can probably make this play nicely by making PUE and jobinfo optional and putting the a type narrowing check in place prior to using them (which is probably good practice anyway).

[abhidg]

We can have the best of both worlds - have mypy run but use # type: ignore annotations where we want be looser, or use typing.Any, or as @andreww suggested not use type annotations for a particular function. mypy can also ignore entire files.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 89.22%. Comparing base (f7ad774) to head (e50379d).

Files	Patch %	Lines
cats/__init__.py	40.00%	3 Missing :warning:
cats/configure.py	80.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #84 +/- ## ========================================== - Coverage 89.71% 89.22% -0.49% ========================================== Files 14 14 Lines 525 529 +4 ========================================== + Hits 471 472 +1 - Misses 54 57 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[andreww]

I rebased this and found (what I think is) a better approach for the types of get_runtime_config. I'll add tests to this PR to make codeconv happy.