You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/11/12 23:05:54 UTC
[GitHub] [tvm] shingjan opened a new pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
shingjan opened a new pull request #9505:
URL: https://github.com/apache/tvm/pull/9505
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on a change in pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#discussion_r752456569
##########
File path: tests/python/unittest/test_tvmscript_type.py
##########
@@ -14,7 +14,7 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-# pylint: disable=missing-function-docstring,missing-module-docstring,invalid-name,pointless-string-statement
+# pylint: disable=missing-function-docstring,missing-module-docstring,invalid-name,pointless-string-statement,line-too-long
Review comment:
rather than disable it, `line-too-long` should be fixed if possible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#issuecomment-975996326
Shall we merge this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#discussion_r753895109
##########
File path: tests/python/unittest/test_tvmscript_type.py
##########
@@ -173,7 +173,7 @@ def different_access_indices(a: T.handle, b: T.handle) -> None:
]
)
with T.init():
- B[vj, vi] = T.exp(B[vi, vj])
+ B[vj, vi] = T.exp(B[vj, vi], dtype="float32")
Review comment:
Does it mean that we no longer allow T.exp without dtype explicitly set? If not, what's the default dtype for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#issuecomment-972474426
It is an issue of mypy that can't be fixed - it requires slice must be constructed with int rather than custom type
see https://github.com/python/typing/issues/159
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#issuecomment-973397767
There are still some CI error. Please make sure it can pass locally
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 edited a comment on pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#issuecomment-967717478
Thank you so much for acting swiftly on this!! BTW, would you mind also adding T.exp in the unittest?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#issuecomment-972139411
@shingjan please address comments above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#issuecomment-972466384
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#discussion_r753895146
##########
File path: tests/python/unittest/test_tvmscript_type.py
##########
@@ -173,7 +173,7 @@ def different_access_indices(a: T.handle, b: T.handle) -> None:
]
)
with T.init():
- B[vj, vi] = T.exp(B[vi, vj])
+ B[vj, vi] = T.exp(B[vj, vi], dtype="float32")
Review comment:
Does it mean that we no longer allow T.exp without dtype explicitly set? If not, what's the default dtype for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 merged pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #9505:
URL: https://github.com/apache/tvm/pull/9505
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#issuecomment-967717478
Thank you so much for acting swiftly on this!! BTW, would you mind also adding T.exp?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#discussion_r754009189
##########
File path: tests/python/unittest/test_tvmscript_type.py
##########
@@ -173,7 +173,7 @@ def different_access_indices(a: T.handle, b: T.handle) -> None:
]
)
with T.init():
- B[vj, vi] = T.exp(B[vi, vj])
+ B[vj, vi] = T.exp(B[vj, vi], dtype="float32")
Review comment:
If we do need to set a default dtype for intrinsics defined outside of `script/tir/intrinsic`, like `exp` here defined in `tir/op`, is `float32` reasonable here? We could do something like
```def exp(x: PrimExpr, dtype: str = "float32") -> PrimExpr: ...```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on pull request #9505: [TVMScript][Fix] Add type hints for more uncovered cases
Posted by GitBox <gi...@apache.org>.
shingjan commented on pull request #9505:
URL: https://github.com/apache/tvm/pull/9505#issuecomment-972405205
@vinx13 Comments addressed. I fixed the CI issue by disabling the mypy check on slice index of that two lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org