-
Notifications
You must be signed in to change notification settings - Fork 15
Replace mypy with ty for static type checking #585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
6fe73a0
9d19581
60dd366
1136c2d
ada8d42
0e40212
789f1c6
00c05e9
3fd253a
db6e523
6792f50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| **/*.pyo | ||
| **/*.pyd | ||
| **/*.pkl | ||
| .ty | ||
| .mypy_cache | ||
| _autosummary | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ dist/ | |
| *.egg-info/ | ||
| miniconda/ | ||
| .mypy_cache/ | ||
| .ty/ | ||
| tools/ | ||
| .metals/ | ||
| .bloop/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1268,7 +1268,7 @@ def _label_pfn(source_node_ids, _): | |
| del label_edge_index | ||
|
|
||
| if is_positive: | ||
| # This assert is added to pass mypy type check, in practice we will not see this fail | ||
| # This assert is added to pass the type checker, in practice we will not see this fail | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we check that (I am going to leave identical comments on all of the changes like this - that way the robots can pick them up and check automatically)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needed |
||
| assert ( | ||
| self._positive_label_edge_index is not None | ||
| ), "Must register positive labels prior to partitioning them" | ||
|
|
@@ -1277,7 +1277,7 @@ def _label_pfn(source_node_ids, _): | |
| if len(self._positive_label_edge_index) == 0: | ||
| self._positive_label_edge_index = None | ||
| else: | ||
| # This assert is added to pass mypy type check, in practice we will not see this fail | ||
| # This assert is added to pass the type checker, in practice we will not see this fail | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we check that (I am going to leave identical comments on all of the changes like this - that way the robots can pick them up and check automatically)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needed |
||
| assert ( | ||
| self._negative_label_edge_index is not None | ||
| ), "Must register negative labels prior to partitioning them" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -268,7 +268,7 @@ def _build_job_config( | |
| if vertex_ai_resource_config.timeout | ||
| else None, | ||
| # This should be `aiplatform.gapic.Scheduling.Strategy[inferencer_resource_config.scheduling_strategy]` | ||
| # But mypy complains otherwise... | ||
| # But the type checker complains otherwise... | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we check that (I am going to leave identical comments on all of the changes like this - that way the robots can pick them up and check automatically)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needed |
||
| # gigl/src/inference/v2/glt_inferencer.py:124: error: The type "type[Strategy]" is not generic and not indexable [misc] | ||
| # TODO(kmonte): Fix this | ||
| scheduling_strategy=getattr( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -44,7 +44,7 @@ def _get_bigquery_ptransform( | |||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| # Below type ignores are due to mypy star expansion issues: https://github.com/python/mypy/issues/6799 | ||||||||||||||||
| # Below type ignores are due to star expansion issues with the type checker: https://github.com/python/mypy/issues/6799 | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we check that (I am going to leave identical comments on all of the changes like this - that way the robots can pick them up and check automatically)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed - nice
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait did we keep the link as is?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This along with others are fixed in the follow up PR where I address the types in one go. Fix here (comment is removed as ignore not needed): GiGL/gigl/src/data_preprocessor/lib/ingest/bigquery.py Lines 69 to 75 in af3dc20
|
||||||||||||||||
| @dataclass(frozen=True) | ||||||||||||||||
| class BigqueryNodeDataReference(NodeDataReference): | ||||||||||||||||
| """ | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |||||||||||||
| from gigl.src.common.types.graph_data import EdgeType, EdgeUsageType, NodeType | ||||||||||||||
| from gigl.src.data_preprocessor.lib.types import InstanceDictPTransform | ||||||||||||||
|
|
||||||||||||||
| # Type hints for abstract dataclasses are currently not supported. https://github.com/python/mypy/issues/5374 | ||||||||||||||
| # Type hints for abstract dataclasses may have limited support in type checkers. https://github.com/python/mypy/issues/5374 | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we check that (I am going to leave identical comments on all of the changes like this - that way the robots can pick them up and check automatically) Also update the TODO from python/mypy?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was fixed with ty - updated
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto on link.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was. removed in follow up: GiGL/gigl/src/data_preprocessor/lib/ingest/reference.py Lines 9 to 14 in af3dc20
|
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| @dataclass(frozen=True) # type: ignore | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -387,8 +387,8 @@ def select_label_edge_types( | |||||||||||||||||||||||||
| str, | ||||||||||||||||||||||||||
| int, | ||||||||||||||||||||||||||
| # TODO(kmonte): Add GLT Partition book here | ||||||||||||||||||||||||||
| # We cannot at the moment as we mypy ignore GLT | ||||||||||||||||||||||||||
| # And adding it as a type here will break mypy. | ||||||||||||||||||||||||||
| # We cannot at the moment as we type-ignore GLT | ||||||||||||||||||||||||||
| # And adding it as a type here will break the type checker. | ||||||||||||||||||||||||||
| # PartitionBook | ||||||||||||||||||||||||||
|
Comment on lines
+390
to
392
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we check that (I am going to leave identical comments on all of the changes like this - that way the robots can pick them up and check automatically)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed - I added: Lines 383 to 394 in af3dc20
|
||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,9 +142,8 @@ lint = [ | |
| "isort~=5.12.0", | ||
| "mdformat==0.7.22", | ||
| "mdformat_tables==1.0.0", | ||
| "mypy==1.8.0", | ||
| "mypy-extensions", | ||
| "mypy-protobuf==3.3.0", | ||
| "ty~=0.0.29", | ||
| "mypy-protobuf==3.3.0", # Used for protobuf stub generation (protoc-gen-mypy), not type checking | ||
| ] | ||
|
|
||
|
|
||
|
|
@@ -283,3 +282,84 @@ exclude = [ | |
| remove-all-unused-imports = true | ||
| in-place = true | ||
| recursive = true | ||
|
|
||
| [tool.ty.environment] | ||
| python-version = "3.11" | ||
|
|
||
| [tool.ty.src] | ||
| exclude = ["*_pb2.py", "*_pb2.pyi", "**/*.ipynb"] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we add a TODO to include our notebooks here eventually?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tbh, I think its fine if we don't Most act as documentation |
||
|
|
||
| [tool.ty.analysis] | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Migrated from mypy.ini - removed any that could be fixed - thank you robots! |
||
| # Migrated from mypy.ini per-module ignore_missing_imports sections, | ||
| # plus additional modules that are optional or not installed in all environments. | ||
| allowed-unresolved-imports = [ | ||
| "absl.**", | ||
| "apache_beam.**", | ||
| "common.**", | ||
| "fastavro.**", | ||
| "google.cloud.**", | ||
| "google_cloud_pipeline_components.**", | ||
| "graphlearn_torch.**", | ||
| "hydra.**", | ||
| "kfp.**", | ||
| "kfp_server_api.**", | ||
| "matplotlib.**", | ||
| "msgpack", | ||
| "networkx.**", | ||
| "ogb.**", | ||
| "orjson", | ||
| "parameterized.**", | ||
| "pyarrow.**", | ||
| "setuptools", | ||
| "tensorflow.**", | ||
| "tensorflow_data_validation.**", | ||
| "tensorflow_metadata.**", | ||
| "tensorflow_transform.**", | ||
| "tfx_bsl.**", | ||
| # torch lacks inline types/stubs; ty cannot resolve torch types in all environments | ||
| "torch.**", | ||
| "torch_geometric.**", | ||
| "torch_sparse.**", | ||
| "torchrec.**", | ||
| ] | ||
| # When mypy's ignore_missing_imports=True was set for a module, mypy would also | ||
| # implicitly treat all types from that module as Any, suppressing downstream | ||
| # type errors (e.g. unresolved-attribute, invalid-argument-type) on values from | ||
| # those modules. ty's allowed-unresolved-imports only suppresses the import | ||
| # error but leaves the type as Unknown, causing cascading errors. This setting | ||
| # restores the mypy behavior by replacing imports from untyped libraries with Any. | ||
| replace-imports-with-any = [ | ||
| "apache_beam.**", | ||
| "fastavro.**", | ||
| "google.cloud.**", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. likewise, is there a reason we're blanket ignoring all gcp imports?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| "google_cloud_pipeline_components.**", | ||
| "graphlearn_torch.**", | ||
| "hydra.**", | ||
| "kfp.**", | ||
| "kfp_server_api.**", | ||
| "matplotlib.**", | ||
| "ogb.**", | ||
| "tensorflow.**", | ||
| "tensorflow_data_validation.**", | ||
| "tensorflow_metadata.**", | ||
| "tensorflow_transform.**", | ||
| "tfx_bsl.**", | ||
| "torch.**", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmmm, we weren't type-ignoring torch before. IIRC I saw some issues with torch when I looked into this earlier, the ty people suggested to enable some flag - did you try this? astral-sh/ty#2244 Since we're so reliant on torch I'd rather not type-ignore it if possible.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unforunately the flag is not the problem From the robots analysis, there are 200+ issues that were not being caught before; I think we should reserve for a future change?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd prefer to keep the torch typing around (if my reading is correct that this infact is equivalent to type-ignoring torch) and just add a bunch of IDK how feasible that'd be to setup tho.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ye for sure. its too wide of a scope change for 1 pr.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I gave the robots a go at it and the generated https://github.com/Snapchat/GiGL/pull/597/changes - WDYT about merging it into your follow up PR? I do feel pretty strongly about not type-ignoring torch, even if temporarily. |
||
| "torch_geometric.**", | ||
| "torch_sparse.**", | ||
| "torchrec.**", | ||
| ] | ||
|
|
||
| [tool.ty.rules] | ||
| # Suppress noise from existing type: ignore comments that ty may not need. | ||
| unused-type-ignore-comment = "ignore" | ||
| unused-ignore-comment = "ignore" | ||
|
|
||
| # Generated protobuf files trigger type errors from the google-protobuf stubs | ||
| # (e.g. RegisterMessage argument types). These files are auto-generated by | ||
| # protoc and cannot be fixed — suppress all type errors in them. | ||
| [[tool.ty.overrides]] | ||
| include = ["**/*_pb2.py"] | ||
| [tool.ty.overrides.rules] | ||
| invalid-argument-type = "ignore" | ||
| unresolved-attribute = "ignore" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we check that
tystill complains?(I am going to leave identical comments on all of the changes like this - that way the robots can pick them up and check automatically)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I threw bot at it, looks like we still might need these.