areusch commented on code in PR #10963:
URL: https://github.com/apache/tvm/pull/10963#discussion_r878523782
##########
python/tvm/autotvm/measure/measure_methods.py:
##########
@@ -496,7 +497,8 @@ def set_task(self, task):
def _build_func_common(measure_input, runtime=None, check_gpu=None,
build_option=None):
"""Common part for building a configuration"""
target, task, config = measure_input
- target, task.target_host = Target.check_and_update_host_consist(target,
task.target_host)
+ assert not isinstance(target, (Map, dict)), "It's expected that 'target'
is a string here."
Review Comment:
the reason i asked for the assert was that `build()` was previously called
below passing `target_host=task.target`. that's different from what may happen
now. when `target` is `Map`, Target.check_and_update_host_consist may
[drop](https://github.com/apache/tvm/blob/main/python/tvm/target/target.py#L311)
`task.target_host` from the returned `target` object. what's changing then is
the way this particular function pre-processes `target`. I don't fully
understand why someone would pass a `Map` in for `target` here, but if they
did, this change could break that usage. i'd rather assert than silently break
it. i don't care where we do it, but i do think avoiding silent break would be
better. at some point we are going to get rid of
`check_and_update_host_consist`, so putting the assert right next to it seems
like the easiest way to prompt a future cleanup.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
