You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/02/17 03:26:31 UTC
[GitHub] [tvm] zxybazh opened a new pull request #7462: [Target] Add target host field for target specification
zxybazh opened a new pull request #7462:
URL: https://github.com/apache/tvm/pull/7462
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r577818377
##########
File path: include/tvm/target/target.h
##########
@@ -44,6 +44,8 @@ class TargetNode : public Object {
public:
/*! \brief The kind of the target device */
TargetKind kind;
+ /*! \brief Target host information of the target device */
+ Optional<ObjectRef> host;
Review comment:
Should this be `Optional<Target>`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781006453
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!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r578324044
##########
File path: src/target/target.cc
##########
@@ -373,6 +373,15 @@ Target::Target(const Map<String, ObjectRef>& config) {
data_ = std::move(target);
}
+Target::Target(Target target, Target host) {
+ ObjectPtr<TargetNode> n = make_object<TargetNode>(*target.get());
+ CHECK(!n->host.defined())
+ << "ValueError: Adding a host to a target whose host field has been defined";
Review comment:
Please correct me if my question doesn't make sense, but am I correct to assume that this check will always pass, as you're adding the field directly to the macro ` TVM_REGISTER_TARGET_KIND`, which is used by all existing targets, and therefore `host` is a field for all of them?
##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -158,6 +155,61 @@ def test_list_kinds():
assert all(isinstance(target_name, str) for target_name in targets)
+def test_target_host_tags():
+ tgt = tvm.target.Target("nvidia/jetson-nano", "nvidia/geforce-rtx-2080-ti")
+ assert tgt.kind.name == "cuda"
+ assert tgt.attrs["arch"] == "sm_53"
+ assert tgt.attrs["shared_memory_per_block"] == 49152
+ assert tgt.attrs["max_threads_per_block"] == 1024
+ assert tgt.attrs["thread_warp_size"] == 32
+ assert tgt.attrs["registers_per_block"] == 32768
+ assert tgt.host.kind.name == "cuda"
+ assert tgt.host.attrs["arch"] == "sm_75"
+ assert tgt.host.attrs["shared_memory_per_block"] == 49152
+ assert tgt.host.attrs["max_threads_per_block"] == 1024
+ assert tgt.host.attrs["thread_warp_size"] == 32
+ assert tgt.host.attrs["registers_per_block"] == 65536
+
+
+def test_target_host_tag_dict():
+ tgt = tvm.target.Target("nvidia/jetson-nano", {"kind": "llvm"})
+ assert tgt.kind.name == "cuda"
+ assert tgt.attrs["arch"] == "sm_53"
+ assert tgt.attrs["shared_memory_per_block"] == 49152
+ assert tgt.attrs["max_threads_per_block"] == 1024
+ assert tgt.attrs["thread_warp_size"] == 32
+ assert tgt.attrs["registers_per_block"] == 32768
+ assert tgt.host.kind.name == "llvm"
+
+
+def test_target_host_single_dict():
+ tgt = tvm.target.Target({"kind": "llvm", "host": "nvidia/jetson-nano"})
+ assert tgt.kind.name == "llvm"
+ assert tgt.host.kind.name == "cuda"
+ assert tgt.host.attrs["arch"] == "sm_53"
+ assert tgt.host.attrs["shared_memory_per_block"] == 49152
+ assert tgt.host.attrs["max_threads_per_block"] == 1024
+ assert tgt.host.attrs["thread_warp_size"] == 32
+ assert tgt.host.attrs["registers_per_block"] == 32768
+
+
+def test_target_host_single_string():
+ tgt = tvm.target.Target("cuda --host llvm")
Review comment:
(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?
##########
File path: include/tvm/target/target_kind.h
##########
@@ -376,7 +376,8 @@ inline TargetKindRegEntry& TargetKindRegEntry::set_name() {
.add_attr_option<String>("tag") \
.add_attr_option<String>("device") \
.add_attr_option<String>("model") \
- .add_attr_option<Array<String>>("libs")
+ .add_attr_option<Array<String>>("libs") \
+ .add_attr_option<Target>("host")
Review comment:
I'm just curious on how this plays with existing logic in `build()` at `build_module.py`, which has a `target_host` parameter:
https://github.com/apache/tvm/blob/b7e0cfb6d469c3745ae2195908daadea9c64d87e/python/tvm/relay/build_module.py#L197
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"...)`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781624761
> > 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r577997935
##########
File path: include/tvm/target/target.h
##########
@@ -44,6 +44,8 @@ class TargetNode : public Object {
public:
/*! \brief The kind of the target device */
TargetKind kind;
+ /*! \brief Target host information of the target device */
+ Optional<ObjectRef> host;
Review comment:
Sure. Fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
zxybazh commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-780277361
@comaniac @leandron @junrushao1994 Please review, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-783788416
@zxybazh @leandron @areusch @comaniac @jroesch Thank you guys so much for the inspiring discussion! It is merged!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-783605837
I am going to merge this PR later today if everyone thinks it is good to go :-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 edited a comment on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781584977
Hey @leandron, thank you for the review!
Per the [Target RFC](https://discuss.tvm.apache.org/t/rfc-tvm-target-specification/6844), 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 `Target`s 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:
```python
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:
1) `target` contains the `host` field, and `target_host` is None - we use `target.host` as `target_with_host.host`
2) `target` doesn't contain the `host` field, and `target_host` is None - `target_with_host.host` will be None
3) `target` contains the `host` field, and `target_host` is not None - we will error out
4) `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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r577335256
##########
File path: python/tvm/target/target.py
##########
@@ -86,10 +86,24 @@ def __init__(self, tag_or_str_or_dict):
mfloat-abi : str (optional)
An llvm setting that is one of 'hard' or 'soft' indicating whether to use
hardware or software floating-point operations.
+ host : Union[str, Dict[str, Any]] (optional)
+ Description for target host. Can be recursive. Similar to tag_or_str_or_dict.
+ host_tag_or_str_or_dict : Union[str, Dict[str, Any]]
Review comment:
```suggestion
host_tag_or_str_or_dict : Optional[Union[str, Dict[str, Any]]]
```
##########
File path: python/tvm/target/target.py
##########
@@ -86,10 +86,24 @@ def __init__(self, tag_or_str_or_dict):
mfloat-abi : str (optional)
An llvm setting that is one of 'hard' or 'soft' indicating whether to use
hardware or software floating-point operations.
+ host : Union[str, Dict[str, Any]] (optional)
+ Description for target host. Can be recursive. Similar to tag_or_str_or_dict.
+ host_tag_or_str_or_dict : Union[str, Dict[str, Any]]
+ Similar to tag_or_str_or_dict but for target host. Can be one of a literal
+ target host string, a json string describing a configuration, or a dictionary of
+ configuration options. When using a dictionary or json string to configure target,
+ the possible values are same as tag_or_str_or_dict.
"""
if not isinstance(tag_or_str_or_dict, (dict, str, Target)):
raise ValueError("target has to be a string or dictionary.")
- self.__init_handle_by_constructor__(_ffi_api.Target, tag_or_str_or_dict)
+ if host_tag_or_str_or_dict is not None:
+ if not isinstance(host_tag_or_str_or_dict, (dict, str, Target)):
+ raise ValueError("target host has to be a string or dictionary.")
+ self.__init_handle_by_constructor__(
+ _ffi_api.Target, tag_or_str_or_dict, host_tag_or_str_or_dict
+ )
Review comment:
This trick could make our life easier:
```suggestion
self.__init_handle_by_constructor__(
_ffi_api.Target, Target(tag_or_str_or_dict), Target(host_tag_or_str_or_dict)
)
```
##########
File path: src/target/target.cc
##########
@@ -456,6 +478,11 @@ void TargetInternal::ConstructorDispatcher(TVMArgs args, TVMRetValue* rv) {
<< runtime::ArgTypeCode2Str(arg.type_code());
}
return;
+ } else if (args.num_args == 2) {
+ const auto &argt = args[0], &argh = args[1];
+ auto func = PackedFunc(ConstructorDispatcher);
+ *rv = Target(func(argt).AsObjectRef<Target>(), func(argh).AsObjectRef<Target>());
+ return;
}
LOG(FATAL) << "ValueError: Invalid number of arguments. Expect 1, but gets: " << args.num_args;
Review comment:
Perhaps we should improve this error message
##########
File path: src/target/target.cc
##########
@@ -456,6 +478,11 @@ void TargetInternal::ConstructorDispatcher(TVMArgs args, TVMRetValue* rv) {
<< runtime::ArgTypeCode2Str(arg.type_code());
}
return;
+ } else if (args.num_args == 2) {
+ const auto &argt = args[0], &argh = args[1];
+ auto func = PackedFunc(ConstructorDispatcher);
+ *rv = Target(func(argt).AsObjectRef<Target>(), func(argh).AsObjectRef<Target>());
Review comment:
We only care about the case that both arguments are "Target"
```suggestion
if (args[0]->IsObjectRef<Target>() && args[1]->IsObjectRef<Target>()) {
Target target = args[0];
Target host = args[1];
*rv = Target(target, host);
}
```
##########
File path: tests/python/unittest/test_target_target.py
##########
@@ -158,6 +155,44 @@ def test_list_kinds():
assert all(isinstance(target_name, str) for target_name in targets)
+def test_target_host_tags():
+ tgt = tvm.target.Target("nvidia/jetson-nano", "nvidia/geforce-rtx-2080-ti")
+ assert tgt.kind.name == "cuda"
+ assert tgt.attrs["arch"] == "sm_53"
+ assert tgt.attrs["shared_memory_per_block"] == 49152
+ assert tgt.attrs["max_threads_per_block"] == 1024
+ assert tgt.attrs["thread_warp_size"] == 32
+ assert tgt.attrs["registers_per_block"] == 32768
+ assert tgt.host.kind.name == "cuda"
+ assert tgt.host.attrs["arch"] == "sm_75"
+ assert tgt.host.attrs["shared_memory_per_block"] == 49152
+ assert tgt.host.attrs["max_threads_per_block"] == 1024
+ assert tgt.host.attrs["thread_warp_size"] == 32
+ assert tgt.host.attrs["registers_per_block"] == 65536
+
+
+def test_target_host_tag_dict():
Review comment:
Yeah I think the tests are written pretty well. Thanks for the effort!
Let's add a few more, like
`cuda --host llvm`
`cuda --host nvidia/jetson-nano`
##########
File path: src/target/target.cc
##########
@@ -373,6 +380,21 @@ Target::Target(const Map<String, ObjectRef>& config) {
data_ = std::move(target);
}
+Target::Target(const Map<String, ObjectRef>& config, const Map<String, ObjectRef>& host_config) {
+ ObjectPtr<TargetNode> n =
+ make_object<TargetNode>(*Target(Target(config), Target(host_config)).get());
+ data_ = std::move(n);
+}
+
+Target::Target(Target target, Target host) {
+ ObjectPtr<TargetNode> n = make_object<TargetNode>(*target.get());
+ if (n->host.defined())
+ throw dmlc::Error(": Error when adding target host to target already with host");
Review comment:
```suggestion
CHECK(!n->host.defined()) << "ValueError: Adding a host to a target whose host field has been defined"
```
##########
File path: src/target/target_kind.cc
##########
@@ -219,6 +219,7 @@ TVM_REGISTER_TARGET_KIND("llvm", kDLCPU)
.add_attr_option<Bool>("system-lib")
.add_attr_option<String>("runtime")
.add_attr_option<Bool>("link-params", Bool(false))
+ .add_attr_option<Target>("host")
Review comment:
No need to add them separately, you may add this line to the `TVM_REGISTER_TARGET_KIND ` macro
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 edited a comment on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781584977
Hey @leandron, thank you for the review!
Per the [Target RFC](https://discuss.tvm.apache.org/t/rfc-tvm-target-specification/6844), 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 `Target`s 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:
```python
def build(..., target, target_host...):
```
To be compatible with the existing API without changing them, our refactoring plan is to create a single `target_with_host = Target(target, target_host)`, and proceed with the subsequent workflow. This way could work with
1) `target` contains the `host` field, and `target_host` is None - we use `target.host` as `target_with_host.host`
2) `target` doesn't contain the `host` field, and `target_host` is None - `target_with_host.host` will be None
3) `target` contains the `host` field, and `target_host` is not None - we will error out
4) `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 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 edited a comment on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781770687
@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:
1) 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](https://github.com/apache/tvm/pull/7462#issuecomment-781584977) for details.
After this step, the **core** system does only have one API.
2) If we decided to change user-facing API, we need to send out a deprecation RFC to the community for broader agreement.
3) 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.
4) 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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781640070
@areush we can warn-once if `target_host` is not None, but before doing that, perhaps we need consensus from the community
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r578001635
##########
File path: include/tvm/target/target.h
##########
@@ -122,14 +125,28 @@ class Target : public ObjectRef {
TVM_DLL explicit Target(std::nullptr_t) { data_ = nullptr; }
/*!
* \brief Construct a Target given a string
- * \param tag_or_config_or_target_str the string to parse
+ * \param tag_or_config_or_target_str the string to parse for target
*/
TVM_DLL explicit Target(const String& tag_or_config_or_target_str);
+ /*!
+ * \brief Construct a Target given a string
+ * \param tag_or_config_or_target_str the string to parse for target
+ * \param host_tag_or_config_or_host_str the string to parse for target host
+ */
+ TVM_DLL explicit Target(const String& tag_or_config_or_target_str,
+ const String& host_tag_or_config_or_host_str);
/*!
* \brief Construct a Target using a JSON-like configuration
- * \param config The JSON-like configuration
+ * \param config The JSON-like configuration for target
*/
TVM_DLL explicit Target(const Map<String, ObjectRef>& config);
+ /*!
+ * \brief Construct a Target using a JSON-like configuration
+ * \param config The JSON-like configuration for target
+ * \param host_config The JSON-like configuration for target host
+ */
+ TVM_DLL explicit Target(const Map<String, ObjectRef>& config,
+ const Map<String, ObjectRef>& host_config);
Review comment:
That's right. I have removed the redundant code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jroesch commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781764791
> > > 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 merged pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #7462:
URL: https://github.com/apache/tvm/pull/7462
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r578719962
##########
File path: 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):
Review comment:
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?
##########
File path: include/tvm/target/target.h
##########
@@ -44,6 +44,8 @@ class TargetNode : public Object {
public:
/*! \brief The kind of the target device */
TargetKind kind;
+ /*! \brief Target host information of the target device */
+ Optional<ObjectRef> host;
Review comment:
could we reverse the order and forward-declare TargetNode?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781584977
Hey @leandron, thank you for the review!
Per the [Target RFC](https://discuss.tvm.apache.org/t/rfc-tvm-target-specification/6844), 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 `Target`s 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:
```python
def build(..., target, target_host...):
```
To be compatible with the existing API without changing them, our refactoring plan is to create a single `target_with_host = Target(target, target_host)`, and proceed with the subsequent workflow. This way could work with
1) `target` contains the `host` field, and `target_host` is None
2) `target` doesn't contain the `host` field, and `target_host` is None
3) `target` contains the `host` field, and `target_host` is not None - we will error out
4) `target` doesn't contain the `host` field, and `target_host` is None - the field will be None
> 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 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r580143157
##########
File path: 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):
Review comment:
@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 https://github.com/apache/tvm/pull/7304 for code and https://discuss.tvm.apache.org/t/byoc-partitioning-support-on-tvmc/8901 for discussion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-783100004
@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!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r577825541
##########
File path: include/tvm/target/target.h
##########
@@ -44,6 +44,8 @@ class TargetNode : public Object {
public:
/*! \brief The kind of the target device */
TargetKind kind;
+ /*! \brief Target host information of the target device */
+ Optional<ObjectRef> host;
Review comment:
It is not doable, because Target is not defined yet 😆
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r579047173
##########
File path: 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):
Review comment:
> 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r577334704
##########
File path: include/tvm/target/target.h
##########
@@ -122,14 +125,28 @@ class Target : public ObjectRef {
TVM_DLL explicit Target(std::nullptr_t) { data_ = nullptr; }
/*!
* \brief Construct a Target given a string
- * \param tag_or_config_or_target_str the string to parse
+ * \param tag_or_config_or_target_str the string to parse for target
*/
TVM_DLL explicit Target(const String& tag_or_config_or_target_str);
+ /*!
+ * \brief Construct a Target given a string
+ * \param tag_or_config_or_target_str the string to parse for target
+ * \param host_tag_or_config_or_host_str the string to parse for target host
+ */
+ TVM_DLL explicit Target(const String& tag_or_config_or_target_str,
+ const String& host_tag_or_config_or_host_str);
/*!
* \brief Construct a Target using a JSON-like configuration
- * \param config The JSON-like configuration
+ * \param config The JSON-like configuration for target
*/
TVM_DLL explicit Target(const Map<String, ObjectRef>& config);
+ /*!
+ * \brief Construct a Target using a JSON-like configuration
+ * \param config The JSON-like configuration for target
+ * \param host_config The JSON-like configuration for target host
+ */
+ TVM_DLL explicit Target(const Map<String, ObjectRef>& config,
+ const Map<String, ObjectRef>& host_config);
Review comment:
nit: No need to provide these two constructors - C++ users can assemble them up using Target(Target(dict_a), Target(dict_b)).
##########
File path: include/tvm/target/target.h
##########
@@ -122,14 +125,28 @@ class Target : public ObjectRef {
TVM_DLL explicit Target(std::nullptr_t) { data_ = nullptr; }
/*!
* \brief Construct a Target given a string
- * \param tag_or_config_or_target_str the string to parse
+ * \param tag_or_config_or_target_str the string to parse for target
*/
TVM_DLL explicit Target(const String& tag_or_config_or_target_str);
+ /*!
+ * \brief Construct a Target given a string
+ * \param tag_or_config_or_target_str the string to parse for target
+ * \param host_tag_or_config_or_host_str the string to parse for target host
+ */
+ TVM_DLL explicit Target(const String& tag_or_config_or_target_str,
+ const String& host_tag_or_config_or_host_str);
/*!
* \brief Construct a Target using a JSON-like configuration
- * \param config The JSON-like configuration
+ * \param config The JSON-like configuration for target
*/
TVM_DLL explicit Target(const Map<String, ObjectRef>& config);
+ /*!
+ * \brief Construct a Target using a JSON-like configuration
+ * \param config The JSON-like configuration for target
+ * \param host_config The JSON-like configuration for target host
+ */
+ TVM_DLL explicit Target(const Map<String, ObjectRef>& config,
+ const Map<String, ObjectRef>& host_config);
Review comment:
No need to provide these two constructors - C++ users can assemble them up using Target(Target(dict_a), Target(dict_b)).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-783605519
@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!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r578694654
##########
File path: include/tvm/target/target_kind.h
##########
@@ -376,7 +376,8 @@ inline TargetKindRegEntry& TargetKindRegEntry::set_name() {
.add_attr_option<String>("tag") \
.add_attr_option<String>("device") \
.add_attr_option<String>("model") \
- .add_attr_option<Array<String>>("libs")
+ .add_attr_option<Array<String>>("libs") \
+ .add_attr_option<Target>("host")
Review comment:
It is super important question, so I answered on the thread below: https://github.com/apache/tvm/pull/7462#issuecomment-781584977. Thank you Leandro for bringing this up!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#issuecomment-781770687
@jroesch It is definitely good that eventually we have only one API in the **core** system, and to make it happen, and here are the steps we planned to take:
1) 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](https://github.com/apache/tvm/pull/7462#issuecomment-781584977) for details.
After this step, the **core** system does only have one API.
2) If we decided to change user-facing API, we need to send out a deprecation RFC to the community for broader agreement.
3) 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.
4) 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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r578322231
##########
File path: include/tvm/target/target_kind.h
##########
@@ -376,7 +376,8 @@ inline TargetKindRegEntry& TargetKindRegEntry::set_name() {
.add_attr_option<String>("tag") \
.add_attr_option<String>("device") \
.add_attr_option<String>("model") \
- .add_attr_option<Array<String>>("libs")
+ .add_attr_option<Array<String>>("libs") \
+ .add_attr_option<Target>("host")
Review comment:
I'm just curious on how this plays with existing logic in `build()` at `build_module.py`, which has a `target_host` parameter:
https://github.com/apache/tvm/blob/b7e0cfb6d469c3745ae2195908daadea9c64d87e/python/tvm/relay/build_module.py#L197
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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxybazh commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
zxybazh commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r578882245
##########
File path: src/target/target.cc
##########
@@ -373,6 +373,15 @@ Target::Target(const Map<String, ObjectRef>& config) {
data_ = std::move(target);
}
+Target::Target(Target target, Target host) {
+ ObjectPtr<TargetNode> n = make_object<TargetNode>(*target.get());
+ CHECK(!n->host.defined())
+ << "ValueError: Adding a host to a target whose host field has been defined";
Review comment:
Hi, I created a new unit tests to demonstrate the ValueError.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r578749624
##########
File path: 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):
Review comment:
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
```python
@staticmethod
def composite(target, devices):
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #7462: [Target] Add target host field for target specification
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #7462:
URL: https://github.com/apache/tvm/pull/7462#discussion_r577849493
##########
File path: include/tvm/target/target.h
##########
@@ -44,6 +44,8 @@ class TargetNode : public Object {
public:
/*! \brief The kind of the target device */
TargetKind kind;
+ /*! \brief Target host information of the target device */
+ Optional<ObjectRef> host;
Review comment:
Ah...good point. I checked the constructor and it does check the host type to be `Target`, so we're good here. A nit might be improving the comment of `host` to explicitly say it must be a `Target`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org