Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PackedFunction to return params from the .so module, show warning when no params are set #9811

Merged
merged 12 commits into from
Feb 10, 2022
Merged

Conversation

argrento
Copy link
Contributor

  • 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:
    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:
      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,

@jwfromm
Copy link
Contributor

jwfromm commented Jan 27, 2022

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?

@junrushao
Copy link
Member

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

@argrento
Copy link
Contributor Author

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?

Copy link
Contributor

@jwfromm jwfromm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this change. I agree with your choice of solution 1 for now. This PR now looks ready to merge.

@jwfromm jwfromm merged commit 61b66cd into apache:main Feb 10, 2022
ylc pushed a commit to ylc/tvm that referenced this pull request Feb 16, 2022
…n no params are set (apache#9811)

* PackedFunction to return params from the .so module, show warning when no params are set

* Linter checkup

* Autoset of params, tests for get_graph_params

* Linter checkup

* Check that inputs were set before run

* Return the original implementation if Run

* Fix RPC behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants