Skip to content

feat: Enable mypy type checking #35

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.

Sign up for GitHub

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

Jump to bottom
Merged
junrushao merged 1 commit into from
Sep 22, 2025
Merged

feat: Enable mypy type checking #35

junrushao merged 1 commit into from
Sep 22, 2025

Conversation

@junrushao
Copy link
Member

@junrushao junrushao commented Sep 21, 2025 •
edited
Loading

With this PR, we are able to enable mypy type checking for the following directories:

  • python/ and tests/
  • examples/inline_module
  • examples/packaging
  • examples/quick_start

@junrushao junrushao marked this pull request as ready for review September 21, 2025 01:38
@junrushao junrushao changed the title feat: Enable mypy type checking under python feat: Enable mypy type checking under ./python and ./tests Sep 21, 2025
@junrushao junrushao force-pushed the 2025-09-20/mypy-compat branch 2 times, most recently from 3250407 to 57b12df Compare September 21, 2025 07:08
@junrushao junrushao changed the title feat: Enable mypy type checking under ./python and ./tests feat: Enable mypy type checking Sep 21, 2025
@junrushao junrushao force-pushed the 2025-09-20/mypy-compat branch 2 times, most recently from 14fbc9e to 03f3629 Compare September 22, 2025 00:38
junrushao
junrushao commented Sep 22, 2025
View reviewed changes
python/tvm_ffi/testing.py
class _TestCxxClassDerivedDerived(_TestCxxClassDerived):
v_str: str = field(default_factory=lambda: "default")
v_bool: bool
v_str: str = field(default_factory=lambda: "default") # type: ignore[assignment]
Copy link
Member Author

@junrushao junrushao Sep 22, 2025

Choose a reason for hiding this comment

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

TODO: need to figure out why field(...) triggers a mypy complain, despite field_specifiers are used properly

@junrushao junrushao requested a review from tqchen September 22, 2025 00:47
@junrushao junrushao force-pushed the 2025-09-20/mypy-compat branch from 03f3629 to bdbbde4 Compare September 22, 2025 00:48
@junrushao
Copy link
Member Author

junrushao commented Sep 22, 2025 •
edited
Loading

Looks like the only controversy [1][2] is whether or not we want generated methods to be put under TYPE_CHECKING or not.

I don't really have strong opinion so @tqchen you may make the call. Arguments on both sides are valid:

  • Reason why they should reside under TYPE_CHECKING: Those are not real methods with concrete implementation.
  • Reason why they should not reside under TYPE_CHECKING: the extra indent looks not super necessary

@junrushao junrushao mentioned this pull request Sep 22, 2025
Merged
@tqchen
Copy link
Member

tqchen commented Sep 22, 2025

if correctness is not an issue, i feel not put under TYPE_CHECKING is better, it is usually ok to put type annotation this way IMO

@junrushao junrushao force-pushed the 2025-09-20/mypy-compat branch 2 times, most recently from 9af2a97 to 32307ff Compare September 22, 2025 17:41
junrushao
junrushao commented Sep 22, 2025
View reviewed changes
@junrushao junrushao force-pushed the 2025-09-20/mypy-compat branch 2 times, most recently from acaa39d to 934993f Compare September 22, 2025 18:29
@junrushao junrushao force-pushed the 2025-09-20/mypy-compat branch from 934993f to 52eba1b Compare September 22, 2025 19:07
tqchen
tqchen reviewed Sep 22, 2025
View reviewed changes
python/tvm_ffi/testing.py Outdated
from .registry import register_object

if TYPE_CHECKING:
from tvm_ffi import Array, Map
Copy link
Member

@tqchen tqchen Sep 22, 2025

Choose a reason for hiding this comment

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

Given array map is used below (and notunder type chekcing), should it now not use type checking? or use String?

Copy link
Member Author

@junrushao junrushao Sep 22, 2025

Choose a reason for hiding this comment

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

the original intention is to avoid potential cyclic dependencies, but indeed there's no cyclic dependency in this case. will remove

python/tvm_ffi/_ffi_api.py Outdated

from . import registry

if TYPE_CHECKING:
Copy link
Member

@tqchen tqchen Sep 22, 2025

Choose a reason for hiding this comment

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

get rid of this TYPE_CHECKING or move to a pyi file

Copy link
Member Author

@junrushao junrushao Sep 22, 2025

Choose a reason for hiding this comment

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

will move to a pyi file

@junrushao junrushao force-pushed the 2025-09-20/mypy-compat branch from 52eba1b to 85b8928 Compare September 22, 2025 19:35
@junrushao junrushao force-pushed the 2025-09-20/mypy-compat branch from 85b8928 to 8dfcb68 Compare September 22, 2025 19:50
@junrushao junrushao merged commit 40e9c83 into apache:main Sep 22, 2025
6 checks passed
tqchen pushed a commit that referenced this pull request Sep 22, 2025
@junrushao
feat(typing): Parameterizable Array[T] and Map[K, V] (#37)
df58a05 
Depends on #35.

Previously `Array` and `Map` types, despite that they inherit from
`collections.abc.Sequence` and `Mapping`, are not properly
parameterized. This PR makes it work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tqchen tqchen tqchen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@junrushao @tqchen