Reduces the number of ignored mypy errors #4495

xadupre · 2022-09-08T09:56:23Z

Description

Reduces the number of mypy errors. It works with the latest version of mypy. It adds the flag --ignore-missing-package to avoid adding type: ignore every time import numpy is used. The PR removes unnecessary # type: ignore every time an external package is imported.

Motivation and Context

tools/style.sh generates many errors not related to the changes. This PR make it easier to read what is related to the user changes.

jcwchen

I believe in the current codebase a lot of code cannot pass mypy 0.971 check. Currently two CIs are still running two different versions of mypy, which is quite confusing:

mypy 0.782 in Linux-CI
mypy 0.971 in Python Lint CI

It seems there is no way to specify mypy version from the latest action-mypy: tsuyoshicho/action-mypy#48. We probably can use an older version of tsuyoshicho/action-mypy, v2.0.1, which is using 0.790. If there are still a lot of existing errors with 0.790, I would suggest either we keep one version of mypy or upgrade mypy to the latest one (0.971). Upgrade mypy to the latest one will need a lot of fixes and perhaps the best we can do for now is only keeping version 0.782. @@justinchuby What's your thoughts on this? Thanks!

justinchuby · 2022-09-08T15:51:28Z

onnx/__init__.py

@@ -77,7 +77,7 @@ def _serialize(proto: Union[bytes, google.protobuf.message.Message]) -> bytes:
                    "Please use save_as_external_data to save tensors separately from the model file."
                ) from e
            raise
-        return result
+        return result  # type: ignore


We should avoid ignores without error nums (too board)

mypy returns Returning Any from function declared to return "bytes" in this case. I assume SerializeToString() is annotated as Any when this function returns bytes. mypy detects the inconsistency. I don't think we can easliy remove it. I don't know if onnx is the source of the wrong annotation or if it is protobuf.

justinchuby · 2022-09-08T15:52:21Z

action-mypy uses the local mypy if found. We will simply need to install mypy at the version of our choice before running the check.

jcwchen

The part for adding ignore-missing-imports makes sense to me. Thank you for updating so many places!

IMO we should be careful about adding more ignore conditions because it's hard to resolve every of them in the future when feasible. I saw a few places were added # type: ignore so I would like to understand more details and whether we can resolve them properly.

workflow_scripts/test_model_zoo.py

onnx/test/test_external_data.py

onnx/__init__.py

onnx/backend/test/runner/__init__.py

jcwchen · 2022-09-12T16:00:39Z

onnx/backend/test/runner/__init__.py

@@ -178,7 +178,7 @@ def tests(self) -> Type[unittest.TestCase]:
            onnx_backend_tests = BackendTest(backend).tests
        """
        tests = self._get_test_case("OnnxBackendTest")
-        for items_map in sorted(self._filtered_test_items.values()):
+        for items_map in sorted(self._filtered_test_items.values()):  # type: ignore


May I understand which kind of error does it ignore? mypy 0.971 produces: error: Value of type variable "SupportsRichComparisonT" of "sorted" cannot be "Dict[str, TestItem]" [type-var]

jcwchen · 2022-09-12T16:01:01Z

onnx/test/shape_inference_test.py

@@ -100,7 +100,7 @@ def _assert_inferred(
        inferred_model = self._inferred(graph, **kwargs)
        inferred_vis = list(inferred_model.graph.value_info)
        vis = list(sorted(vis, key=lambda x: x.name))
-        inferred_vis = list(sorted(inferred_vis, key=lambda x: x.name))
+        inferred_vis = list(sorted(inferred_vis, key=lambda x: x.name))  # type: ignore


May I understand which kind of error does it ignore? mypy 0.971 produces: error: Returning Any from function declared to return "Union[SupportsDunderLT, SupportsDunderGT]" [no-any-return]

I would still suggest adding the error code in square brackets to narrow the ignore for clarity.

From what I understood, you can ignore this error by forcing the type: type: expected type. But mypy checks the expected type exists and for that it must have been imported. But if I do that, it would raise the warning unused import. So I think # type: ignore is not bad here.

onnx/test/symbolic_shape_test.py

tools/style.sh

* remove unnecessary import Signed-off-by: xadupre <[email protected]> * lint Signed-off-by: xadupre <[email protected]> * black Signed-off-by: xadupre <[email protected]> * black Signed-off-by: xadupre <[email protected]> * lint Signed-off-by: xadupre <[email protected]> * restore old import Signed-off-by: xadupre <[email protected]> Signed-off-by: xadupre <[email protected]>

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

* primary ops to function milestone 1 Signed-off-by: Liqun Fu <[email protected]> * float type Signed-off-by: Liqun Fu <[email protected]> * formatting Signed-off-by: Liqun Fu <[email protected]> * pass backend test Signed-off-by: Liqun Fu <[email protected]> * formatting Signed-off-by: Liqun Fu <[email protected]> * commit Signed-off-by: Liqun Fu <[email protected]> * format Signed-off-by: Liqun Fu <[email protected]> * format Signed-off-by: Liqun Fu <[email protected]> * layernorm Signed-off-by: Liqun Fu <[email protected]> * pass runtime check Signed-off-by: Liqun Fu <[email protected]> * format Signed-off-by: Liqun Fu <[email protected]> * pure function Signed-off-by: Liqun Fu <[email protected]> * format Signed-off-by: Liqun Fu <[email protected]> * reviewer's comments, clean up some Signed-off-by: Liqun Fu <[email protected]> * keep op original version Signed-off-by: Liqun Fu <[email protected]> * fix gtest.function_verify_test Signed-off-by: Liqun Fu <[email protected]> * formatting Signed-off-by: Liqun Fu <[email protected]> * update according to reviewer's comment Signed-off-by: Liqun Fu <[email protected]> Signed-off-by: Liqun Fu <[email protected]> Co-authored-by: G. Ramalingam <[email protected]> Signed-off-by: xadupre <[email protected]>

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

Signed-off-by: xadupre <[email protected]>

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

…#4470) * apply filesystem from C+17 to handle encoding on Windows Signed-off-by: Chun-Wei Chen <[email protected]> * add comment in CMakeLists Signed-off-by: Chun-Wei Chen <[email protected]> * precise msg if missing support Signed-off-by: Chun-Wei Chen <[email protected]> * void normalize_sep for two types Signed-off-by: Chun-Wei Chen <[email protected]> * remove typo const Signed-off-by: Chun-Wei Chen <[email protected]> * remove template functions to header Signed-off-by: Chun-Wei Chen <[email protected]> * apply clang-format Signed-off-by: Chun-Wei Chen <[email protected]> * use _wstat Signed-off-by: Chun-Wei Chen <[email protected]> * use define function to refactor code Signed-off-by: Chun-Wei Chen <[email protected]> * add required C++ version in readme Signed-off-by: Chun-Wei Chen <[email protected]> * void char* for std::string in test Signed-off-by: Chun-Wei Chen <[email protected]> * fix clang-format Signed-off-by: Chun-Wei Chen <[email protected]> * move wchar_t and wstring only for Windows Signed-off-by: Chun-Wei Chen <[email protected]> * refactor template Signed-off-by: Chun-Wei Chen <[email protected]> * typo Signed-off-by: Chun-Wei Chen <[email protected]> * use char tempalte in template Signed-off-by: Chun-Wei Chen <[email protected]> * add comments Signed-off-by: Chun-Wei Chen <[email protected]> * add tests for wstring Signed-off-by: Chun-Wei Chen <[email protected]> * typo Signed-off-by: Chun-Wei Chen <[email protected]> * nit comments Signed-off-by: Chun-Wei Chen <[email protected]> * use existing functions from std::filesystem::path Signed-off-by: Chun-Wei Chen <[email protected]> * honor != Signed-off-by: Chun-Wei Chen <[email protected]> * constexpr const char Signed-off-by: Chun-Wei Chen <[email protected]> * checker disallow absolute path in external tensors; remove related tests Signed-off-by: Chun-Wei Chen <[email protected]> * fix format Signed-off-by: Chun-Wei Chen <[email protected]> * add more checker tests Signed-off-by: Chun-Wei Chen <[email protected]> * black Signed-off-by: Chun-Wei Chen <[email protected]> * improve comments Signed-off-by: Chun-Wei Chen <[email protected]> Signed-off-by: Chun-Wei Chen <[email protected]> Signed-off-by: xadupre <[email protected]>

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

Signed-off-by: xadupre <[email protected]>

workflow_scripts/test_model_zoo.py

requirements-dev.txt

Signed-off-by: xadupre <[email protected]>

jcwchen

LGTM. Thank you for enabling the latest mypy!

gramalingam · 2022-09-23T15:55:30Z

Thanks for fixing these!

* Remove unnecessary import (onnx#4484) * remove unnecessary import Signed-off-by: xadupre <[email protected]> * lint Signed-off-by: xadupre <[email protected]> * black Signed-off-by: xadupre <[email protected]> * black Signed-off-by: xadupre <[email protected]> * lint Signed-off-by: xadupre <[email protected]> * restore old import Signed-off-by: xadupre <[email protected]> Signed-off-by: xadupre <[email protected]> * fix mypy issues Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]> * fix mypy issues Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]> * update mypy Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]> * primary ops to function milestone 1 (onnx#4458) * primary ops to function milestone 1 Signed-off-by: Liqun Fu <[email protected]> * float type Signed-off-by: Liqun Fu <[email protected]> * formatting Signed-off-by: Liqun Fu <[email protected]> * pass backend test Signed-off-by: Liqun Fu <[email protected]> * formatting Signed-off-by: Liqun Fu <[email protected]> * commit Signed-off-by: Liqun Fu <[email protected]> * format Signed-off-by: Liqun Fu <[email protected]> * format Signed-off-by: Liqun Fu <[email protected]> * layernorm Signed-off-by: Liqun Fu <[email protected]> * pass runtime check Signed-off-by: Liqun Fu <[email protected]> * format Signed-off-by: Liqun Fu <[email protected]> * pure function Signed-off-by: Liqun Fu <[email protected]> * format Signed-off-by: Liqun Fu <[email protected]> * reviewer's comments, clean up some Signed-off-by: Liqun Fu <[email protected]> * keep op original version Signed-off-by: Liqun Fu <[email protected]> * fix gtest.function_verify_test Signed-off-by: Liqun Fu <[email protected]> * formatting Signed-off-by: Liqun Fu <[email protected]> * update according to reviewer's comment Signed-off-by: Liqun Fu <[email protected]> Signed-off-by: Liqun Fu <[email protected]> Co-authored-by: G. Ramalingam <[email protected]> Signed-off-by: xadupre <[email protected]> * lint Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]> * remove # type: ignore for imports Signed-off-by: xadupre <[email protected]> * lint Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]> * Use filesystem to load filename to prevent encoding issues on Windows (onnx#4470) * apply filesystem from C+17 to handle encoding on Windows Signed-off-by: Chun-Wei Chen <[email protected]> * add comment in CMakeLists Signed-off-by: Chun-Wei Chen <[email protected]> * precise msg if missing support Signed-off-by: Chun-Wei Chen <[email protected]> * void normalize_sep for two types Signed-off-by: Chun-Wei Chen <[email protected]> * remove typo const Signed-off-by: Chun-Wei Chen <[email protected]> * remove template functions to header Signed-off-by: Chun-Wei Chen <[email protected]> * apply clang-format Signed-off-by: Chun-Wei Chen <[email protected]> * use _wstat Signed-off-by: Chun-Wei Chen <[email protected]> * use define function to refactor code Signed-off-by: Chun-Wei Chen <[email protected]> * add required C++ version in readme Signed-off-by: Chun-Wei Chen <[email protected]> * void char* for std::string in test Signed-off-by: Chun-Wei Chen <[email protected]> * fix clang-format Signed-off-by: Chun-Wei Chen <[email protected]> * move wchar_t and wstring only for Windows Signed-off-by: Chun-Wei Chen <[email protected]> * refactor template Signed-off-by: Chun-Wei Chen <[email protected]> * typo Signed-off-by: Chun-Wei Chen <[email protected]> * use char tempalte in template Signed-off-by: Chun-Wei Chen <[email protected]> * add comments Signed-off-by: Chun-Wei Chen <[email protected]> * add tests for wstring Signed-off-by: Chun-Wei Chen <[email protected]> * typo Signed-off-by: Chun-Wei Chen <[email protected]> * nit comments Signed-off-by: Chun-Wei Chen <[email protected]> * use existing functions from std::filesystem::path Signed-off-by: Chun-Wei Chen <[email protected]> * honor != Signed-off-by: Chun-Wei Chen <[email protected]> * constexpr const char Signed-off-by: Chun-Wei Chen <[email protected]> * checker disallow absolute path in external tensors; remove related tests Signed-off-by: Chun-Wei Chen <[email protected]> * fix format Signed-off-by: Chun-Wei Chen <[email protected]> * add more checker tests Signed-off-by: Chun-Wei Chen <[email protected]> * black Signed-off-by: Chun-Wei Chen <[email protected]> * improve comments Signed-off-by: Chun-Wei Chen <[email protected]> Signed-off-by: Chun-Wei Chen <[email protected]> Signed-off-by: xadupre <[email protected]> * remove unnecessary ignore-missing-type Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]> * remove two type ignore Signed-off-by: xadupre <[email protected]> * lint Signed-off-by: xadupre <[email protected]> * type Signed-off-by: xadupre <[email protected]> Signed-off-by: xadupre <[email protected]> Signed-off-by: sdpython <[email protected]> Signed-off-by: Liqun Fu <[email protected]> Signed-off-by: Chun-Wei Chen <[email protected]> Co-authored-by: sdpython <[email protected]> Co-authored-by: liqun Fu <[email protected]> Co-authored-by: G. Ramalingam <[email protected]> Co-authored-by: Chun-Wei Chen <[email protected]>

xadupre requested review from a team as code owners September 8, 2022 09:56

jcwchen reviewed Sep 8, 2022

View reviewed changes

justinchuby reviewed Sep 8, 2022

View reviewed changes

jcwchen mentioned this pull request Sep 8, 2022

Install specified mypy version first in Python Lint CI to make mypy version consistent #4498

Merged

xadupre requested review from jcwchen and justinchuby September 12, 2022 14:06

jcwchen reviewed Sep 12, 2022

View reviewed changes

justinchuby reviewed Sep 13, 2022

View reviewed changes

tools/style.sh Outdated Show resolved Hide resolved

xadupre and others added 13 commits September 14, 2022 15:53

fix mypy issues

6e7c7ea

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

fix mypy issues

1b90ac2

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

update mypy

d5a6414

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

lint

230b435

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

remove # type: ignore for imports

642ba23

Signed-off-by: xadupre <[email protected]>

lint

aee6153

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

remove unnecessary ignore-missing-type

9ea3b41

Signed-off-by: sdpython <[email protected]> Signed-off-by: xadupre <[email protected]>

remove two type ignore

f04431f

Signed-off-by: xadupre <[email protected]>

lint

9ae1994

Signed-off-by: xadupre <[email protected]>

Merge branch 'main' into style2

aa6ca40

xadupre requested a review from jcwchen September 14, 2022 17:26

jcwchen reviewed Sep 14, 2022

View reviewed changes

workflow_scripts/test_model_zoo.py Outdated Show resolved Hide resolved

requirements-dev.txt Outdated Show resolved Hide resolved

type

3290a85

Signed-off-by: xadupre <[email protected]>

jcwchen approved these changes Sep 14, 2022

View reviewed changes

xadupre added 3 commits September 15, 2022 09:57

Merge branch 'main' of https://github.com/onnx/onnx into style2

d0c6e14

Merge branch 'main' of https://github.com/onnx/onnx into style2

6605752

Merge branch 'main' of https://github.com/onnx/onnx into style2

6d2eef3

Merge branch 'main' into style2

4320e58

gramalingam approved these changes Sep 23, 2022

View reviewed changes

gramalingam enabled auto-merge (squash) September 23, 2022 15:55

gramalingam merged commit 7f0a9f2 into onnx:main Sep 23, 2022

jcwchen mentioned this pull request Sep 23, 2022

Upgrade mypy to latest version #3964

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduces the number of ignored mypy errors #4495

Reduces the number of ignored mypy errors #4495

xadupre commented Sep 8, 2022 •

edited

Loading

jcwchen left a comment

justinchuby Sep 8, 2022

xadupre Sep 13, 2022

justinchuby commented Sep 8, 2022

jcwchen left a comment

jcwchen Sep 12, 2022 •

edited

Loading

jcwchen Sep 12, 2022 •

edited

Loading

justinchuby Sep 13, 2022

xadupre Sep 13, 2022

jcwchen left a comment

gramalingam commented Sep 23, 2022

Reduces the number of ignored mypy errors #4495

Reduces the number of ignored mypy errors #4495

Conversation

xadupre commented Sep 8, 2022 • edited Loading

Description

Motivation and Context

jcwchen left a comment

Choose a reason for hiding this comment

justinchuby Sep 8, 2022

Choose a reason for hiding this comment

xadupre Sep 13, 2022

Choose a reason for hiding this comment

justinchuby commented Sep 8, 2022

jcwchen left a comment

Choose a reason for hiding this comment

jcwchen Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

jcwchen Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

justinchuby Sep 13, 2022

Choose a reason for hiding this comment

xadupre Sep 13, 2022

Choose a reason for hiding this comment

jcwchen left a comment

Choose a reason for hiding this comment

gramalingam commented Sep 23, 2022

xadupre commented Sep 8, 2022 •

edited

Loading

jcwchen Sep 12, 2022 •

edited

Loading

jcwchen Sep 12, 2022 •

edited

Loading