Enable professional-grade mypy #129

glatterf42 · 2024-11-12T11:53:52Z

Finally, here it is!
After reading this blog post about "professional-grade" mypy, I began adopting the professional-grade config for ixmp4. This PR is the result of the process.
It type hints everything, though some type hints might still need relaxation or tightening. Even our test suite is type-hinted here. This allows us to trust all variables we're dealing with have the correct type. In particular, refactoring can be done with improved confidence as mypy would catch a wide variety of bugs if the refactoring doesn't fit the existing code. Having these type hints also helps with readability, I think, as it is one kind of documentation in itself. I found this process especially insightful for understanding how the filters work :)
Of course, this PR also bumps mypy to its latest version.

That being said, there are a few points where I think we can still improve the PR, or add improvements through a future PR:

For the create() functions in the DB layer, I often kept the *args and **kwargs since the facade layer often uses positional arguments, while the rest/api layer uses keywords. This is different from tabulate() and list(), which are generally only called via enumerate() and with kwargs, so this would be a place to streamline the signatures.
In data/abstract/meta, I needed to type hint the _type_map, for which I needed to use the built-in type. This led to confusion for mypy since meta already had an attribute called type. I have renamed the attribute to dtype to enable the type hint.
For most kwargs, I have learned that the best way to type hint them is by creating a TypedDict subclass, which explicitly spells out all possible kwargs and their types. I have not done so for some /base/ functions, which mainly act as templates or pass on all *args, **kwargs that they receive. When args or kwargs were unused, I sometimes removed them from the signature and type-hinted them as Any at other times, depending on how I understood the function's purpose. So there are currently two approached visible, which come down to different understandings of type hints, I suppose:
On the one hand, one could try to spell out everything explicitly how it is currently used. This improves the documentation aspect and enables mypy to catch more bugs before they reach production. On the other hand, this would block newly introduced kwargs until they are added to these kwargs-classes, which increases maintenance burden. Alternatively, one could convey the purpose of the function through the type hints, being more general. The pros and cons are essentially the inverse of the above.
For code that I didn't write, it's harder to correctly guess the purpose of functions, so I have generally opted more for the first approach. However, I'm open to discussion here.
Where I did write kwargs-classes, they are sometimes not organized as well as they could, I feel. In the DB layer, for example, EnumerateKwargs could often inherit from CreateKwargs at the moment. For the filter classes, spelling out everything like name__like is also lengthy, we could e.g. think of using mixins there.
For some /base/ functions that pass on their arguments, I used Any because I couldn't get the alternative to work: in theory, this sounds like a job for ParamSpec: "They are used to forward the parameter types of one callable to another callable". However, I couldn't figure out exactly how to do that. On that note, if you know a good tutorial on ParamSpec, I'd be happy to read it as this almost seems like magic to me right now.

Lastly, I applied some type: ignore markers where I couldn't quite understand why mypy was complaining or where I intend to refactor the code very soon, anyway. In principle, though, almost none of that should be necessary, strictly-speaking. "Code I want to change soon" mainly refers to the DB model for parameters, equations, and variables, where I expect a more performant model can make do without the whole column thing. And I would maybe also drop table entirely since this is used nowhere in the tutorials. This is less certain, however, which is why I did type hint table, still.

Note

As so nicely displayed by codecov, we don't have 100% test coverage yet (that's another juicy small project goal...). So when reviewing this, please use the branch like main might be used to ensure that things we don't test did not get broken. In particular, this probably means running some CLI commands from this branch.

codecov · 2024-11-12T12:19:06Z

Codecov Report

Attention: Patch coverage is 95.10651% with 85 lines in your changes missing coverage. Please review.

Project coverage is 86.9%. Comparing base (362d503) to head (3fb15b9).

Files with missing lines	Patch %	Lines
ixmp4/cli/platforms.py	0.0%	13 Missing ⚠️
ixmp4/data/generator.py	0.0%	9 Missing ⚠️
ixmp4/data/db/iamc/timeseries/repository.py	78.1%	7 Missing ⚠️
ixmp4/db/filters.py	83.7%	7 Missing ⚠️
ixmp4/conf/manager.py	73.3%	4 Missing ⚠️
ixmp4/conf/settings.py	84.0%	4 Missing ⚠️
ixmp4/data/db/timeseries.py	87.0%	4 Missing ⚠️
ixmp4/server/rest/deps.py	69.2%	4 Missing ⚠️
ixmp4/core/iamc/data.py	78.5%	3 Missing ⚠️
ixmp4/data/api/base.py	95.8%	3 Missing ⚠️
... and 19 more

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #129     +/-   ##
=======================================
+ Coverage   85.9%   86.9%   +1.0%     
=======================================
  Files        227     228      +1     
  Lines       7583    8141    +558     
=======================================
+ Hits        6514    7081    +567     
+ Misses      1069    1060      -9

Files with missing lines	Coverage Δ
ixmp4/conf/__init__.py	`100.0% <100.0%> (ø)`
ixmp4/conf/credentials.py	`100.0% <100.0%> (ø)`
ixmp4/conf/toml.py	`97.9% <100.0%> (ø)`
ixmp4/core/__init__.py	`100.0% <100.0%> (ø)`
ixmp4/core/base.py	`93.3% <100.0%> (ø)`
ixmp4/core/decorators.py	`100.0% <100.0%> (ø)`
ixmp4/core/exceptions.py	`95.4% <100.0%> (+<0.1%)`	⬆️
ixmp4/core/iamc/__init__.py	`100.0% <100.0%> (ø)`
ixmp4/core/iamc/variable.py	`84.3% <100.0%> (+0.8%)`	⬆️
ixmp4/core/meta.py	`100.0% <100.0%> (ø)`
... and 183 more

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

glatterf42 · 2024-11-12T12:38:37Z

If we adopt this PR, we should also present a mypy badge in our readme :)

glatterf42 · 2024-11-12T12:39:37Z

pyproject.toml

+plugins = ['numpy.typing.mypy_plugin', 'pandera.mypy', 'pydantic.mypy', 'sqlalchemy.ext.mypy.plugin']
+disallow_untyped_defs = true
+disallow_any_unimported = true
+# disallow_any_generics = true


Another note: while working on this PR, I stumbled upon the setting disallow_any_generics = true. It is not active as of now, as that would introduce a few hundred more errors, but it would be an even stricter rule we could employ (to follow the spirit of using the strictest possible settings for mypy). This rule would prevent us from returning dict or list somewhere: all such generics would need to be explicitly specified as list[int] or some such. Happy to hear your thoughts on whether we should do that.

If we are already at it and its a comparatively small amount of work I would tend to go ahead! "wenn schon denn schon"

Thanks for this suggestion, this PR enables now all strict mypy rules except for no_implicit_reexport, which we discussed in person and wanted to keep for another PR as it might change the API.

meksor

Hi, what a feat! Thanks a lot, this is very cool.
I couldnt find what you mentioned in point 5. It sounds like that should be possible, but I've seen ParamSpec for the first time in this PR and couldnt tell you exactly what to do..
I also left some comments inline.

ixmp4/data/abstract/iamc/datapoint.py

meksor · 2024-11-12T15:12:53Z

ixmp4/data/abstract/model.py

+    name__notlike: str
+    name__notilike: str
+    iamc: (
+        dict[


Can we not solve this with a more general, recursive type-hint?

For example:

FilterDict = dict[ str, int | str | Iterable[int] | Iterable[str] | "FilterDict" ]

ixmp4/data/api/meta.py

ixmp4/data/abstract/iamc/variable.py

ixmp4/data/abstract/iamc/timeseries.py

ixmp4/data/api/base.py

glatterf42 · 2024-11-20T13:34:45Z

Okay, the update is here :)
It should clear the tests and mypy now, which has almost become strict (only missing no_implicit_reexport, which mypy's docs describe as "not too hard to get passing, but return on investment is lower".

I still need to do some cleanup, I want to look into your suggestion of recursive hints (and maybe employ some TypeAliases) in particular, but right now, I'll have to take a step back and check if we can already support Python 3.13 :)
After I do the cleanup, I'll still raise any remaining questions explicitly.

glatterf42

This took longer than expected again, but here it is: calling out every single thing I had a question about or an issue with! I'll push one or two more clean-up commits with things I noticed while going through all files, but after that, I'm happy with this PR :)

glatterf42 · 2024-11-22T12:28:03Z

ixmp4/conf/manager.py

        if res.status_code != 200:
            raise ManagerApiError(f"[{str(res.status_code)}] {res.text}")
-        return res.json()
+        # TODO Can we really assume this type?
+        return cast(dict[str, Any], res.json())


This seems to work based on all test we have, but I'm not sure if this is actually true since res.json() is just annotated to return Any. I think we can leave this, but please let me knof if this should be tested in some way.

glatterf42 · 2024-11-22T12:33:34Z

ixmp4/core/exceptions.py

+# TODO can't find a similar open issues, should I open one?
+# For reference, this occurred after adding disallow_subclassing_any
+# NOTE mypy seems to say Exception has type Any, don't see how we could change that
+class RemoteExceptionMeta(ExcMeta):  # type: ignore[misc]


Mypy complained here about subclassing from Any, which I don't see how we could avoid if Exception really inherits from that. But in that case, I would have expected this to be fairly common and noted in some mypy issue, but I couldn't find any. Should I open one or do you know what's going on here?

glatterf42 · 2024-11-22T12:35:16Z

ixmp4/core/iamc/variable.py

-            return None
-        elif isinstance(variable, str):
+        # TODO this check seems kind of redundant...
+        if isinstance(variable, str):


This check seems kind of redundant since variable can only be a string, anyway. But I didn't know what to do about the error: would we instead want to raise some error if the variable name is unknown?

glatterf42 · 2024-11-22T12:36:19Z

ixmp4/core/model.py

-            return None
-        elif isinstance(model, str):
+        # TODO this check seems kind of redundant...
+        if isinstance(model, str):


Same as for iamc/variable: what should we do about this error message when removing the if-check?

glatterf42 · 2024-11-22T12:39:52Z

ixmp4/core/scenario.py

-            return None
-        elif isinstance(scenario, str):
+        # TODO this check seems kind of redundant...
+        if isinstance(scenario, str):


Another redundant check: what do we do with the error?

glatterf42 · 2024-11-22T13:30:27Z

ixmp4/data/db/optimization/variable/repository.py

@@ -84,6 +87,7 @@ def add(

    @guard("view")
    def get(self, run_id: int, name: str) -> Variable:
+        # TODO by avoiding super().select, don't we also miss out on filters and auth?


Something I noticed while working on this PR, likely warrants its own follow-up issue.

glatterf42 · 2024-11-22T13:43:52Z

ixmp4/db/filters.py

+                #     raise ProgrammingError(
+                #         "Field argument `lookups` must be `list` of `str`."
+                #     )
+                override_lookups = _ensure_str_list(jschema_lookups)


This solution isn't ideal, as mentioned in the TODO comment above, but it seems to never be called (at least by our test suite), so the performance impact should be negligible. Please raise better ideas, though :)

glatterf42 · 2024-11-22T14:02:07Z

tests/data/test_count.py



-def deepgetattr(obj, attr):
+# TODO How to type hint this to return Repos with .list() and .count()?
+def deepgetattr(obj: Backend, attr: str) -> Any:


This type hint is not great, but I don't know how to make it better.

glatterf42 · 2024-11-22T14:08:22Z

tests/test_benchmark_filters.py

+    platform: ixmp4.Platform,
+    # TODO: improve type hints for fixtures
+    profiled: Any,
+    benchmark: Any,


I haven't figured out how to properly annotate these fixtures. Do you know? If not, should we figure this out or accept them as they are now?

glatterf42 · 2024-11-22T14:16:58Z

ixmp4/data/api/base.py

+    # TODO The order is important here at the moment, in particular having int before
+    # float! This should likely not be the case, but using StrictInt and StrictFloat
+    # from pydantic only created even more errors.
+    data: (
+        list[
+            list[
+                bool
+                | datetime
+                | int
+                | float
+                | str
+                | dict[str, Any]
+                # TODO should be able to remove this once PR#122 is merged
+                | list[float | int | str]
+                | None
+            ]
+        ]
+        | None
+    )


This type hint is not great as explained in the comment. I think I really don't like pydantic using the order in type hints to coerce types, because that's something that I wouldn't expect. Here, it seems to be important that int come before float. Similar to meta, I tried using StrictInt etc, but this broke some other tests. Please let me know if you have a good idea for this.

* Complete type hints, even for tests * Bump mypy to latest release version

glatterf42 added the enhancement New feature or request label Nov 12, 2024

glatterf42 requested a review from meksor November 12, 2024 11:53

glatterf42 self-assigned this Nov 12, 2024

glatterf42 force-pushed the enh/professional-grade-mypy branch from 46dca74 to 9173823 Compare November 12, 2024 12:14

glatterf42 mentioned this pull request Nov 12, 2024

Clear up several mypy # type: ignores #123

Closed

glatterf42 linked an issue Nov 12, 2024 that may be closed by this pull request

Fix type hinting for optimization.indexset #111

Open

glatterf42 commented Nov 12, 2024

View reviewed changes

meksor reviewed Nov 14, 2024

View reviewed changes

glatterf42 force-pushed the enh/professional-grade-mypy branch from e44a0c9 to 72f9bb1 Compare November 22, 2024 11:03

glatterf42 commented Nov 22, 2024

View reviewed changes

glatterf42 added 15 commits November 22, 2024 15:23

Enable professional-grade mypy

002d147

* Complete type hints, even for tests * Bump mypy to latest release version

Bump mypy for pre-commit

dd2e494

Fix type hints for itertools.cycle()

dce451a

Enable mypy for docs

93c2d0c

Enable more strict mypy rules

5c05007

Address mypy error that only appears in CI

49d354f

Remove outdated mypy.overrides

8978247

Note reason for type: ignore for future reference

8ac1caa

Use cast instead of recreating dict

67d4cf3

DRY enumerate kwargs

3759b62

DRY more type hints using TypeAlias

fc1fe17

DRY even more type hints using TypeAlias

4eb6d32

Resolve conflict in poetry.lock

13753e5

Clean up various small things

a8ae436

Exclude if TYPE_CHECKING from coverage

3fb15b9

glatterf42 force-pushed the enh/professional-grade-mypy branch from 72f9bb1 to 3fb15b9 Compare November 22, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable professional-grade mypy #129

Enable professional-grade mypy #129

glatterf42 commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

glatterf42 commented Nov 12, 2024

glatterf42 Nov 12, 2024

meksor Nov 12, 2024

glatterf42 Nov 20, 2024

meksor left a comment

meksor Nov 12, 2024

glatterf42 commented Nov 20, 2024

glatterf42 left a comment

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

glatterf42 Nov 22, 2024

Enable professional-grade mypy #129

Are you sure you want to change the base?

Enable professional-grade mypy #129

Conversation

glatterf42 commented Nov 12, 2024 • edited Loading

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

glatterf42 commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meksor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glatterf42 commented Nov 20, 2024

glatterf42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glatterf42 commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading