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/03/15 06:29:57 UTC
[GitHub] [tvm] akmaru opened a new pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
akmaru opened a new pull request #7658:
URL: https://github.com/apache/tvm/pull/7658
Adds TensorRT integration as a composite target into TVMC.
----------------------------------------------------------------
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] akmaru commented on a change in pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
akmaru commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r639177537
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
if codegen["config_key"] is not None:
- config[codegen["config_key"]] = codegen_from_cli["opts"]
+ config[codegen["config_key"]] = codegen_from_cli["opts"] or codegen_config
Review comment:
Sorry, I don't know it means.
The implementation before this change use all of `codegen_from_cli` as the config.
If should consider only uses a part of `partiton_for_xxx`, it's due to not this change but original implementation.
Or, should it apply also in this change?
--
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] akmaru commented on a change in pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
akmaru commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r639132253
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
if codegen["config_key"] is not None:
- config[codegen["config_key"]] = codegen_from_cli["opts"]
+ config[codegen["config_key"]] = codegen_from_cli["opts"] or codegen_config
Review comment:
The config returned by `partiton_for_tensorrt` reflects the user config by the`codegen_from_cli` argument.
And, the other composite_targets don't return config.
So, config with a `extra_codegen` can be overwritten codegen_config if any.
--
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 #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r639197069
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
if codegen["config_key"] is not None:
- config[codegen["config_key"]] = codegen_from_cli["opts"]
+ config[codegen["config_key"]] = codegen_from_cli["opts"] or codegen_config
Review comment:
Sorry I misinterpreted the meaning of `codegen_from_cli` (their names are sort of confusing...).
Since `codegen_from_cli` is given by each composite target, this way should be fine.
--
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] akmaru commented on pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
akmaru commented on pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#issuecomment-848211550
Sorry for my late fix, please review again!
--
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 #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r594205774
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
In the interest of standardizing all the `partition_for_*` functions, I think it would be good to do a little refactoring in the `partition_for_tensorrt` so that it only returns `mod`. This is important not only for TVMC, but I think it would be a good move in terms of future partitioning support in TVM.
There is some new functionality (yet to be merged) in #7577 to have an `init` function per target in tvmc, in which we can hen populate the configs using the existing `get_tensorrt_version`.
I suggest we coordinate to get #7577 landed and then adjust this to comply with the suggestions above, so that we can get a uniform support for all targets. What do you think?
(cc @comaniac)
----------------------------------------------------------------
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 #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r639146908
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
if codegen["config_key"] is not None:
- config[codegen["config_key"]] = codegen_from_cli["opts"]
+ config[codegen["config_key"]] = codegen_from_cli["opts"] or codegen_config
Review comment:
> The config returned by partiton_for_tensorrt reflects the user config by the codegen_from_cli argument.
This is not guaranteed. What if `partition_for_xxx` only uses a part of the configs in `codegen_from_cli`?
--
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] akmaru commented on a change in pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
akmaru commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r639133321
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
For now, I tried to receive and apply the config returned partiton_for_tensrort.
--
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 #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r643027119
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
Yeah, I agree this is simpler and effective from now. Happy to approve once we get the linting issues sorted out and CI passing here.
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
Yeah, I agree this is simpler and effective for now. Happy to approve once we get the linting issues sorted out and CI passing here.
--
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] masahi commented on pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#issuecomment-1008437596
@akmaru can you update?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a change in pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r594625451
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
Fair enough. I thought a bit more about it and as with #7577, we will be already sending a config dictionary for the partition function. so I think it is ok to get the same, or a modified version of that dictionary as a result, which fits with the existing support on TensorRT.
----------------------------------------------------------------
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] luohao123 commented on pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
luohao123 commented on pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#issuecomment-925635957
@akmaru Any updates on this?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r594519337
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
if codegen["config_key"] is not None:
- config[codegen["config_key"]] = codegen_from_cli["opts"]
+ config[codegen["config_key"]] = codegen_from_cli["opts"] or codegen_config
Review comment:
It means the generated codegen_config will be completely overwritten by the user config, but we should make it like
```python
codegen_config.update(codegen_from_cli["opts"] if codegen_from_cli["opts"] is not None else {})
```
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
I agree that it would be easier to ask all pritition functions return a single module. However, the init function introduced by #7577 seems not applicable to this case to me. The config required by TensorRT is used when building the module; while the init function used by Vitis-AI imports the required package. I have no idea how could we make it general enough to support both Vitis-AI and TensorRT.
On the other hand, let a partition function return a tuple of module and build config seems more straightforward at the first glance, as I cannot think of a better idea for now. First, partition functions won't be a Relay pass in a pass sequence (at least for now). Second, it is reasonable that the build config may need to be constructed during the partition. Separating them to two APIs may introduce overheads and be more confusing to non TVMC users, which are still majority in this 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] comaniac commented on a change in pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r595476422
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
That's our ultimate goal of the composite target, which integrates partition and other codegen specific functions. Once we have it, the partition logic could be completely hidden from not only users but also frontend interface.
----------------------------------------------------------------
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 #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r594205774
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
In the interest of standardizing all the `partition_for_*` functions, I think it would be good to do a little refactoring in the `partition_for_tensorrt` so that it only returns `mod`. This is important not only for TVMC, but I think it would be a good move in terms of future partitioning support in TVM.
There is some new functionality (yet to be merged) in #7577 to have an `init` function per target in tvmc, in which we can then populate the configs using the existing `get_tensorrt_version`.
I suggest we coordinate to get #7577 landed and then adjust this to comply with the suggestions above, so that we can get a uniform support for all targets. What do you think?
(cc @comaniac)
----------------------------------------------------------------
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] trevor-m commented on a change in pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r594555474
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
I agree with @comaniac, I don't see a good way to separate this yet. The parameters used for `parition_for_tensorrt` affect both partitioning and building the module.
----------------------------------------------------------------
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] trevor-m commented on a change in pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#discussion_r595473587
##########
File path: python/tvm/driver/tvmc/compiler.py
##########
@@ -196,9 +196,9 @@ def compile_model(
for codegen_from_cli in extra_targets:
codegen = composite_target.get_codegen_by_target(codegen_from_cli["name"])
partition_function = codegen["pass_pipeline"]
- mod = partition_function(mod, params)
+ mod, codegen_config = partition_function(mod, params)
Review comment:
I guess an alternative could be for the partitioning to be done within the pass context, and have the partition function set the config in the passcontext.
----------------------------------------------------------------
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 pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
leandron commented on pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#issuecomment-815572412
@akmaru just a heads-up that #7577 is now merged and we can proceed with the changes proposed here.
--
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 pull request #7658: [TVMC][TensorRT] Enable TensorRT target through TVMC
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #7658:
URL: https://github.com/apache/tvm/pull/7658#issuecomment-940174322
Gentle ping @akmaru please fix the CI
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org