-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
[tvmc] Add a --config option to tvmc compile
#8253
Conversation
if config_type == "IntImm": | ||
# "Bool" configurations in the PassContext are recognized as | ||
# IntImm, so deal with this case here | ||
mapping_values = { | ||
"false": False, | ||
"true": True, | ||
} | ||
|
||
if value.isdigit(): | ||
parsed_value = int(value) | ||
else: | ||
# if not an int, accept only values on the mapping table, case insensitive | ||
parsed_value = mapping_values.get(value.lower(), None) | ||
|
||
if parsed_value is None: | ||
raise TVMCException(f"Invalid value '{value}' for configuration '{name}'. ") | ||
|
||
if config_type == "runtime.String": | ||
parsed_value = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if config_type == "IntImm": | |
# "Bool" configurations in the PassContext are recognized as | |
# IntImm, so deal with this case here | |
mapping_values = { | |
"false": False, | |
"true": True, | |
} | |
if value.isdigit(): | |
parsed_value = int(value) | |
else: | |
# if not an int, accept only values on the mapping table, case insensitive | |
parsed_value = mapping_values.get(value.lower(), None) | |
if parsed_value is None: | |
raise TVMCException(f"Invalid value '{value}' for configuration '{name}'. ") | |
if config_type == "runtime.String": | |
parsed_value = value | |
parsed_value = value | |
if config_type == "IntImm": | |
if value.isdigit(): | |
parsed_value = int(value) | |
else: | |
# must be boolean values if not an int | |
try: | |
parsed_value = bool(distutils.util.strtobool(value)) | |
except ValueError as err: | |
raise TVMCException(f"Invalid value '{value}' for configuration '{name}'. ") |
If the dependency of distuilts
is a concen, the following also works:
try:
parsed_value = json.loads(value)
except json.decoder.JSONDecodeError as err:
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some investigation on this, and wrt to the distutils
I didn't want to add that dependency here, because I think it would be a a bit misplaced.
Also wrt to the json
approach, I think it would still require more validation because the allowed values for that option are "int numbers", "true" or "false", and opening that to "json.loads" would add all sorts of json passing, then requiring more validation. That's why I added my own mapping table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm you already processed int so I don't think it would be an issue here. Anyways, I don't have a strong preference of using json
so I'll commit.
* Allow to send some configurations to the PassContext via command line * Add various validations to the new option with appropriate error messages * Add unit testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if config_type == "IntImm": | ||
# "Bool" configurations in the PassContext are recognized as | ||
# IntImm, so deal with this case here | ||
mapping_values = { | ||
"false": False, | ||
"true": True, | ||
} | ||
|
||
if value.isdigit(): | ||
parsed_value = int(value) | ||
else: | ||
# if not an int, accept only values on the mapping table, case insensitive | ||
parsed_value = mapping_values.get(value.lower(), None) | ||
|
||
if parsed_value is None: | ||
raise TVMCException(f"Invalid value '{value}' for configuration '{name}'. ") | ||
|
||
if config_type == "runtime.String": | ||
parsed_value = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm you already processed int so I don't think it would be an issue here. Anyways, I don't have a strong preference of using json
so I'll commit.
just curious if there will be a disk format as well so that people don't have to maintain long command-lines? |
* Allow to send some configurations to the PassContext via command line * Add various validations to the new option with appropriate error messages * Add unit testing
* Allow to send some configurations to the PassContext via command line * Add various validations to the new option with appropriate error messages * Add unit testing
[tvmc] Add a
--config
option totvmc compile
:PassContext
via command lineThis builds on top of #8212 and #8226, in which I exposed such API parts to the Python layer.
cc @gromero @comaniac @manupa-arm