[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5753: [Draft] Support Module based interface runtime

[GitBox <gi...@apache.org>]

tqchen edited a comment on pull request #5753:
URL: https://github.com/apache/incubator-tvm/pull/5753#issuecomment-654347387


   Thanks @FrozenGene  I made some initial comments.  Would like to followup on the general design directions. The PR as it is implements the features we want. However, it is equally important to think about the **minimalism**. 
   
   In particular, we want to implement the feature using a minimum set of concepts(APIs). The runtime module based interface is more like a interface convention instead of a common implementation that we use for packaging. We can imagine different kinds of implementations, GraphRuntimeFactory is one of them(for graph executions). We would also like as much de-coupling as possible.
   
   So the ley challenge is -- how can we implement the features using as a minimum set of API interface as possible.
   
   We can dissect the current API into two category of functionalities
   
   - F0: Load the module in, execute  
   - F1: Result of the relay.build, backward compatibility, write the module out.
   
   Notably, F0 and F1 does not have to use the same runtime.module implementations.
   
   ## Minimum Design for F0 
   
   If we focus on F0, we can find that we only need one interface for graph runtime in the C++ side(via Module API) -- the creation function:
   
   ```python
   from tvm.contrib import graph_runtime
   gmod = graph_runtime.GraphModule(mod['resnet18'](tvm.cpu(0)))
   ```
   
   Notably, in the use cases of F0, we **do not** need the GraphRuntimeFactory wrapper(as the wrapper itself is primarily for backward compatiblity reasons).
   
   ## Minimum Design for F1
   
   If we do not need to support the additional features(e.g. disable `package_params` or `get_params`). Then no additional API is needed.
   
   We would certainly need the `GraphFactoryModule` wrapper to hold the return value of relay.build. However, note that the wrapper is only needed for backward compatibility reason only. As a result, we do not need to place  `GraphFactoryModule` in the runtime folder, instead, we can just place it near under relay.backend, or close to `graph_runtime.py` for now. When we deprecate the old runtime API eventually, we could remove the python wrapper.
   
   ## Discussions
   
   From the above discussions, we can find that the only really necessary API is the factory creation function. We could certainly expose `get_params` so users can obtain the parameters.
   
   The current way of implementing `disable_params` should be simplified. First of all, we would prefer stateless classes as much as possible, so the API that switch a flag on and off is not really a good idea.
   
   One potential way to address the problem is to still use a compositional API:
   
   ```python
   mod = relay.build()
   mod_no_params = mod["remove_params"]()
   # no params will be exported
   mod_no_params.export_library("xyz.so")
   ```
   
   We can discuss more API naming choices. Another parallel thread is how to create a debug runtime(if available). In this case, we could simply do ```mod["debug_create"]("default", ctx.cpu(0))```.
   
   


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