Skip to content

[TVMScript][Fix] Add type hints for more uncovered cases - #9505

Merged
vinx13 merged 7 commits into
apache:mainfrom
shingjan:tvmscript-type-hint-fix
Nov 22, 2021
Merged

[TVMScript][Fix] Add type hints for more uncovered cases#9505
vinx13 merged 7 commits into
apache:mainfrom
shingjan:tvmscript-type-hint-fix

Conversation

@shingjan

@shingjan shingjan commented Nov 12, 2021

Copy link
Copy Markdown

This PR intends to add type hints for some uncovered cases in TVMScript.

cc: @junrushao1994 @vinx13

@junrushao

junrushao commented Nov 12, 2021

Copy link
Copy Markdown
Member

Thank you so much for acting swiftly on this!! BTW, would you mind also adding T.exp in the unittest?

@vinx13

vinx13 commented Nov 17, 2021

Copy link
Copy Markdown
Member

@shingjan please address comments above

@shingjan
shingjan force-pushed the tvmscript-type-hint-fix branch from b38fccf to 96740bc Compare November 18, 2021 01:25
@shingjan

Copy link
Copy Markdown
Author

@vinx13 Comments addressed. I fixed the CI issue by disabling the mypy check on slice index of that two lines.

@junrushao

Copy link
Copy Markdown
Member

The following two errors should be fixed rather than skipped (if possible)

tests/python/unittest/test_tvmscript_type.py: note: In function "different_access_indices":
tests/python/unittest/test_tvmscript_type.py:168:29: error: Slice index must be an integer or None
tests/python/unittest/test_tvmscript_type.py:169:29: error: Slice index must be an integer or None

@vinx13

vinx13 commented Nov 18, 2021

Copy link
Copy Markdown
Member

It is an issue of mypy that can't be fixed - it requires slice must be constructed with int rather than custom type
see python/typing#159

Comment thread tests/python/unittest/test_tvmscript_type.py Outdated
@shingjan
shingjan requested a review from vinx13 November 18, 2021 21:32
@vinx13

vinx13 commented Nov 18, 2021

Copy link
Copy Markdown
Member

There are still some CI error. Please make sure it can pass locally

Comment thread tests/python/unittest/test_tvmscript_type.py
@junrushao

Copy link
Copy Markdown
Member

Shall we merge this PR?

@vinx13
vinx13 merged commit cdca84a into apache:main Nov 22, 2021
@shingjan
shingjan deleted the tvmscript-type-hint-fix branch November 23, 2021 21:58
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021
* add support for prevously uncovered cases

* remove PrimExpr import

* add exp test and mypy ignore

* disable ling too long

* resolve long line

* nit

* add dtype to unary ops
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021
* add support for prevously uncovered cases

* remove PrimExpr import

* add exp test and mypy ignore

* disable ling too long

* resolve long line

* nit

* add dtype to unary ops
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
* add support for prevously uncovered cases

* remove PrimExpr import

* add exp test and mypy ignore

* disable ling too long

* resolve long line

* nit

* add dtype to unary ops
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
* add support for prevously uncovered cases

* remove PrimExpr import

* add exp test and mypy ignore

* disable ling too long

* resolve long line

* nit

* add dtype to unary ops
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* add support for prevously uncovered cases

* remove PrimExpr import

* add exp test and mypy ignore

* disable ling too long

* resolve long line

* nit

* add dtype to unary ops

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Help my super box is stuck on android I really need help please didn't watch TV in too much. I'm losing my mind. I'm gonna wind up doing something to crypto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants