[AutoTVM] Use new target object #10963
Conversation
|
This commit removes the following deprecate warning when using The new target object was introduced by #7534 |
| @@ -496,7 +496,7 @@ 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) | |||
| target, _ = Target.check_and_update_host_consist(target, task.target_host) | |||
There was a problem hiding this comment.
could we add an assert before this statement to validate that target is not Map or dict? looking at check_and_update_host_consist, this should behave properly if those two things hold, but might not otherwise.
There was a problem hiding this comment.
@areusch good catch, thanks! Done.
It seems all tests will pass fine but I'm now intrigued if that will work for all use cases of tvmc tune, can't foresee any issue but not really sure.
There was a problem hiding this comment.
Just coming back to this, if the expectation is that we don't pass those things into check_and_update_host_consist then shouldn't it validate the inputs itself? As in, place the assert within check_and_update_host_consist?
There was a problem hiding this comment.
Also, if we're unsure of the behaviour of check_and_update_host_consist in different scenarios we should add tests to exercise it?
There was a problem hiding this comment.
@mbs-octoml is working to overhaul this function to work with VirtualDevice, so I think it may not be worth spending cycles on that right now. I agree with you otherwise @Mousius, though here my request comes more from that it's really tricky to mentally model what may happen in all the various scenarios with that function.
There was a problem hiding this comment.
it's really tricky to mentally model what may happen in all the various scenarios with that function
One excellent reason to have comprehensive test cases for these internal functions 😸
Use new target object tvm.target.Targe(), which now holds both 'target' and 'target_host', in AutoTVM measurement functions. At some point the tvm.driver.build() will be called by AutoTVM and it expects now such a kind of target instead of the old (separate) one for 'target' and 'target_host'. This avoids several deprecate warning messages caused by build() when AutoTVM is used by tools like 'tvmc'. Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
gromero
force-pushed
the
tvmc_tune_fix_target_deprecate_warnings
branch
from
60044d9
to
de4fb30
Compare
| @@ -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." | |||
There was a problem hiding this comment.
Can we add a unit test to check this assertion fires correctly if the wrong thing is passed?
There was a problem hiding this comment.
@Mousius I think we could but what would be the achievement exactly with a unit test for that internal function (_build_func_common)? More broadly, what has been our policy about testing assert in TVM code? Should we test all the asserts? I recall there were a discussion some time ago about it but could not find the PR, iirc you're discussing that with other Arm folks.
There was a problem hiding this comment.
I think we could but what would be the achievement exactly with a unit test for that internal function (_build_func_common)?
@gromero it's generally good practice to have comprehensive test coverage, including input validation and error handling (which is essentially what these asserts do). One reason is to figure out whether or not we can actually trigger such a situation from a more public interface (i.e. test LocalBuilder in some way to trigger this), another is to document the expected interface of the function so it's well understood how we expect it to behave.
More broadly, what has been our policy about testing assert in TVM code? Should we test all the asserts? I recall there were a discussion some time ago about it but could not find the PR, iirc you're discussing that with other Arm folks.
There was a discussion awhile back on #9331 (comment), which was resolved by @manupa-arm merging the PR without the tests added, no follow up conversation has happened since. Different people follow the Code Review Guidelines differently, and there are contributions without testing at all, so I think the policy is very much down to the Committer rather than strictly defined.
There was a problem hiding this comment.
I think we could but what would be the achievement exactly with a unit test for that internal function (_build_func_common)?
@gromero it's generally good practice to have comprehensive test coverage, including input validation and error handling (which is essentially what these
asserts do). One reason is to figure out whether or not we can actually trigger such a situation from a more public interface (i.e. testLocalBuilderin some way to trigger this), another is to document the expected interface of the function so it's well understood how we expect it to behave.
I think my question was not clear, maybe I should have said "[...] unit test for that assert in that internal function [...]". I agreed with the test coverage necessity and also with Andrew's suggestion for adding the assert, and everything else about the assert. My question was actually about testing (with a unit test) the assert in that specific function.
More broadly, what has been our policy about testing assert in TVM code? Should we test all the asserts? I recall there were a discussion some time ago about it but could not find the PR, iirc you're discussing that with other Arm folks.
There was a discussion awhile back on #9331 (comment), which was resolved by @manupa-arm merging the PR without the tests added, no follow up conversation has happened since. Different people follow the Code Review Guidelines differently, and there are contributions without testing at all, so I think the policy is very much down to the Committer rather than strictly defined.
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 #10865, but for this case what is the value? 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).
There was a problem hiding this comment.
asserts aside, let me check again check_and_update_host_consist and the whole Target implementation it might be better fix (there always is...). I think check_and_update_host_consist is just an helper (static) function added as helper for the transition to the new target object, but why no creating a brand new shiny Target() instead of using that helper function I wonder.
There was a problem hiding this comment.
There was a problem hiding this comment.
Apologies for not getting back to you on this! I was referring to testing this at the higher interface than _build_func_common which I think you're right would be here as that's the tune interface. In doing so the assert could remain in _build_func_common as the interface would eventually be fulfilled by the internal function raising an error, but I think you're right that it'd be much clearer if the check was next to the inputs to tune.
Having said that, I would still strongly favour keeping the assertions inside of the Target API, as it seems as though we're defensively coding around our own internals here rather than actively wanting to clamp tune to only accepting these inputs?
There was a problem hiding this comment.
Having said that, I would still strongly favour keeping the assertions inside of the Target API, as it seems as though we're defensively coding around our own internals here rather than actively wanting to clamp tune to only accepting these inputs?
i mean sure, i agree, but i think you're asking for a different PR to be written here. i think it'd be great to merge this
(with the assert moved up to tune) as a step in the right direction of simplifying Target usage, and address the Target thing in a Target-focused PR rather than a cleanup in another component.
There was a problem hiding this comment.
I think we agree that the assert should be in the Target code, I totally agree that it's outside of the scope of this PR.
If we introduce an assert in this PR, when does that get cleaned up? If it's an immediate follow up PR, then we can either not introduce it in this PR or we can put it anywhere as it'll get removed straight away. If not, we've left this in an untested state, with an extra assert for a different component which may never get cleaned up.
At a cursory glance I think check_and_update_host_consist supports any of the valid Target formats? So I'd suggest a follow up clarifying that with test cases.
In the spirit of getting this merged rather than continuing to debate that function here, I would go without the assert - what do you think?
There was a problem hiding this 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 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.
|
Shoot, I've stepped on this in #11382, sorry about that. In that PR I clearly distinguish the 'multi-target' APIs from the 'single-target' APIs. Everything in the tuning world needs to use the 'single-target' APIs. I'd be fine with an assert isinstance(target, tvm.target.Target) if we know all codepaths respect that. But I think targets transition from Target to str and back quite a bit in the tuning world, so I'd suggest just letting the canonicalization deal with it. |
areusch
added
needs-triage
|
@gromero do we still want to do this? Or should we close it? |
|
Closing for now, re-open if necessary 😸 |
Use new target object tvm.target.Targe(), which now holds both 'target'
and 'target_host', in AutoTVM measurement functions. At some point the
tvm.driver.build() will be called by AutoTVM and it expects now such a
kind of target instead of the old (separate) one for 'target' and
'target_host'.
This avoids several deprecate warning messages caused by build() when
AutoTVM is used by tools like 'tvmc'.
Signed-off-by: Gustavo Romero gustavo.romero@linaro.org