Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Target] Add target host field for target specification #7462
[Target] Add target host field for target specification #7462
Changes from 10 commits
940aeab
3c57ce6
a969fae
0d5d6d8
c395606
4c3a344
a1cc2c2
a1cb881
2f75e6c
3671378
71746c5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious on how this plays with existing logic in
build()
atbuild_module.py
, which has atarget_host
parameter:tvm/python/tvm/relay/build_module.py
Line 197 in b7e0cfb
Does this mean we now have two ways of declaring
target_host
? If so, are planning to deprecate one of them?Also, in case the user declares both (doesn't make much sense, but can be done once this PR is meged), which one takes precedece, or, should we show an error? I'm referrin to something like
relay.build(target="cuda --host nvidia/jetson-nano", target_host="llvm"...)
cc @manupa-arm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is super important question, so I answered on the thread below: #7462 (comment). Thank you Leandro for bringing this up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the API, I would almost expect
Target(target_host, sub_target_0, sub_target_1, ...)
what would be the future path towards supporting multiple sub-targets? would there be a target_host object which contains all the sub-targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of sub-targets, I think it will be better to converge to composite targets instead, where we can specify a list of targets as
--devices
: https://github.com/apache/tvm/blob/main/src/target/target_kind.cc#L311-L313.We can help create a short cut to construct the composite kind, like adding a static function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of API, that would be very good indeed!
If internally we would convert that to represent the required
composite target
, with specific dictionary representing all the internal data structures, etc., that's implementation details the we care about, but the end-user, interested in compiling a model shouldn't.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@areusch as a follow-up to this, we've recently updated
tvmc
to accept something similar - in terms of user facing syntax to express "targets" for compilation - to what you present here.Check it out on #7304 for code and https://discuss.tvm.apache.org/t/byoc-partitioning-support-on-tvmc/8901 for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(putting this comment here, but this is not comment about this test, just wanted to pin it somewhere with a reference)
I noticed there are some syntax changes in this. If this becomes (with this PR) our preferred way to declare the "target to target host" dependency, should we also update the tutorials to reflect that?