[Target] Add target host field for target specification #7462

zxybazh · 2021-02-17T03:26:15Z

As mentioned, adding support for target specification with a host (can be two strings, two configs, or a single config with host field), like target("cuda", "llvm"). The field is currently called host, creating some conflicts with previous field name target_host in composite type of target. While the PR is backward compatible, a lot of legacy code should be changed into the new API for better readbility and simplicity.

zxybazh · 2021-02-17T03:47:26Z

@comaniac @leandron @junrushao1994 Please review, thanks!

include/tvm/target/target.h

junrushao

Thanks for your contribution! It overall looks pretty good. Please find my comments below :-)

python/tvm/target/target.py

src/target/target_kind.cc

src/target/target.cc

tests/python/unittest/test_target_target.py

comaniac

Also agree on the comments @junrushao1994 has.

include/tvm/target/target.h

junrushao · 2021-02-18T02:52:28Z

The PR overall looks good to me.

It does introduce a breaking change - renaming composite target’s target_host to host for simplification. Therefore, I would love to ask for @leandron to review on the change. Thanks a lot!

leandron

In general it looks OK, I'm just a bit worried it creates some ambiguity in declaring the "target host" dependency.

Below my questions and comments.

leandron · 2021-02-18T10:58:22Z

include/tvm/target/target_kind.h

+          .add_attr_option<Array<String>>("libs")                 \
+          .add_attr_option<Target>("host")


I'm just curious on how this plays with existing logic in build() at build_module.py, which has a target_host parameter:

tvm/python/tvm/relay/build_module.py

Line 197 in b7e0cfb

def build(mod, target=None, target_host=None, params=None, mod_name="default"):

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

It is super important question, so I answered on the thread below: #7462 (comment). Thank you Leandro for bringing this up!

src/target/target.cc

leandron · 2021-02-18T11:15:00Z

tests/python/unittest/test_target_target.py

+
+
+def test_target_host_single_string():
+    tgt = tvm.target.Target("cuda --host llvm")


(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?

junrushao · 2021-02-18T19:33:59Z

Hey @leandron, thank you for the review!

Per the Target RFC, this PR implement the logic that folds target_host into the target class, so that is why we added a host field in the TargetNode class.

Does this mean we now have two ways of declaring target_host?

Yes. We can create a target with a single dictionary containing the host field (the new canonical), or with two Targets combined (syntactic sugar I), or with a target string like cuda --host=llvm (syntactic sugar II).

This design is a solution to keep perfect backward compatibility. In your example, right now, the signature of the build API is:

def build(..., target, target_host...):

To be compatible with the existing API without breaking them, our refactoring plan is to create a single target_with_host = Target(target, target_host), and proceed with the subsequent workflow with target_with_host. This way could work with all the 4 possibilities:

target contains the host field, and target_host is None - we use target.host as target_with_host.host
target doesn't contain the host field, and target_host is None - target_with_host.host will be None
target contains the host field, and target_host is not None - we will error out
target doesn't contain the host field, and target_host is not None - target_with_host.host will be target_host

If so, are planning to deprecate one of them?

Since our solution keeps perfect backward compatibility, we are still deciding whether we really need deprecation at this time point. Even if we finally decided to deprecate the current API and move to a unified target in the function signature, I think we still need to keep it for 2 release cycles so that the community could really get onboard.

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?

Yeah. We are quite overwhelmed by the upcoming TensorIR upstreaming work. After that, I should really spend some time on doc improvement, etc.

areusch · 2021-02-18T20:19:40Z

python/tvm/target/target.py

@@ -46,7 +46,7 @@ class Target(Object):
    - :py:func:`tvm.target.intel_graphics` create Intel Graphics target
    """

-    def __init__(self, tag_or_str_or_dict):
+    def __init__(self, tag_or_str_or_dict, host_tag_or_str_or_dict=None):


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?

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

@staticmethod def composite(target, devices):

for the API, I would almost expect Target(target_host, sub_target_0, sub_target_1, ...)

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.

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

include/tvm/target/target.h

areusch · 2021-02-18T20:47:58Z

If so, are planning to deprecate one of them?

Since our solution keeps perfect backward compatibility, we are still deciding whether we really need deprecation at this time point. Even if we finally decided to deprecate the current API and move to a unified target in the function signature, I think we still need to keep it for 2 release cycles so that the community could really get onboard.

@junrushao1994 I think we should aim to just have one way to declare Targets. we don't have to remove the other way here, but I think it would be good to indicate the "preferred" way to new users by marking one way deprecated.

junrushao · 2021-02-18T21:16:41Z

@Areush we can warn-once if target_host is not None, but before doing that, perhaps we need consensus from the community

jroesch · 2021-02-19T02:11:05Z

If so, are planning to deprecate one of them?

Since our solution keeps perfect backward compatibility, we are still deciding whether we really need deprecation at this time point. Even if we finally decided to deprecate the current API and move to a unified target in the function signature, I think we still need to keep it for 2 release cycles so that the community could really get onboard.

@junrushao1994 I think we should aim to just have one way to declare Targets. we don't have to remove the other way here, but I think it would be good to indicate the "preferred" way to new users by marking one way deprecated.

I would actually go further and disagree, we should strive to eventually only have one way and push to delete everything but the preferred way. I feel like we use lots of extra arguments instead of composite objects and over all feel like we should move towards bundling commonly packaged state into single objects.

junrushao · 2021-02-19T02:29:17Z

@jroesch It is definitely good that eventually we have only one API in the core system, and to make it happen, here are the steps we planned to take:

in the next PR, we will allow only a single target in the core system, and we are able to make it happen without breaking user-facing API at all. Basically we treat the two-argument case as a syntactic sugar of the single-argument case. See my previous comment for details.

After this step, the core system does only have one API.

If we decided to change user-facing API, we need to send out a deprecation RFC to the community for broader agreement.
In the next release cycle, we keep that API, but prints a deprecation warning in python saying it is going to be removed in the next cycle.
In the next fo next release cycle, we finally remove target_host from the signature. Because the core system has been upgraded in step 1), the effort to remove the argument is minimal

junrushao · 2021-02-22T05:42:05Z

@leandron @areusch @comaniac @jroesch

All the tests have passed on this specific PR, please take another look and explicitly approve if it looks good :-)

I know there is a lot to discuss about potential future API deprecation, etc, which is beyond the scope of this PR. Let's move the discussion to the Target RFC (https://discuss.tvm.apache.org/t/rfc-tvm-target-specification/6844). Thanks a lot!

junrushao

LGTM

comaniac

LGTM for the scope of this PR.

areusch

LGTM, great work @zxybazh !

leandron

LGTM, as it keeps backwards compatibility, and going forward we can try to converge into a user friendly target API, building upon what's implemented here.

junrushao · 2021-02-22T19:10:51Z

@zxybazh would you mind raising API backward compatibility topic in the Target RFC (https://discuss.tvm.apache.org/t/rfc-tvm-target-specification/6844) thread, so that we can continue our discussion there and more people could see it? Thanks a lot!

junrushao · 2021-02-22T19:11:26Z

I am going to merge this PR later today if everyone thinks it is good to go :-)

junrushao · 2021-02-23T00:55:28Z

@zxybazh @leandron @areusch @comaniac @jroesch Thank you guys so much for the inspiring discussion! It is merged!

* Add target host field in Target * Add host as a config field to target * Add target host support for Python api * Add unit tests * Adjust format for cpplint * Remove unnecessary after in Python file * Remove redundancy and add param description * Fix format issue * Fix param description * Add unit test for duplicate target hosts

zxybazh added 6 commits February 10, 2021 15:10

Add target host field in Target

940aeab

Add host as a config field to target

3c57ce6

Add target host support for Python api

a969fae

Add unit tests

0d5d6d8

Merge unit tests

c395606

Adjust format for cpplint

4c3a344

Remove unnecessary after in Python file

a1cc2c2

junrushao reviewed Feb 17, 2021

View reviewed changes

include/tvm/target/target.h Outdated Show resolved Hide resolved

junrushao requested changes Feb 17, 2021

View reviewed changes

comaniac reviewed Feb 17, 2021

View reviewed changes

include/tvm/target/target.h Show resolved Hide resolved

zxybazh added 3 commits February 17, 2021 15:43

Remove redundancy and add param description

a1cb881

Fix format issue

2f75e6c

Fix param description

3671378

leandron reviewed Feb 18, 2021

View reviewed changes

areusch reviewed Feb 18, 2021

View reviewed changes

Add unit test for duplicate target hosts

71746c5

junrushao approved these changes Feb 22, 2021

View reviewed changes

comaniac approved these changes Feb 22, 2021

View reviewed changes

areusch approved these changes Feb 22, 2021

View reviewed changes

leandron approved these changes Feb 22, 2021

View reviewed changes

junrushao merged commit 794f6c6 into apache:main Feb 23, 2021

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Target] Add target host field for target specification #7462

[Target] Add target host field for target specification #7462

zxybazh commented Feb 17, 2021

zxybazh commented Feb 17, 2021

junrushao left a comment

comaniac left a comment

junrushao commented Feb 18, 2021

leandron left a comment

leandron Feb 18, 2021 •

edited

Loading

junrushao Feb 18, 2021

leandron Feb 18, 2021

junrushao commented Feb 18, 2021 •

edited

Loading

areusch Feb 18, 2021

junrushao Feb 18, 2021

leandron Feb 19, 2021

leandron Feb 22, 2021

areusch commented Feb 18, 2021

junrushao commented Feb 18, 2021

jroesch commented Feb 19, 2021

junrushao commented Feb 19, 2021 •

edited

Loading

junrushao commented Feb 22, 2021

junrushao left a comment

comaniac left a comment

areusch left a comment

leandron left a comment •

edited

Loading

junrushao commented Feb 22, 2021

junrushao commented Feb 22, 2021

junrushao commented Feb 23, 2021

		.add_attr_option<Array<String>>("libs") \
		.add_attr_option<Target>("host")



		def test_target_host_single_string():
		tgt = tvm.target.Target("cuda --host llvm")

[Target] Add target host field for target specification #7462

[Target] Add target host field for target specification #7462

Conversation

zxybazh commented Feb 17, 2021

zxybazh commented Feb 17, 2021

junrushao left a comment

Choose a reason for hiding this comment

comaniac left a comment

Choose a reason for hiding this comment

junrushao commented Feb 18, 2021

leandron left a comment

Choose a reason for hiding this comment

leandron Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

junrushao Feb 18, 2021

Choose a reason for hiding this comment

leandron Feb 18, 2021

Choose a reason for hiding this comment

junrushao commented Feb 18, 2021 • edited Loading

areusch Feb 18, 2021

Choose a reason for hiding this comment

junrushao Feb 18, 2021

Choose a reason for hiding this comment

leandron Feb 19, 2021

Choose a reason for hiding this comment

leandron Feb 22, 2021

Choose a reason for hiding this comment

areusch commented Feb 18, 2021

junrushao commented Feb 18, 2021

jroesch commented Feb 19, 2021

junrushao commented Feb 19, 2021 • edited Loading

junrushao commented Feb 22, 2021

junrushao left a comment

Choose a reason for hiding this comment

comaniac left a comment

Choose a reason for hiding this comment

areusch left a comment

Choose a reason for hiding this comment

leandron left a comment • edited Loading

Choose a reason for hiding this comment

junrushao commented Feb 22, 2021

junrushao commented Feb 22, 2021

junrushao commented Feb 23, 2021

leandron Feb 18, 2021 •

edited

Loading

junrushao commented Feb 18, 2021 •

edited

Loading

junrushao commented Feb 19, 2021 •

edited

Loading

leandron left a comment •

edited

Loading