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/12/29 11:05:53 UTC
[GitHub] [tvm] argrento opened a new pull request #9811: PackedFunction to return params from the .so module, show warning when no params are set
argrento opened a new pull request #9811:
URL: https://github.com/apache/tvm/pull/9811
- Issue
`debug_executor` workflow described in https://tvm.apache.org/docs/arch/debugger.html#how-to-use-debugger has strange behavior. When I used code from the step 4, the result of inference differs from time to time. After a short investigation, I found that the `params` are not set correctly. Code that I used:
```python
onnx_model = onnx.load("./resnet50-v2-7.onnx")
mod, params = relay.frontend.from_onnx(onnx_model, {'data': (1, 3, 224, 224)})
with tvm.transform.PassContext(opt_level=3):
lib = relay.build(mod, target="llvm", params=params)
lib_name = "lib.so"
lib.export_library(lib_name)
loaded_lib = tvm.runtime.load_module(lib_name)
m = graph_executor.create(loaded_lib["get_graph_json"](), loaded_lib, dev, dump_root="/tmp/tvmdbg")
m.set_input('data', tvm.nd.array(data.astype(dtype)))
m.set_input(**params)
m.run()
tvm_out = m.get_output(0, tvm.nd.empty(out_shape, dtype)).numpy()
- Solution
- Implementation of `get_graph_params` function, which allows to get params directly from the lib file. Thus a single line can be added to the debugger example code:
```python
lib = tvm.runtime.load_module("network.so")
params = lib['get_graph_params']() # <-----
m = graph_executor.create(lib["get_graph_json"](), lib, dev, dump_root="/tmp/tvmdbg")
# set inputs
m.set_input('data', tvm.nd.array(data.astype(dtype)))
m.set_input(**params)
# execute
m.run()
tvm_out = m.get_output(0, tvm.nd.empty(out_shape, dtype)).numpy()
```
After this the result of inference will be correct.
- As a additional mark, special warning was added. This warning will be shown when a developer tries to call `run()` before setting inputs and params,
--
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] jwfromm commented on a change in pull request #9811: PackedFunction to return params from the .so module, show warning when no params are set
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #9811:
URL: https://github.com/apache/tvm/pull/9811#discussion_r784151076
##########
File path: src/runtime/graph_executor/graph_executor_factory.cc
##########
@@ -57,6 +58,14 @@ PackedFunc GraphExecutorFactory::GetFunction(
return PackedFunc(
[sptr_to_self, this](TVMArgs args, TVMRetValue* rv) { *rv = this->graph_json_; });
+ } else if (name == "get_graph_params") {
Review comment:
Whats the motivation behind adding this? It does seem useful so I'm all for it but we should add some tests to make sure it works as expected.
##########
File path: src/runtime/graph_executor/graph_executor.cc
##########
@@ -56,6 +56,9 @@ inline size_t GetDataAlignment(const DLTensor& arr) {
* \brief Run all the operations one by one.
*/
void GraphExecutor::Run() {
+ if (!params_set_) {
+ DLOG(WARNING) << "Params are not set, result may be invalid.";
Review comment:
`params_set_` is only checking that the `set_input` method has been called at least once. This doesn't necessarily mean that all the parameters of the model have been set so this warning is a little misleading. I would recommend changing it to something like "No inputs have been provided, to get valid results call set_input before running."
--
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] junrushao1994 commented on pull request #9811: PackedFunction to return params from the .so module, show warning when no params are set
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9811:
URL: https://github.com/apache/tvm/pull/9811#issuecomment-1023538656
@jwfromm that makes sense to me. The checking itself, although i would say the overhead might be minimal, could take some time and cause regressions on small workloads. The rest of the PR looks pretty good BTW.
--
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] argrento commented on a change in pull request #9811: PackedFunction to return params from the .so module, show warning when no params are set
Posted by GitBox <gi...@apache.org>.
argrento commented on a change in pull request #9811:
URL: https://github.com/apache/tvm/pull/9811#discussion_r785907783
##########
File path: src/runtime/graph_executor/graph_executor_factory.cc
##########
@@ -57,6 +58,14 @@ PackedFunc GraphExecutorFactory::GetFunction(
return PackedFunc(
[sptr_to_self, this](TVMArgs args, TVMRetValue* rv) { *rv = this->graph_json_; });
+ } else if (name == "get_graph_params") {
Review comment:
This function will return params from the compiled module (.so binary). This is the only way to read them from the compiled module. Without this one will not be able to read and set them with `set_params`.
Which tests will you suggest to add?
--
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] jwfromm commented on pull request #9811: PackedFunction to return params from the .so module, show warning when no params are set
Posted by GitBox <gi...@apache.org>.
jwfromm commented on pull request #9811:
URL: https://github.com/apache/tvm/pull/9811#issuecomment-1023461773
After thinking about this PR a little more, I'm of the opinion that we probably should not add input checking to GraphExecutor::Run. It's important that we keep Run as minimal as possible, and the checking of inputs and more importantly printing of the warning may cause Run to be slower. I like the rest of this PR but think we should drop `input_set_`. What do you think @junrushao1994?
--
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] jwfromm merged pull request #9811: PackedFunction to return params from the .so module, show warning when no params are set
Posted by GitBox <gi...@apache.org>.
jwfromm merged pull request #9811:
URL: https://github.com/apache/tvm/pull/9811
--
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] argrento commented on pull request #9811: PackedFunction to return params from the .so module, show warning when no params are set
Posted by GitBox <gi...@apache.org>.
argrento commented on pull request #9811:
URL: https://github.com/apache/tvm/pull/9811#issuecomment-1024100624
Thank you for the comments! Currently I have 2 solutions:
1. Restore the original logic of Run function
2. Since current time complexity of Run is O(N^2), I can rewrite it and reduce complexity to, probably, O(1).
Which one do you prefer?
--
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] jwfromm commented on a change in pull request #9811: PackedFunction to return params from the .so module, show warning when no params are set
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #9811:
URL: https://github.com/apache/tvm/pull/9811#discussion_r787090528
##########
File path: src/runtime/graph_executor/graph_executor_factory.cc
##########
@@ -57,6 +58,14 @@ PackedFunc GraphExecutorFactory::GetFunction(
return PackedFunc(
[sptr_to_self, this](TVMArgs args, TVMRetValue* rv) { *rv = this->graph_json_; });
+ } else if (name == "get_graph_params") {
Review comment:
I think it'd be worth adding a test that you can take a compiled module, call `set_params` then `get_graph_params` (which we might want to rename to `get_params` for parity) and confirm the before and after match.
--
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