Skip to content
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

Merged
merged 1 commit into from
Jun 16, 2021
Merged

[tvmc] Add a --config option to tvmc compile #8253

merged 1 commit into from
Jun 16, 2021

Conversation

leandron
Copy link
Contributor

@leandron leandron commented Jun 14, 2021

[tvmc] Add a --config option to tvmc compile:

  • 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

This builds on top of #8212 and #8226, in which I exposed such API parts to the Python layer.

cc @gromero @comaniac @manupa-arm

python/tvm/driver/tvmc/compiler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/common.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/common.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/common.py Outdated Show resolved Hide resolved
Comment on lines +423 to +457
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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:
    ...

Copy link
Contributor Author

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.

Copy link
Contributor

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.

python/tvm/driver/tvmc/compiler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/common.py Outdated Show resolved Hide resolved
tests/python/driver/tvmc/test_tvmc_common.py Show resolved Hide resolved
 * 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
@leandron
Copy link
Contributor Author

@comaniac @gromero I updated this incorporating most of you comments. Please have a look when you have a moment.

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +423 to +457
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
Copy link
Contributor

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.

@leandron leandron merged commit 0fc5b7b into apache:main Jun 16, 2021
@leandron leandron deleted the tvmc_config_opt branch June 16, 2021 16:48
@areusch
Copy link
Contributor

areusch commented Jun 16, 2021

just curious if there will be a disk format as well so that people don't have to maintain long command-lines?

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 17, 2021
* 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
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 17, 2021
* 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
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.

4 participants