TYP: setup basic type checking and fix existing type errors #3546

neutrinoceros · 2021-10-07T09:33:29Z

PR Summary

This is a work in progress in support to the ongoing effort to add more type hints to the code base.
Goals:

enable continuous type checking with mypy
fix any existing errors (type inconsistencies) with minimal changes
add optional yt dependencies to Mypy workflow env and repeat (this may be left to a follow up PR)

matthewturk

I didn't make it all the way through. I do notice there are a bunch of changes I'm not sure really fit, like some changes to the volume rendering camera, that I think I need to understand why they're bundled here.

matthewturk · 2021-10-08T11:05:12Z

pyproject.toml

@@ -76,3 +76,15 @@ addopts = '''
    --ignore='yt/frontends/owls_subfind/tests/test_outputs.py'
    --ignore='yt/frontends/ramses/tests/test_outputs.py'
 '''
+


I don't think I can necessarily review these, but they seem okay-ish.

matthewturk · 2021-10-08T11:05:43Z

yt/config.py

@@ -220,7 +220,7 @@ def _repr_json_(self):


 if not os.path.exists(_global_config_file):
-    cfg = {"yt": {}}
+    cfg = {"yt": {}}  # type: ignore


I have a sneaking suspicion type: ignore is going to get pretty frustrating...

matthewturk · 2021-10-08T11:06:14Z

yt/data_objects/data_containers.py

 import weakref
 from collections import defaultdict
 from contextlib import contextmanager
+from typing import List, Tuple, Union


When are we able to use list and the like with just a __future__ import?

When we drop Python 3.6 later this month. And it will come basically free thanks to pyupgrade + pre-commit

matthewturk · 2021-10-08T11:07:08Z

yt/data_objects/data_containers.py

@@ -469,9 +470,6 @@ def write_out(self, filename, fields=None, format="%0.16e"):
        if fields is None:
            fields = sorted(self.field_data.keys())

-        if self._key_fields is None:


Why are we getting rid of this? Does adding the type check create a default value?

no, it doesn't. So here's my reasoning: the YTDataContainer class was never meant for instantiation, but it's useful to inherit from, so it is essentially an abstract class. Many of the class attributes are defined there with placeholder values. Now, if the _key_fields attribute needs to be defined in subclasses, as the check I removed here seems to imply, then it doesn't make much sense to give it Optional[List[str]] as a type when really it needs to be List[str]. Does this make sense ?

matthewturk · 2021-10-08T11:07:56Z

yt/data_objects/index_subobjects/octree_subset.py

@@ -38,7 +39,7 @@ class OctreeSubset(YTSelectionContainer):
    _num_ghost_zones = 0
    _type_name = "octree_subset"
    _skip_add = True
-    _con_args = ("base_region", "domain", "ds")
+    _con_args: Tuple[str, ...] = ("base_region", "domain", "ds")


Am I reading it right that we need to re-type-hint things in subclasses?

For tuples of arbitrary length in classes that have children of their own, yes.
Mypy is pretty strict in type inference so it will consider that tuples of different lengths are incompatible types unless we explicitly mark the length as arbitrary in the parent class.

On the other hand, if these attributes were lists (mutable) instead of tuples (immutable), we could define their types as list[str] once and for all.

They aren't mutable, though.

which is why mypy is a little harder to satisfy, but I actually prefer keeping it the way it is now too

matthewturk · 2021-10-08T11:09:18Z

yt/fields/domain_context.py

 domain_context_registry = {}


-class DomainContext:
+class DomainContext(abc.ABC):


What benefit does making this an ABC without defining any required characteristics give us?

see my reply about YTDataContainer for my global reasoning. Here it doesn't seem necessary, but it also doesn't break any test so I don't think it hurts to explicitly declare that this class isn't meant to be instantiated. Should I revert it ?

matthewturk · 2021-10-08T11:11:52Z

yt/frontends/ramses/field_handlers.py

-    attrs = None  # The attributes of the header
+    ftype: Optional[str] = None  # The name to give to the field type
+    fname: Optional[str] = None  # The name of the file(s)
+    attrs: Optional[


I need to think carefully about this specific type.

matthewturk · 2021-10-08T11:12:53Z

yt/utilities/command_line.py

-            action="store_true",
-            help="Store the configuration in the global configuration file.",
-        ),
+_global_local_args = (


Where did "exclusive" go?

This is a mistake, thanks for catching this !

Reverted the "fix" now that I saw it breaking CI https://github.com/yt-project/yt/actions/runs/1320143467
I don't understand where this string was supposed to be used. If you know it is actually useful I may need to revert the change here and modify corresponding hints accordingly.

matthewturk · 2021-10-08T11:13:23Z

yt/utilities/io_handler.py

@@ -20,8 +21,8 @@ def _make_io_key(args, *_args, **kwargs):


 class BaseIOHandler:
-    _vector_fields = ()
-    _dataset_type = None
+    _vector_fields: Tuple[str, ...] = ()


Should this always be just a str, or could it contain a ftype, fname tuple?

Dunno. Started with the strictest type that I knew was possible and stopped there because I saw no further conflicts reported. It is absolutely fine to extend this definition later if needed.

neutrinoceros · 2021-10-08T11:17:38Z

You mean the properties ? I think the way it was implemented 7yrs was likely a hack to get Python2 and Python3 compat, and mypy doesn't like it so I simply modernized it.

neutrinoceros · 2021-10-09T08:18:15Z

Something happened in the last couple hours that apparently broke the py38 Jenkins job (2739 errors, not just this PR !).
edit: there's the issue to keep track of upstream: sympy/sympy#22241

So I think it's preferable to keep this as a draft for now, because I'd like to collapse my ~30 commits down to 2 or 3 commits via rebasing, but I don't feel comfortable doing it while CI is red, since I'll loose the granularity I need to bisect bugs easily.

neutrinoceros · 2021-10-11T16:47:41Z

@yt-fido test this please

neutrinoceros · 2021-10-12T08:14:33Z

yay, this is back to only typing failures (as opposed to red CI), should be able to rebase and open this soon

cphyc

One thing that's getting me worried is all the # type: ignore in the imports.
Could you elaborate why they are required?

yt/data_objects/static_output.py

yt/frontends/gadget/data_structures.py

yt/frontends/ramses/field_handlers.py

yt/frontends/rockstar/definitions.py

yt/frontends/ramses/particle_handlers.py

yt/frontends/ramses/field_handlers.py

yt/sample_data/api.py

yt/utilities/object_registries.py

yt/visualization/volume_rendering/camera.py

yt/visualization/volume_rendering/scene.py

neutrinoceros · 2021-10-12T10:13:21Z

One thing that's getting me worried is all the # type: ignore in the imports.
Could you elaborate why they are required?

sure. So the reason I'm adding a lot of such comments in places where yt.units is imported from is that mypy complains along the lines of

yt.units has no attribute "..."

and this is inherently because yt.units acts as a wrapper around unyt using from ... import *, which is something that mypy can't resolve easily. Now I think #2597 would have helped with this, but it only ever raised concerns and I was never able to convince anyone else to consider it for inclusion, hence # type: ignore 🤷🏻‍♂️

neutrinoceros · 2021-10-12T17:17:32Z

almost there. Now praying I didn't break anything in my rebasing. If all goes well I'll remove the draft status later today.

neutrinoceros · 2021-11-12T10:18:34Z

Noe to reviewers: I got this mergeable again, but needed to rebase to resolve some conflicts. Everything that's changed since the last time you reviewed is found in small, isolated commits, so I hope it's still manageable.
Also note that, as most sweeping changes, this has the potential of becoming unmanageable on my end very quickly, so I'd appreciate if we can consider moving this forward as soon as possible. Thanks

matthewturk · 2021-11-12T14:24:02Z

I think moving it forward is a good idea. And honestly, as long as there are no functional changes, I think reviewing the top-level restructurings is good enough, and the type annotations themselves do not necessarily require detailed review.

…

On Fri, Nov 12, 2021 at 4:18 AM Clément Robert ***@***.***> wrote: Noe to reviewers: I got this mergeable again, but needed to rebase to resolve some conflicts. Everything that's changed since the last time you reviewed is found in small, isolated commits, so I hope it's still manageable. Also note that, as most sweeping changes, this has the potential of becoming unmanageable on my end very quickly, so I'd appreciate if we can consider moving this forward as soon as possible. Thanks — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

neutrinoceros · 2021-11-12T14:28:50Z

There are some tiny functional changes though, almost exclusively in input validation routines, where I raised some type degeneracies detected by mypy. Other than that, there should be none

- add basic type checking workflow - bootstrap mypy configuration - sprinkle minimal type hints to satisfy mypy

…e image filename validation

for more information, see https://pre-commit.ci

neutrinoceros · 2021-11-20T07:46:28Z

forced-pushed again to resolve conflicts. Again, the bulk of the diff is contained in the unchanged (conflicts aside) first commit. I didn't need any new commit this time, phew

matthewturk · 2021-11-22T14:22:47Z

I personally think that this is good to go.

neutrinoceros · 2021-11-22T14:33:21Z

@cphyc, sorry I know you're busy, but could you give this the final review since you've already done most of it ?

BUG: hotfix for a functional breakage introduced in #3546

neutrinoceros force-pushed the mypy-setup branch 2 times, most recently from 3eecf38 to 5ef7902 Compare October 7, 2021 09:37

neutrinoceros mentioned this pull request Oct 7, 2021

ENH: Particle reader cleanup #3526

Merged

neutrinoceros added infrastructure Related to CI, versioning, websites, organizational issues, etc enhancement Making something better labels Oct 7, 2021

neutrinoceros force-pushed the mypy-setup branch 7 times, most recently from 76d7437 to b738ccc Compare October 8, 2021 09:30

matthewturk reviewed Oct 8, 2021

View reviewed changes

neutrinoceros force-pushed the mypy-setup branch 2 times, most recently from 0f5e3ab to 7c974c7 Compare October 8, 2021 15:49

neutrinoceros force-pushed the mypy-setup branch 2 times, most recently from 84b06a9 to 1a3baf6 Compare October 11, 2021 07:48

neutrinoceros force-pushed the mypy-setup branch from 80f745e to a0f51cf Compare October 11, 2021 20:50

neutrinoceros mentioned this pull request Oct 12, 2021

TYP: add a yt._typing module and start annotating yt.loaders #3064

Closed

cphyc reviewed Oct 12, 2021

View reviewed changes

neutrinoceros force-pushed the mypy-setup branch 2 times, most recently from 2e7eeb0 to 96d9135 Compare October 12, 2021 17:14

neutrinoceros marked this pull request as ready for review October 12, 2021 19:55

neutrinoceros closed this Oct 14, 2021

neutrinoceros marked this pull request as ready for review November 11, 2021 21:50

neutrinoceros and others added 14 commits November 20, 2021 08:44

TYP:

06b791e

- add basic type checking workflow - bootstrap mypy configuration - sprinkle minimal type hints to satisfy mypy

TYP: fix a broken type expression

b81c427

TYP: add a missing type check

f2bdae0

TYP: fix a broken type check

6127f75

TYP: cleanup a seemingly unused and untyped class attr

3b21c89

TYP: fix a broken type expression

4a4a4e5

TYP: fix, keep a var type consistent

217558c

TYP: fix an unsafe type check block

280a703

TYP: add a missing type hint

fade176

TYP: add a missing type hint (fme)

c9dff82

TYP: fix broken type hints

55f116a

TYP: fix a broken return type

fa94707

BUG: fix a bug introduced by an earlier type-related commit that brok…

319e0ed

…e image filename validation

[pre-commit.ci] auto fixes from pre-commit.com hooks

e53e681

for more information, see https://pre-commit.ci

neutrinoceros force-pushed the mypy-setup branch from 4e29b49 to e53e681 Compare November 20, 2021 07:45

matthewturk approved these changes Nov 22, 2021

View reviewed changes

cphyc approved these changes Nov 22, 2021

View reviewed changes

cphyc merged commit f137e1d into yt-project:main Nov 22, 2021

neutrinoceros deleted the mypy-setup branch November 22, 2021 14:45

neutrinoceros added a commit to neutrinoceros/yt that referenced this pull request Nov 22, 2021

BUG: hotfix for a functional breakage introduced in yt-project#3546

206499f

neutrinoceros mentioned this pull request Nov 22, 2021

BUG: hotfix for a functional breakage introduced in #3546 #3692

Merged

matthewturk added a commit that referenced this pull request Nov 22, 2021

Merge pull request #3692 from neutrinoceros/hotfix_callback_breakage

d4d833b

BUG: hotfix for a functional breakage introduced in #3546

neutrinoceros mentioned this pull request Jan 19, 2022

Type annotation #2864

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TYP: setup basic type checking and fix existing type errors #3546

TYP: setup basic type checking and fix existing type errors #3546

neutrinoceros commented Oct 7, 2021 •

edited

Loading

matthewturk left a comment

matthewturk Oct 8, 2021

matthewturk Oct 8, 2021

matthewturk Oct 8, 2021

neutrinoceros Oct 8, 2021

matthewturk Oct 8, 2021

neutrinoceros Oct 8, 2021 •

edited

Loading

matthewturk Oct 8, 2021

neutrinoceros Oct 8, 2021

neutrinoceros Oct 26, 2021

matthewturk Oct 26, 2021

neutrinoceros Oct 26, 2021

matthewturk Oct 8, 2021

neutrinoceros Oct 8, 2021

matthewturk Oct 8, 2021

matthewturk Oct 8, 2021

neutrinoceros Oct 8, 2021

neutrinoceros Oct 8, 2021

neutrinoceros Oct 8, 2021

matthewturk Oct 8, 2021

neutrinoceros Oct 8, 2021

neutrinoceros commented Oct 8, 2021

neutrinoceros commented Oct 9, 2021 •

edited

Loading

neutrinoceros commented Oct 11, 2021

neutrinoceros commented Oct 12, 2021

cphyc left a comment

neutrinoceros commented Oct 12, 2021

neutrinoceros commented Oct 12, 2021

neutrinoceros commented Nov 12, 2021

matthewturk commented Nov 12, 2021 via email

neutrinoceros commented Nov 12, 2021

neutrinoceros commented Nov 20, 2021

matthewturk commented Nov 22, 2021

neutrinoceros commented Nov 22, 2021

TYP: setup basic type checking and fix existing type errors #3546

TYP: setup basic type checking and fix existing type errors #3546

Conversation

neutrinoceros commented Oct 7, 2021 • edited Loading

PR Summary

matthewturk 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

neutrinoceros Oct 8, 2021 • edited Loading

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

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

neutrinoceros commented Oct 8, 2021

neutrinoceros commented Oct 9, 2021 • edited Loading

neutrinoceros commented Oct 11, 2021

neutrinoceros commented Oct 12, 2021

cphyc left a comment

Choose a reason for hiding this comment

neutrinoceros commented Oct 12, 2021

neutrinoceros commented Oct 12, 2021

neutrinoceros commented Nov 12, 2021

matthewturk commented Nov 12, 2021 via email

neutrinoceros commented Nov 12, 2021

neutrinoceros commented Nov 20, 2021

matthewturk commented Nov 22, 2021

neutrinoceros commented Nov 22, 2021

neutrinoceros commented Oct 7, 2021 •

edited

Loading

neutrinoceros Oct 8, 2021 •

edited

Loading

neutrinoceros commented Oct 9, 2021 •

edited

Loading