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 2022/04/05 17:03:57 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #10906: [CI] Updated argument parsing of optional arguments in ci.py
Lunderberg opened a new pull request, #10906:
URL: https://github.com/apache/tvm/pull/10906
Previously, optional arguments were identified by comparing the string `"typing.Optional"`. This misses some cases, as `Optional[int]` expands to `Union[int, NoneType]`. This commit updates the check to identify `typing.Union` annotations where one of the types is `NoneType`.
--
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] Lunderberg commented on pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10906:
URL: https://github.com/apache/tvm/pull/10906#issuecomment-1089059313
@driazati Thank you for putting together the `ci.py` script, and I really like the convenience of it. This was something I ran into when trying to run `ci.py hexagon`, and the optional `--tests` argument gave an error stating that it was mandatory.
--
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] driazati commented on a diff in pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #10906:
URL: https://github.com/apache/tvm/pull/10906#discussion_r843123158
##########
tests/scripts/ci.py:
##########
@@ -32,6 +32,8 @@
import subprocess
import platform
import textwrap
+import typing
Review Comment:
could you add the `#!` line in this PR too since its pretty trivial
##########
tests/scripts/ci.py:
##########
@@ -434,6 +436,26 @@ def cli_name(s: str) -> str:
return s.replace("_", "-")
+def typing_get_origin(annotation):
+ if sys.version_info >= (3, 8):
+ return typing.get_origin(annotation)
+ else:
+ return annotation.__origin__
+
+
+def typing_get_args(annotation):
+ if sys.version_info >= (3, 8):
+ return typing.get_args(annotation)
+ else:
+ return annotation.__args__
+
+
+def is_optional_type(annotation):
Review Comment:
this bit fixes the 3.7 issue
```suggestion
def is_optional_type(annotation):
if not hasattr(annotation, "__origin__"):
return False
```
--
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] Lunderberg commented on a diff in pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #10906:
URL: https://github.com/apache/tvm/pull/10906#discussion_r843136706
##########
tests/scripts/ci.py:
##########
@@ -434,6 +436,26 @@ def cli_name(s: str) -> str:
return s.replace("_", "-")
+def typing_get_origin(annotation):
+ if sys.version_info >= (3, 8):
+ return typing.get_origin(annotation)
+ else:
+ return annotation.__origin__
Review Comment:
Thank you! It looks like python3.9+ represent it as `typing.Optional`, while earlier versions give `typing.Union`. After some quick testing, checking for `__origin__` works on all versions. Unfortunately, I don't see any public classes for checking if the annotation is any `typing.*` class, as the `typing._Final` class is private.
--
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] Lunderberg commented on a diff in pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #10906:
URL: https://github.com/apache/tvm/pull/10906#discussion_r843126588
##########
tests/scripts/ci.py:
##########
@@ -32,6 +32,8 @@
import subprocess
import platform
import textwrap
+import typing
Review Comment:
Since the `#!` line was already present, I've made the `chmod` change here as well.
--
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] Lunderberg commented on pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10906:
URL: https://github.com/apache/tvm/pull/10906#issuecomment-1090350420
Bumping CI to restart, [possible flaky test](https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-10906/2/tests).
--
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] driazati commented on pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
driazati commented on PR #10906:
URL: https://github.com/apache/tvm/pull/10906#issuecomment-1089129468
`chmod` sounds good to be too (though I've been skirting around it by using `alias ci=python tests/scripts.ci.py` in my shell)
--
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] gromero commented on a diff in pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
gromero commented on code in PR #10906:
URL: https://github.com/apache/tvm/pull/10906#discussion_r843215881
##########
tests/scripts/ci.py:
##########
@@ -32,6 +32,8 @@
import subprocess
import platform
import textwrap
+import typing
Review Comment:
@Lunderberg thanks!
--
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] Lunderberg commented on pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10906:
URL: https://github.com/apache/tvm/pull/10906#issuecomment-1089127742
I very much like the idea of adding the `chmod`, especially if we also add the standard `#!/usr/bin/env python3` at the top.
--
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] leandron merged pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
leandron merged PR #10906:
URL: https://github.com/apache/tvm/pull/10906
--
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] gromero commented on pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
gromero commented on PR #10906:
URL: https://github.com/apache/tvm/pull/10906#issuecomment-1089063928
@Lunderberg @driazati I was wondering if we could also `chmod +x ci.py`. wdyt?
--
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] driazati commented on a diff in pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #10906:
URL: https://github.com/apache/tvm/pull/10906#discussion_r843120750
##########
tests/scripts/ci.py:
##########
@@ -434,6 +436,26 @@ def cli_name(s: str) -> str:
return s.replace("_", "-")
+def typing_get_origin(annotation):
+ if sys.version_info >= (3, 8):
+ return typing.get_origin(annotation)
+ else:
+ return annotation.__origin__
Review Comment:
Thanks for the fix! Python's treatment of `Optional` is pretty annoying and has changed at some point between 3.7 and 3.10 (moved from an alias to `Union` to its own thing), I noticed on 3.7 this fails though:
```
Traceback (most recent call last):
File "tests/scripts/ci.py", line 681, in <module>
main()
File "tests/scripts/ci.py", line 657, in main
add_subparser(func, subparsers)
File "tests/scripts/ci.py", line 506, in add_subparser
is_optional = is_optional_type(value.annotation)
File "tests/scripts/ci.py", line 456, in is_optional_type
return (typing_get_origin(annotation) == typing.Union) and (
File "tests/scripts/ci.py", line 445, in typing_get_origin
return annotation.__origin__
AttributeError: type object 'bool' has no attribute '__origin__'
```
--
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] driazati commented on a diff in pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
driazati commented on code in PR #10906:
URL: https://github.com/apache/tvm/pull/10906#discussion_r843123158
##########
tests/scripts/ci.py:
##########
@@ -32,6 +32,8 @@
import subprocess
import platform
import textwrap
+import typing
Review Comment:
~~could you add the `#!` line in this PR too since its pretty trivial~~ nevermind it's already there
--
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] leandron commented on pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
leandron commented on PR #10906:
URL: https://github.com/apache/tvm/pull/10906#issuecomment-1091160830
Thanks @Lunderberg @gromero @driazati
--
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] Lunderberg commented on a diff in pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #10906:
URL: https://github.com/apache/tvm/pull/10906#discussion_r843134575
##########
tests/scripts/ci.py:
##########
@@ -434,6 +436,26 @@ def cli_name(s: str) -> str:
return s.replace("_", "-")
+def typing_get_origin(annotation):
+ if sys.version_info >= (3, 8):
+ return typing.get_origin(annotation)
+ else:
+ return annotation.__origin__
+
+
+def typing_get_args(annotation):
+ if sys.version_info >= (3, 8):
+ return typing.get_args(annotation)
+ else:
+ return annotation.__args__
+
+
+def is_optional_type(annotation):
Review Comment:
Thank you, and added in!
--
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] Lunderberg commented on pull request #10906: [CI] Updated argument parsing of optional arguments in ci.py
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #10906:
URL: https://github.com/apache/tvm/pull/10906#issuecomment-1089135466
Looks like I am misremembering the shebang line, which has been present since https://github.com/apache/tvm/pull/9534, so it would only need a chmod to be immediately executable.
--
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