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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to