Skip to content

[AutoTVM] Use new target object #10963

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.

Sign up for GitHub

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

Closed
gromero wants to merge 1 commit into from
Closed

[AutoTVM] Use new target object #10963

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions python/tvm/autotvm/measure/measure_methods.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
from tvm.contrib.popen_pool import PopenPoolExecutor
from tvm.driver import build
from tvm.error import TVMError
from tvm.ir.container import Map
from tvm.target import Target

from ..env import AutotvmGlobalScope
Expand Down Expand Up @@ -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."

@Mousius Mousius Apr 12, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add a unit test to check this assertion fires correctly if the wrong thing is passed?

@gromero gromero Apr 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

@Mousius Mousius Apr 13, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@gromero gromero Apr 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

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).

@gromero gromero Apr 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@areusch areusch Apr 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Mousius I spent some time looking to find the "highest-level" place we could put such an assert in the autotvm infrastructure. I believe it's here. would that be better? We could just then and there assert that we always get an instance of Target (which should be true, looking at the log decoder.

@Mousius Mousius May 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@areusch areusch May 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@Mousius Mousius May 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@areusch areusch May 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

target, _ = Target.check_and_update_host_consist(target, task.target_host)

@areusch areusch Apr 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@gromero gromero Apr 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

@Mousius Mousius Apr 13, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@Mousius Mousius Apr 13, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, if we're unsure of the behaviour of check_and_update_host_consist in different scenarios we should add tests to exercise it?

@areusch areusch Apr 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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.

@Mousius Mousius Apr 20, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 😸


with target:
s, args = task.instantiate(config)
Expand All @@ -520,7 +522,7 @@ def _build_func_common(measure_input, runtime=None, check_gpu=None, build_option
func = vta.build(s, args, target_host=task.target_host)
else:
with tvm.ir.transform.PassContext(config=opts):
func = build(s, args, target_host=task.target_host, runtime=runtime)
func = build(s, args, target=target, runtime=runtime)
return func, tuple((get_const_tuple(x.shape), x.dtype) for x in args)


Expand Down