Mousius commented on code in PR #10963:
URL: https://github.com/apache/tvm/pull/10963#discussion_r850363168
##########
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:
> Thanks, that was exactly the PR I had in mind. OK, I've read it again
carefully and I'm 100% with Manupa, Ashutosh, and Elen about we should not test
internal asserts (or, as Elen says, "developer facing asserts" ). I totally see
the necessary of having a good test coverage and adding tests like for
https://github.com/apache/tvm/pull/10865, but for this case what is the value?
We can also consider cases such as https://github.com/apache/tvm/pull/9299,
where because we didn't introduce a test to ensure the behaviour, we opened
ourselves up to typos which impact the behaviour without our knowledge - this
was later fixed with error handling tested in
https://github.com/apache/tvm/pull/9470. There's other instances such as
https://github.com/apache/tvm/pull/9313 where there has been testing of
internal functions error handling without any push back at all. Testing units
in isolation is common practice in many software projects, they're low effort
and quick to run so I'm unsure why there's such fierce pushback against them in
TVM.
> Testing on a code refactor if the assert gets messed up or removed (a
regression in the assert itself) from _build_func_common? Besides that the
other tests for more public interfaces should also be already exercising the
assert() properly (giving a more meaningful full traceback in case the assert
fires).
This is an interesting point, because it raises the question as to where we
should be testing this, I previously mentioned we might write a test in
`LocalBuilder` to check this rather than actually exercising
`_build_func_common` itself. I agree `_build_func_common` was not the best
place to write the actual test for this behaviour but rather
`check_and_update_host_consist`. `check_and_update_host_consist` itself doesn't
have this as part of its interface (which seems the more appropriate place to
do input validation) so @areusch had to reverse engineer it to figure out that
interface, this seems to motivate testing internal interfaces. I'd rather see
us asking how best to test something and ensure the internal interfaces are
well understood than trying to find exclusions from our test coverage
guidelines.
--
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]
