Add MYPYSTICT lintrunner job by jacobhinkle · Pull Request #1077 · NVIDIA/Fuser

jacobhinkle · 2023-10-12T23:36:47Z

Enabling mypy will require some fixes to our python code but should help keep our ever-growing python codebase clean. Similar to the clang-tidy situation, this may mean we should either fix a bunch of errors up front or CI will fail when modifying these files in the future.

The mypy lintrunner jobs were not running since these files were missing. Enabling this will require some fixes to our python code but should help keep our ever-growing python codebase clean.

jacobhinkle · 2023-10-12T23:38:48Z

mypy-strict.ini

+
+[mypy]
+python_version = 3.8
+#plugins = mypy_plugins/check_mypy_version.py, numpy.typing.mypy_plugin


This is the only change I made to these files. I don't think we need the mypy version check since we install 1.4.0 in the lintrunner config anyway. I haven't looked through everything else yet. We might want a minimal set of options, or to disable the strict version.

Another simple approach is to remove the strict version and also remove the --config=mypy.ini argument (and these files), so that we'd use the default settings. We'd probably at least want to set --ignore-missing-imports, so perhaps trimming down mypy.ini to just a few options is preferable.

I'm wondering if we should just blankly skip checks on specific libraries? i.e.

[mypy] ignore_missing_imports = True

I know little about mypy, let me know if I'm talking non-sense 😆

jacobhinkle · 2023-10-12T23:45:18Z

mypy-strict.ini

+# files.
+
+[mypy]
+python_version = 3.8


jacobhinkle · 2023-10-13T00:38:46Z

.lintrunner.private.toml

@@ -0,0 +1 @@
+.lintrunner.toml


I'm not sure why this symlink is needed, but I have needed it to run lintrunner on my workstation also.

hmm. this shouldn't be needed. It's probably some funny script issue for lintrunner.

Meanwhile, can we change the command to lintrunner --config .lintrunner.toml XXXXXXX

jacobhinkle · 2023-10-14T15:23:25Z

nvfuser/nvfuser_version.py

        self._cls_name = cls_name

-    def get_cls(self):
+    def get_cls(self) -> Any:


I fixed this one missing return type annotation to try and trigger a mypy run over this file. Locally when I run lintrunner nvfuser/nvfuser_version.py I get a number of other errors.

jjsjann123 · 2023-10-16T18:12:00Z

Ha, this is likely something that gets accidentally dropped when switching to lintrunner.
Thanks for patching this.

jjsjann123

some nitpick here and there.

jjsjann123 · 2023-10-16T18:36:26Z

.lintrunner.private.toml

@@ -0,0 +1 @@
+.lintrunner.toml


hmm. this shouldn't be needed. It's probably some funny script issue for lintrunner.

Meanwhile, can we change the command to lintrunner --config .lintrunner.toml XXXXXXX

jjsjann123 · 2023-10-16T18:44:06Z

mypy-strict.ini

+
+[mypy]
+python_version = 3.8
+#plugins = mypy_plugins/check_mypy_version.py, numpy.typing.mypy_plugin


I'm wondering if we should just blankly skip checks on specific libraries? i.e.

[mypy] ignore_missing_imports = True

I know little about mypy, let me know if I'm talking non-sense 😆

This should force mypy to check all the .py files instead of just this stub

github-actions · 2025-07-24T11:34:10Z

Review updated until commit d3cb42b

Description

Add MYPYSTRICT lintrunner job for strict type checking
Update nvfuser_version.py to include type hint
Modify .github/workflows/lint.yml to use .lintrunner.toml
Remove MYPY and only run MYPYSTRICT
Update Python version to 3.12 and exclude some files

Changes walkthrough 📝

Relevant files

Enhancement

nvfuser_version.py Add type hint to `get_cls` python/nvfuser/nvfuser_version.py Add return type hint to `get_cls` method	+1/-1
lint.yml `Update lintrunner configuration` .github/workflows/lint.yml Update lintrunner command to use `.lintrunner.toml` Skip CLANGTIDY in lintrunner	+2/-2
.lintrunner.toml `Update MYPYSTRICT configuration` .lintrunner.toml Remove MYPY linter configuration Update MYPYSTRICT linter configuration	+0/-21
mypy-strict.ini `Add mypy-strict.ini` mypy-strict.ini Add strict mypy configuration for PyTorch codebase	+67/-0

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
⚡ Recommended focus areas for review Type Hinting The new method `get_cls` should have a return type specified. Currently, it uses `Any`, which defeats the purpose of using mypy for type checking. def get_cls(self) -> Any: MYPYSTRICT Configuration The `lintrunner` command in the CI pipeline now includes `MYPYSTRICT` but the configuration file `mypy-strict.ini` is not being used. Ensure that the `lintrunner` command points to the correct configuration file. lintrunner --config .lintrunner.toml --force-color --skip CLANGTIDY --all-files MYPY Removal The `MYPY` linter configuration has been removed and replaced with `MYPYSTRICT`. Verify that all necessary configurations have been transferred to `MYPYSTRICT` to maintain the same level of type checking. [[linter]]

I don't know if this is required but we currently get a warning about missing .lintrunner.private.toml even though that should be a link to .lintruner.toml

jacobhinkle · 2025-07-24T11:44:33Z

@jjsjann123 I updated this PR and I think it's ready for another look. If it gets to be a pain fixing typing errors we could disable it. What do you think?

jacobhinkle · 2025-07-24T11:57:48Z

Running locally via lintrunner I get mypy errors, but for some reason none are found in github CI. I need to figure that out before this will be considered ready.

jjsjann123 · 2025-07-25T16:43:43Z

@jjsjann123 I updated this PR and I think it's ready for another look. If it gets to be a pain fixing typing errors we could disable it. What do you think?

sounds good to me.
Sorry this somehow dropped off my radar. 🙇

Add mypy.ini and mypy-strict.ini

ff10947

The mypy lintrunner jobs were not running since these files were missing. Enabling this will require some fixes to our python code but should help keep our ever-growing python codebase clean.

jacobhinkle requested a review from jjsjann123 October 12, 2023 23:36

jacobhinkle commented Oct 12, 2023

View reviewed changes

mypy-strict.ini Outdated

# files.

[mypy]

python_version = 3.8

Copy link

Collaborator Author

jacobhinkle Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1078

jacobhinkle added 2 commits October 12, 2023 20:13

Minor change to trigger CI on nvfuser_version.py

e4b30c0

Add .lintrunner.private.toml -> .lintrunner.toml

1c641d8

jacobhinkle commented Oct 13, 2023

View reviewed changes

jacobhinkle added 4 commits October 12, 2023 20:52

Trying to get lintruner to fire in CI...

f235495

Change lintrunner options to try and fix CI

e65fc4b

Change back to --all-files

96d394c

Remove libtinfo5 and --force-color

7c24b99

jacobhinkle commented Oct 14, 2023

View reviewed changes

jjsjann123 reviewed Oct 16, 2023

View reviewed changes

jacobhinkle and others added 10 commits October 17, 2023 05:56

Merge branch 'main' into mypy_config

abe49e4

Ignore pkg_resources missing imports, no others

b22fb89

Manually specify config file

2148d04

Try removing stub __init__.pyi

46533c0

This should force mypy to check all the .py files instead of just this stub

Merge remote-tracking branch 'origin/main' into mypy_config

0251bb8

Remove config option from lintrunner init

fe83788

Merge remote-tracking branch 'origin/main' into mypy_config

2cbdbb9

Remove MYPY and only run MYPYSTRICT

67d9c0e

Update python version to 3.12

85a663e

Exclude some files to speed things up

f991ae7

jacobhinkle changed the title ~~Add mypy.ini and mypy-strict.ini~~ Add MYPYSTICT lintrunner job Jul 24, 2025

jacobhinkle added 2 commits July 24, 2025 07:37

Reset some options that might not be needed in github workflow

2c497f5

Explicitly provide lintrunner config location

d3cb42b

I don't know if this is required but we currently get a warning about missing .lintrunner.private.toml even though that should be a link to .lintruner.toml

jacobhinkle mentioned this pull request Jul 24, 2025

[DO NOT MERGE] Test mypy linting #4838

Draft

Comments

Conversation

jacobhinkle commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjsjann123 commented Oct 16, 2023

Uh oh!

jjsjann123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes walkthrough 📝

PR Reviewer Guide 🔍

Uh oh!

jacobhinkle commented Jul 24, 2025

Uh oh!

jacobhinkle commented Jul 24, 2025

Uh oh!

jjsjann123 commented Jul 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jacobhinkle commented Oct 12, 2023 •

edited

Loading

github-actions bot commented Jul 24, 2025 •

edited

Loading