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/04/21 21:22:48 UTC
[GitHub] [tvm] mehrdadh opened a new pull request #7903: [microTVM] Graph Executor Debugger: fix parameter dump and introduce two debugging mode
mehrdadh opened a new pull request #7903:
URL: https://github.com/apache/tvm/pull/7903
This PR:
- changes graph executor debugger to have two modes {timing, accuracy}. These two modes are different in a sense that in timing we measure the performance of each layer, however in accuracy we look for the correctness of each layer.
- debug_mode=timing does not dump parameter anymore since the parameter placeholders are override after executing the entire model.
- debug_mode=accuracy runs the model up to each layer and saves the output tensor.
--
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] mehrdadh commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r620697672
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -210,65 +210,42 @@ class GraphExecutorDebug : public GraphExecutor {
return static_cast<size_t>(nodes_[index].param.num_outputs);
}
- /*!
- * \brief Execute network to a specific node and return result.
- *
- * This method will do a partial run of the the graph
- * from begining upto the index-th node and return output of index-th node.
- * This is costly operation and suggest to use only for debug porpose.
- *
- * \param index: The index of the node.
- * \return Node output array.
- */
- Array<NDArray> DebugGetNodeOutput(int index) {
- ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- Array<NDArray> results;
-
- for (size_t i = 0; i <= static_cast<size_t>(index); i++) {
- if (op_execs_[i]) op_execs_[i]();
- }
- for (size_t j = 0; j < NodeGetNumOutputs(index); j++) {
- results.push_back(data_entry_[entry_id(index, j)].CopyTo({kDLCPU, 0}));
- }
- return results;
- }
-
/*!
* \brief Execute next node in the network.
*
* This method will execute next node assuming
* previous nodes has been executed and return output of index-th node.
*
- * \param index: The index of the node.
- * \param eid The Entry id of the op.
- * \return Node output array.
+ * \param node_ind: The index of the node.
+ * \param output_ind: The output index of this node.
+ * \return Node outputs array.
*/
- NDArray ExecuteNextNodeGetOutput(int index, int eid) {
- ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- if (op_execs_[index]) op_execs_[index]();
- return data_entry_[entry_id(index, eid)];
+ NDArray ExecuteNextNodeGetOutputs(int node_ind, int output_ind) {
+ ICHECK_LT(static_cast<size_t>(node_ind), op_execs_.size());
+ if (op_execs_[node_ind]) op_execs_[node_ind]();
Review comment:
thanks for the feedback!
I added a variable to keep track of last executed Op.
--
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] mehrdadh commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825044072
@elvin-nnov Thanks for your feedbacks!
I agree with your suggestion and I'm changing it to have two functions with the purposes that you mentioned.
Regrading the `debug_get_output` function, I was going to remove it since it has wrong output. However, it has been mentioned in multiple places to be used like [graph_executor](https://github.com/mehrdadh/tvm/blob/83baec30d4418ac1470c696097913aafbfc8ad48/python/tvm/contrib/graph_executor.py#L262).
wdyt?
--
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] mehrdadh commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-830669619
@areusch this PR finally passed CI. PTAL.
Thansk!
--
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] mehrdadh commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825114645
@elvin-nnov actually `debug_get_output` by itself doesn't execute the network and only returns the output of a tensor that was captured by `self. _get_output_by_layer()` and recorded in `self.debug_datum._output_tensor_list`.
So assuming we fix the _run_debug, then we can use `debug_get_output` function.
--
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] mehrdadh commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r618841255
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -170,22 +173,74 @@ def _create_debug_env(self, graph_json, device):
# init the debug dumping environment
self.debug_datum = debug_result.DebugResult(graph_json, self._dump_path)
+ def _execute_next_node(self, node_index, entry_id):
+ """Execute node assuming all previous nodes has been executed.
+ Return the output of this node.
+
+ Parameters
+ ----------
+ node_index : int
+ The node index
+
+ entry_id : int
+ The entry id.
+ """
+ out_tensor = self._execute_next_node_get_output(node_index, entry_id)
+ return array(out_tensor)
+
+ def _run_per_layer(self):
+ """Execute up to each node and each debug output will be
+ copied to the buffer.
+
+ """
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ num_outputs = self.debug_datum.get_graph_node_output_num(node)
+ for j in range(num_outputs):
+ logging.info("running: output=%d of node_name: %s", j, node["name"])
+ out_tensor = self._execute_next_node(i, j)
+ self.debug_datum._output_tensor_list.append(out_tensor)
+
+ def execute_node(self, node_name):
+ """Execute network up to this node.
+ Return the outputs of this node.
+
+ Parameters
+ ----------
+ node_index : str
+ The node name
+ """
+ node_index = None
+ selected_node = None
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ if node["name"] == node_name:
+ selected_node = node
+ node_index = i
+ break
+ if not node_index:
+ raise AttributeError("Node name is not valid.")
+
+ num_outputs = self.debug_datum.get_graph_node_output_num(selected_node)
+ output_tensors = []
+ for j in range(num_outputs):
+ out = self._execute_node_get_output(node_index, j)
+ out = array(out)
Review comment:
removed it!
--
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] mehrdadh edited a comment on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh edited a comment on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825114645
@elvin-nnov actually `debug_get_output` by itself doesn't execute the network and only returns the output of a tensor that was already captured by `self. _get_output_by_layer()` and recorded in `self.debug_datum._output_tensor_list`.
So assuming we fix the _run_debug, then we can use `debug_get_output` function.
--
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] elvin-nnov edited a comment on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
elvin-nnov edited a comment on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825181927
> @elvin-nnov actually `debug_get_output` by itself doesn't execute the network and only returns the output of a tensor that was already captured by `self. _get_output_by_layer()` and recorded in `self.debug_datum._output_tensor_list`.
> So assuming we fix the _run_debug, then we can use `debug_get_output` function.
here https://github.com/apache/tvm/blob/main/src/runtime/graph_executor/debug/graph_executor_debug.cc#L292 `debug_get_output` calls `DebugGetNodeOutput` and in its turn `DebugGetNodeOutput` executes required number of ops here https://github.com/apache/tvm/blob/main/src/runtime/graph_executor/debug/graph_executor_debug.cc#L225
--
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] elvin-nnov commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r619129976
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -282,17 +324,17 @@ class GraphExecutorDebug : public GraphExecutor {
PackedFunc GraphExecutorDebug::GetFunction(const std::string& name,
const ObjectPtr<Object>& sptr_to_self) {
// return member functions during query.
- if (name == "get_output_by_layer") {
+ if (name == "debug_get_node_output") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
- *rv = this->GetOutputByLayer(args[0], args[1]);
+ *rv = this->DebugGetNodeOutput(args[0]);
});
- } else if (name == "debug_get_output") {
Review comment:
if we delete `debug_get_output`, we need to care about this place as well https://github.com/apache/tvm/blob/main/jvm/core/src/main/java/org/apache/tvm/contrib/GraphModule.java#L52
--
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] areusch commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-831404881
thanks @mehrdadh @elvin-nnov @tkonolige , the PR is now merged!
--
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] tkonolige commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825075667
I agree with removing `debug_get_output`. It is essentially useless.
I can see two use-cases for getting output: you want output from a specific layer or you want the output from each layer. We can implement the latter by repeatedly running the former, but this may be inefficient. An incremental approach still has benefits when running over rpc or on an embedded system.
I think we should use this incremental approach, but I would like the interface to get all outputs to also be available from c++. i.e. can we put as little of the interface in python as possible?
--
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] mehrdadh commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r620555111
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -282,17 +324,17 @@ class GraphExecutorDebug : public GraphExecutor {
PackedFunc GraphExecutorDebug::GetFunction(const std::string& name,
const ObjectPtr<Object>& sptr_to_self) {
// return member functions during query.
- if (name == "get_output_by_layer") {
+ if (name == "debug_get_node_output") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
- *rv = this->GetOutputByLayer(args[0], args[1]);
+ *rv = this->DebugGetNodeOutput(args[0]);
});
- } else if (name == "debug_get_output") {
Review comment:
I revert it so we can keep the java API.
Thanks!
--
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] elvin-nnov commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r619129976
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -282,17 +324,17 @@ class GraphExecutorDebug : public GraphExecutor {
PackedFunc GraphExecutorDebug::GetFunction(const std::string& name,
const ObjectPtr<Object>& sptr_to_self) {
// return member functions during query.
- if (name == "get_output_by_layer") {
+ if (name == "debug_get_node_output") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
- *rv = this->GetOutputByLayer(args[0], args[1]);
+ *rv = this->DebugGetNodeOutput(args[0]);
});
- } else if (name == "debug_get_output") {
Review comment:
if we delete `debug_get_output`, we need to care about this place as well https://github.com/apache/tvm/blob/main/jvm/core/src/main/java/org/apache/tvm/contrib/GraphModule.java#L52
--
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] areusch commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r619492054
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -208,25 +201,74 @@ class GraphExecutorDebug : public GraphExecutor {
}
/*!
- * \brief Copy index-th node to data_out.
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute network to a specific node and return result.
*
* This method will do a partial run of the the graph
* from begining upto the index-th node and return output of index-th node.
* This is costly operation and suggest to use only for debug porpose.
*
- * \param index: The index of the node.
- * \param data_out the node data.
+ * \param index: The index of the node.
+ * \return Node output array.
*/
- void DebugGetNodeOutput(int index, DLTensor* data_out) {
+ Array<NDArray> DebugGetNodeOutput(int index) {
ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- uint32_t eid = index;
+ Array<NDArray> results;
- for (size_t i = 0; i < op_execs_.size(); ++i) {
+ for (size_t i = 0; i <= static_cast<size_t>(index); i++) {
if (op_execs_[i]) op_execs_[i]();
- if (static_cast<int>(i) == index) break;
}
+ for (size_t j = 0; j < NodeGetNumOutputs(index); j++) {
+ results.push_back(data_entry_[entry_id(index, j)].CopyTo({kDLCPU, 0}));
+ }
+ return results;
+ }
+
+ /*!
+ * \brief Execute next node in the network.
+ *
+ * This method will execute next node assuming
+ * previous nodes has been executed and return output of index-th node.
+ *
+ * \param index: The index of the node.
+ * \param eid The Entry id of the op.
Review comment:
e.g. we shouldn't need to call `op_execs_[index]()` for each `eid` from the same `index`, I think?
--
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] tkonolige commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r618826620
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -170,22 +173,74 @@ def _create_debug_env(self, graph_json, device):
# init the debug dumping environment
self.debug_datum = debug_result.DebugResult(graph_json, self._dump_path)
+ def _execute_next_node(self, node_index, entry_id):
+ """Execute node assuming all previous nodes has been executed.
+ Return the output of this node.
+
+ Parameters
+ ----------
+ node_index : int
+ The node index
+
+ entry_id : int
+ The entry id.
+ """
+ out_tensor = self._execute_next_node_get_output(node_index, entry_id)
+ return array(out_tensor)
+
+ def _run_per_layer(self):
+ """Execute up to each node and each debug output will be
+ copied to the buffer.
+
+ """
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ num_outputs = self.debug_datum.get_graph_node_output_num(node)
+ for j in range(num_outputs):
+ logging.info("running: output=%d of node_name: %s", j, node["name"])
+ out_tensor = self._execute_next_node(i, j)
+ self.debug_datum._output_tensor_list.append(out_tensor)
+
+ def execute_node(self, node_name):
+ """Execute network up to this node.
+ Return the outputs of this node.
+
+ Parameters
+ ----------
+ node_index : str
Review comment:
```suggestion
node_name : str
```
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -170,22 +173,74 @@ def _create_debug_env(self, graph_json, device):
# init the debug dumping environment
self.debug_datum = debug_result.DebugResult(graph_json, self._dump_path)
+ def _execute_next_node(self, node_index, entry_id):
+ """Execute node assuming all previous nodes has been executed.
+ Return the output of this node.
+
+ Parameters
+ ----------
+ node_index : int
+ The node index
+
+ entry_id : int
+ The entry id.
+ """
+ out_tensor = self._execute_next_node_get_output(node_index, entry_id)
+ return array(out_tensor)
+
+ def _run_per_layer(self):
+ """Execute up to each node and each debug output will be
+ copied to the buffer.
+
+ """
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ num_outputs = self.debug_datum.get_graph_node_output_num(node)
+ for j in range(num_outputs):
+ logging.info("running: output=%d of node_name: %s", j, node["name"])
+ out_tensor = self._execute_next_node(i, j)
+ self.debug_datum._output_tensor_list.append(out_tensor)
+
+ def execute_node(self, node_name):
+ """Execute network up to this node.
+ Return the outputs of this node.
+
+ Parameters
+ ----------
+ node_index : str
+ The node name
+ """
+ node_index = None
+ selected_node = None
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ if node["name"] == node_name:
+ selected_node = node
+ node_index = i
+ break
+ if not node_index:
+ raise AttributeError("Node name is not valid.")
Review comment:
```suggestion
raise AttributeError(f"Could not find a node named {node_name} in this graph.")
```
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -170,22 +173,74 @@ def _create_debug_env(self, graph_json, device):
# init the debug dumping environment
self.debug_datum = debug_result.DebugResult(graph_json, self._dump_path)
+ def _execute_next_node(self, node_index, entry_id):
+ """Execute node assuming all previous nodes has been executed.
+ Return the output of this node.
+
+ Parameters
+ ----------
+ node_index : int
+ The node index
+
+ entry_id : int
+ The entry id.
+ """
+ out_tensor = self._execute_next_node_get_output(node_index, entry_id)
+ return array(out_tensor)
+
+ def _run_per_layer(self):
+ """Execute up to each node and each debug output will be
+ copied to the buffer.
+
+ """
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ num_outputs = self.debug_datum.get_graph_node_output_num(node)
+ for j in range(num_outputs):
+ logging.info("running: output=%d of node_name: %s", j, node["name"])
+ out_tensor = self._execute_next_node(i, j)
+ self.debug_datum._output_tensor_list.append(out_tensor)
+
+ def execute_node(self, node_name):
+ """Execute network up to this node.
+ Return the outputs of this node.
+
+ Parameters
+ ----------
+ node_index : str
+ The node name
+ """
+ node_index = None
+ selected_node = None
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ if node["name"] == node_name:
+ selected_node = node
+ node_index = i
+ break
+ if not node_index:
+ raise AttributeError("Node name is not valid.")
+
+ num_outputs = self.debug_datum.get_graph_node_output_num(selected_node)
+ output_tensors = []
+ for j in range(num_outputs):
+ out = self._execute_node_get_output(node_index, j)
Review comment:
`_execute_node_get_output` should return all of the outputs instead of making you index into them. This way we can avoid re-executing for each output.
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -208,25 +201,70 @@ class GraphExecutorDebug : public GraphExecutor {
}
/*!
- * \brief Copy index-th node to data_out.
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t GetNodeNumOfOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute network to a specific node and return result.
*
* This method will do a partial run of the the graph
* from begining upto the index-th node and return output of index-th node.
* This is costly operation and suggest to use only for debug porpose.
*
- * \param index: The index of the node.
- * \param data_out the node data.
+ * \param index: The index of the node.
+ * \param eid The Entry id of the op.
+ * \return Node output array.
*/
- void DebugGetNodeOutput(int index, DLTensor* data_out) {
+ NDArray ExecuteNodeGetOutput(int index, int eid) {
ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- uint32_t eid = index;
- for (size_t i = 0; i < op_execs_.size(); ++i) {
+ for (size_t i = 0; i <= static_cast<size_t>(index); i++) {
if (op_execs_[i]) op_execs_[i]();
- if (static_cast<int>(i) == index) break;
}
- data_entry_[eid].CopyTo(data_out);
+ return data_entry_[entry_id(index, eid)];
+ }
+
+ /*!
+ * \brief Execute next node in the network.
+ *
+ * This method will execute next node assuming
+ * previous nodes has been executed and return output of index-th node.
+ *
+ * \param index: The index of the node.
+ * \param eid The Entry id of the op.
+ * \return Node output array.
+ */
+ NDArray ExecuteNextNodeGetOutput(int index, int eid) {
+ ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
+ if (op_execs_[index]) op_execs_[index]();
+ return data_entry_[entry_id(index, eid)];
+ }
+
+ /*!
+ * \brief Execute full network and store each layer outputs.
+ *
+ * This method will execute a full network and store layer
+ * outputs along execution.
+ *
+ * \return All node outputs.
+ */
+ Array<NDArray> RunAndGetLayersOutputs() {
Review comment:
This function should return an `Array<Array<NDArray>>` where the outer node corresponds to the layer and the inner array is that layers outputs.
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -170,22 +173,74 @@ def _create_debug_env(self, graph_json, device):
# init the debug dumping environment
self.debug_datum = debug_result.DebugResult(graph_json, self._dump_path)
+ def _execute_next_node(self, node_index, entry_id):
+ """Execute node assuming all previous nodes has been executed.
+ Return the output of this node.
+
+ Parameters
+ ----------
+ node_index : int
+ The node index
+
+ entry_id : int
+ The entry id.
+ """
+ out_tensor = self._execute_next_node_get_output(node_index, entry_id)
+ return array(out_tensor)
+
+ def _run_per_layer(self):
+ """Execute up to each node and each debug output will be
+ copied to the buffer.
+
+ """
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ num_outputs = self.debug_datum.get_graph_node_output_num(node)
+ for j in range(num_outputs):
+ logging.info("running: output=%d of node_name: %s", j, node["name"])
+ out_tensor = self._execute_next_node(i, j)
+ self.debug_datum._output_tensor_list.append(out_tensor)
+
+ def execute_node(self, node_name):
+ """Execute network up to this node.
+ Return the outputs of this node.
+
+ Parameters
+ ----------
+ node_index : str
+ The node name
+ """
+ node_index = None
+ selected_node = None
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ if node["name"] == node_name:
+ selected_node = node
+ node_index = i
+ break
+ if not node_index:
+ raise AttributeError("Node name is not valid.")
+
+ num_outputs = self.debug_datum.get_graph_node_output_num(selected_node)
+ output_tensors = []
+ for j in range(num_outputs):
+ out = self._execute_node_get_output(node_index, j)
+ out = array(out)
+ output_tensors.append(out)
+ return output_tensors
+
def _run_debug(self):
"""Execute the node specified with index will be executed.
Each debug output will be copied to the buffer
Time consumed for each execution will be set as debug output.
"""
+ # Get timing.
self.debug_datum._time_list = [[float(t)] for t in self.run_individual(10, 1, 1)]
- for i, node in enumerate(self.debug_datum.get_graph_nodes()):
- num_outputs = self.debug_datum.get_graph_node_output_num(node)
- for j in range(num_outputs):
- out_tensor = self._get_output_by_layer(i, j)
- out_tensor = array(out_tensor)
- self.debug_datum._output_tensor_list.append(out_tensor)
+
+ # Get outputs.
+ self._run_per_layer()
def debug_get_output(self, node, out=None):
Review comment:
Let's replace `debug_get_output` with `execute_node`. (Rename `execute_node` to `debug_get_output`).
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -170,22 +173,74 @@ def _create_debug_env(self, graph_json, device):
# init the debug dumping environment
self.debug_datum = debug_result.DebugResult(graph_json, self._dump_path)
+ def _execute_next_node(self, node_index, entry_id):
+ """Execute node assuming all previous nodes has been executed.
+ Return the output of this node.
+
+ Parameters
+ ----------
+ node_index : int
+ The node index
+
+ entry_id : int
+ The entry id.
+ """
+ out_tensor = self._execute_next_node_get_output(node_index, entry_id)
+ return array(out_tensor)
+
+ def _run_per_layer(self):
+ """Execute up to each node and each debug output will be
+ copied to the buffer.
+
+ """
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ num_outputs = self.debug_datum.get_graph_node_output_num(node)
+ for j in range(num_outputs):
+ logging.info("running: output=%d of node_name: %s", j, node["name"])
+ out_tensor = self._execute_next_node(i, j)
+ self.debug_datum._output_tensor_list.append(out_tensor)
+
+ def execute_node(self, node_name):
+ """Execute network up to this node.
+ Return the outputs of this node.
+
+ Parameters
+ ----------
+ node_index : str
+ The node name
+ """
+ node_index = None
+ selected_node = None
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ if node["name"] == node_name:
+ selected_node = node
+ node_index = i
+ break
+ if not node_index:
+ raise AttributeError("Node name is not valid.")
+
+ num_outputs = self.debug_datum.get_graph_node_output_num(selected_node)
+ output_tensors = []
+ for j in range(num_outputs):
+ out = self._execute_node_get_output(node_index, j)
+ out = array(out)
Review comment:
Is this necessary?
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -208,25 +201,70 @@ class GraphExecutorDebug : public GraphExecutor {
}
/*!
- * \brief Copy index-th node to data_out.
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t GetNodeNumOfOutputs(size_t index) {
Review comment:
```suggestion
size_t NodeGetNumOutputs(size_t index) {
```
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -170,22 +173,74 @@ def _create_debug_env(self, graph_json, device):
# init the debug dumping environment
self.debug_datum = debug_result.DebugResult(graph_json, self._dump_path)
+ def _execute_next_node(self, node_index, entry_id):
+ """Execute node assuming all previous nodes has been executed.
+ Return the output of this node.
+
+ Parameters
+ ----------
+ node_index : int
+ The node index
+
+ entry_id : int
+ The entry id.
+ """
+ out_tensor = self._execute_next_node_get_output(node_index, entry_id)
+ return array(out_tensor)
+
+ def _run_per_layer(self):
+ """Execute up to each node and each debug output will be
+ copied to the buffer.
+
+ """
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ num_outputs = self.debug_datum.get_graph_node_output_num(node)
+ for j in range(num_outputs):
+ logging.info("running: output=%d of node_name: %s", j, node["name"])
+ out_tensor = self._execute_next_node(i, j)
+ self.debug_datum._output_tensor_list.append(out_tensor)
+
+ def execute_node(self, node_name):
Review comment:
Document return value.
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -208,25 +201,70 @@ class GraphExecutorDebug : public GraphExecutor {
}
/*!
- * \brief Copy index-th node to data_out.
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t GetNodeNumOfOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute network to a specific node and return result.
*
* This method will do a partial run of the the graph
* from begining upto the index-th node and return output of index-th node.
* This is costly operation and suggest to use only for debug porpose.
*
- * \param index: The index of the node.
- * \param data_out the node data.
+ * \param index: The index of the node.
+ * \param eid The Entry id of the op.
+ * \return Node output array.
*/
- void DebugGetNodeOutput(int index, DLTensor* data_out) {
+ NDArray ExecuteNodeGetOutput(int index, int eid) {
Review comment:
```suggestion
NDArray DebugGetNodeOutput(int index, int eid) {
```
--
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] mehrdadh commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r621406524
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -207,6 +200,42 @@ class GraphExecutorDebug : public GraphExecutor {
return -1;
}
+ /*!
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
Review comment:
removed it. thanks!
--
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] mehrdadh commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r621408161
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -207,6 +200,42 @@ class GraphExecutorDebug : public GraphExecutor {
return -1;
}
+ /*!
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute next node in the network.
+ *
+ * This method will execute next node assuming
+ * previous nodes has been executed and return output of index-th node.
+ *
+ * \param node_ind: The index of the node.
+ * \param output_ind: The output index of this node.
+ * \return Node outputs array.
+ */
+ NDArray ExecuteNextNodeGetOutputs(int node_ind, int output_ind) {
+ ICHECK_LT(static_cast<size_t>(node_ind), op_execs_.size());
+
+ // Reset execution.
+ if (node_ind < 0) {
+ last_executed_node_ = -1;
+ return NDArray();
+ }
+
+ if (node_ind > last_executed_node_) {
Review comment:
thanks for the feedbacks!
I divided this PackedFunc to two functions as @areusch suggested.
--
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] elvin-nnov commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r618636277
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -110,7 +110,7 @@ tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
}
}
-uint8_t memory[512 * 1024];
+uint8_t memory[1024 * 1024];
Review comment:
why 1024 is good enough? Is it just a fix to make some use case not crash or it is some reasons that solves issue for any use case?
--
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] mehrdadh commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825303698
@areusch PTAL.
Thanks!
--
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] mehrdadh commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r619518394
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -208,25 +201,74 @@ class GraphExecutorDebug : public GraphExecutor {
}
/*!
- * \brief Copy index-th node to data_out.
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute network to a specific node and return result.
*
* This method will do a partial run of the the graph
* from begining upto the index-th node and return output of index-th node.
* This is costly operation and suggest to use only for debug porpose.
*
- * \param index: The index of the node.
- * \param data_out the node data.
+ * \param index: The index of the node.
+ * \return Node output array.
*/
- void DebugGetNodeOutput(int index, DLTensor* data_out) {
+ Array<NDArray> DebugGetNodeOutput(int index) {
ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- uint32_t eid = index;
+ Array<NDArray> results;
- for (size_t i = 0; i < op_execs_.size(); ++i) {
+ for (size_t i = 0; i <= static_cast<size_t>(index); i++) {
if (op_execs_[i]) op_execs_[i]();
- if (static_cast<int>(i) == index) break;
}
+ for (size_t j = 0; j < NodeGetNumOutputs(index); j++) {
+ results.push_back(data_entry_[entry_id(index, j)].CopyTo({kDLCPU, 0}));
+ }
+ return results;
+ }
+
+ /*!
+ * \brief Execute next node in the network.
+ *
+ * This method will execute next node assuming
+ * previous nodes has been executed and return output of index-th node.
+ *
+ * \param index: The index of the node.
+ * \param eid The Entry id of the op.
Review comment:
yeah that makes sense. So one way to fix it is to just send node_index and return all outputs which runs op_execs_[index]() only once.
--
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] areusch commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r619374951
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -282,17 +324,17 @@ class GraphExecutorDebug : public GraphExecutor {
PackedFunc GraphExecutorDebug::GetFunction(const std::string& name,
const ObjectPtr<Object>& sptr_to_self) {
// return member functions during query.
- if (name == "get_output_by_layer") {
+ if (name == "debug_get_node_output") {
return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
- *rv = this->GetOutputByLayer(args[0], args[1]);
+ *rv = this->DebugGetNodeOutput(args[0]);
});
- } else if (name == "debug_get_output") {
+ } else if (name == "exectue_next_node_get_output") {
Review comment:
nit: execute
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -208,25 +201,74 @@ class GraphExecutorDebug : public GraphExecutor {
}
/*!
- * \brief Copy index-th node to data_out.
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute network to a specific node and return result.
*
* This method will do a partial run of the the graph
* from begining upto the index-th node and return output of index-th node.
* This is costly operation and suggest to use only for debug porpose.
*
- * \param index: The index of the node.
- * \param data_out the node data.
+ * \param index: The index of the node.
+ * \return Node output array.
*/
- void DebugGetNodeOutput(int index, DLTensor* data_out) {
+ Array<NDArray> DebugGetNodeOutput(int index) {
ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- uint32_t eid = index;
+ Array<NDArray> results;
- for (size_t i = 0; i < op_execs_.size(); ++i) {
+ for (size_t i = 0; i <= static_cast<size_t>(index); i++) {
if (op_execs_[i]) op_execs_[i]();
- if (static_cast<int>(i) == index) break;
}
+ for (size_t j = 0; j < NodeGetNumOutputs(index); j++) {
+ results.push_back(data_entry_[entry_id(index, j)].CopyTo({kDLCPU, 0}));
+ }
+ return results;
+ }
+
+ /*!
+ * \brief Execute next node in the network.
+ *
+ * This method will execute next node assuming
+ * previous nodes has been executed and return output of index-th node.
+ *
+ * \param index: The index of the node.
+ * \param eid The Entry id of the op.
Review comment:
this isn't the entry_id, it's the index of the output. i think it'd be great to separate execute from get-output for nodes which output tuples
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -170,19 +173,44 @@ def _create_debug_env(self, graph_json, device):
# init the debug dumping environment
self.debug_datum = debug_result.DebugResult(graph_json, self._dump_path)
+ def _execute_next_node(self, node_index, entry_id):
+ """Execute node assuming all previous nodes has been executed.
+ Return the output of this node.
+
+ Parameters
+ ----------
+ node_index : int
+ The node index
+
+ entry_id : int
+ The entry id.
+ """
+ out_tensor = self._execute_next_node_get_output(node_index, entry_id)
+ return array(out_tensor)
+
+ def _run_per_layer(self):
+ """Execute up to each node and each debug output will be
+ copied to the buffer.
+
+ """
+ for i, node in enumerate(self.debug_datum.get_graph_nodes()):
+ num_outputs = self.debug_datum.get_graph_node_output_num(node)
+ for j in range(num_outputs):
+ logging.info("running: output=%d of node_name: %s", j, node["name"])
+ out_tensor = self._execute_next_node(i, j)
+ self.debug_datum._output_tensor_list.append(out_tensor)
Review comment:
could you make this a function on debug_datum? I don't like us appending to private class members here.
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -231,6 +258,22 @@ def run(self, **input_dict):
# Step 4. Display the collected information
self.debug_datum.display_debug_result()
+ def get_per_layer_outputs(self, **input_dict):
Review comment:
can we just fit the modifications into run()?
##########
File path: python/tvm/contrib/debugger/debug_executor.py
##########
@@ -196,20 +224,19 @@ def debug_get_output(self, node, out=None):
The output array container
"""
if isinstance(node, str):
- output_tensors = self.debug_datum.get_output_tensors()
- try:
- out = output_tensors[node]
- except KeyError:
- node_list = output_tensors.keys()
- raise RuntimeError(
- "Node " + node + " not found, available nodes are: " + str(node_list) + "."
- )
+ node_index = None
+ for i, graph_node in enumerate(self.debug_datum.get_graph_nodes()):
+ if graph_node["name"] == node:
+ node_index = i
+ break
+ if not node_index:
Review comment:
you can use `else:` here instead (matches for-loop, not the inner if).
--
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] mehrdadh commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r619411388
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -208,25 +201,74 @@ class GraphExecutorDebug : public GraphExecutor {
}
/*!
- * \brief Copy index-th node to data_out.
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute network to a specific node and return result.
*
* This method will do a partial run of the the graph
* from begining upto the index-th node and return output of index-th node.
* This is costly operation and suggest to use only for debug porpose.
*
- * \param index: The index of the node.
- * \param data_out the node data.
+ * \param index: The index of the node.
+ * \return Node output array.
*/
- void DebugGetNodeOutput(int index, DLTensor* data_out) {
+ Array<NDArray> DebugGetNodeOutput(int index) {
ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- uint32_t eid = index;
+ Array<NDArray> results;
- for (size_t i = 0; i < op_execs_.size(); ++i) {
+ for (size_t i = 0; i <= static_cast<size_t>(index); i++) {
if (op_execs_[i]) op_execs_[i]();
- if (static_cast<int>(i) == index) break;
}
+ for (size_t j = 0; j < NodeGetNumOutputs(index); j++) {
+ results.push_back(data_entry_[entry_id(index, j)].CopyTo({kDLCPU, 0}));
+ }
+ return results;
+ }
+
+ /*!
+ * \brief Execute next node in the network.
+ *
+ * This method will execute next node assuming
+ * previous nodes has been executed and return output of index-th node.
+ *
+ * \param index: The index of the node.
+ * \param eid The Entry id of the op.
Review comment:
how do you suggest to separate it?
I think this API actually aligns with the first goal. Because it only transfers one output at a time.
--
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] elvin-nnov commented on a change in pull request #7903: [Graph Executor Debugger] fix parameter dump and introduce two debugging modes
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r618237281
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -229,6 +229,28 @@ class GraphExecutorDebug : public GraphExecutor {
data_entry_[eid].CopyTo(data_out);
}
+ /*!
+ * \brief Run debugger to a specific node and return result.
+ *
+ * This method will do a partial run of the the graph
+ * from begining upto the index-th node and return output of index-th node.
+ * This is costly operation and suggest to use only for debug porpose.
+ *
+ * \param index: The index of the node.
+ * \param eid The Entry id of the op.
+ * \return Node output array.
+ */
+ NDArray RunLayerGetOutput(int index, int eid) {
+ ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
+
+ for (size_t i = 0; i < op_execs_.size(); ++i) {
+ if (op_execs_[i]) op_execs_[i]();
+ if (static_cast<int>(i) == index) break;
Review comment:
why not to move this condition to above for() statement? like
``` for (size_t i = 0; i < (index < op_execs_.size()) ? index : op_execs_.size(); ++i)```
--
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] mehrdadh commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825191897
@elvin-nnov PTAL.
Thanks!
--
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] elvin-nnov commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-828166113
LGTM
--
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] areusch commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r621323819
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -207,6 +200,42 @@ class GraphExecutorDebug : public GraphExecutor {
return -1;
}
+ /*!
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute next node in the network.
+ *
+ * This method will execute next node assuming
+ * previous nodes has been executed and return output of index-th node.
+ *
+ * \param node_ind: The index of the node.
+ * \param output_ind: The output index of this node.
+ * \return Node outputs array.
+ */
+ NDArray ExecuteNextNodeGetOutputs(int node_ind, int output_ind) {
+ ICHECK_LT(static_cast<size_t>(node_ind), op_execs_.size());
+
+ // Reset execution.
+ if (node_ind < 0) {
+ last_executed_node_ = -1;
+ return NDArray();
+ }
+
+ if (node_ind > last_executed_node_) {
Review comment:
I think it would be great if, in the PackedFunc API, we can separate the executing of layers from returning of outputs. it's confusing when the process to get output data is different depending which index you care about (which would be the case, in practice, with this API). can we have two functions:
1. `execute_node(i)` runs nodes up to node `i`, returns nothing. it can be smart internally by tracking `last_executed_node_` and only running the additional nodes needed to increment `last_executed_node_` to `i`, when `i` > `last_executed_node_`.
2. `get_output(i, j)`, which returns output `j` from node `i`. should assert `i == last_executed_node_`, which provides a check that the returned value will be valid.
--
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] areusch commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r620669445
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -210,65 +210,42 @@ class GraphExecutorDebug : public GraphExecutor {
return static_cast<size_t>(nodes_[index].param.num_outputs);
}
- /*!
- * \brief Execute network to a specific node and return result.
- *
- * This method will do a partial run of the the graph
- * from begining upto the index-th node and return output of index-th node.
- * This is costly operation and suggest to use only for debug porpose.
- *
- * \param index: The index of the node.
- * \return Node output array.
- */
- Array<NDArray> DebugGetNodeOutput(int index) {
- ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- Array<NDArray> results;
-
- for (size_t i = 0; i <= static_cast<size_t>(index); i++) {
- if (op_execs_[i]) op_execs_[i]();
- }
- for (size_t j = 0; j < NodeGetNumOutputs(index); j++) {
- results.push_back(data_entry_[entry_id(index, j)].CopyTo({kDLCPU, 0}));
- }
- return results;
- }
-
/*!
* \brief Execute next node in the network.
*
* This method will execute next node assuming
* previous nodes has been executed and return output of index-th node.
*
- * \param index: The index of the node.
- * \param eid The Entry id of the op.
- * \return Node output array.
+ * \param node_ind: The index of the node.
+ * \param output_ind: The output index of this node.
+ * \return Node outputs array.
*/
- NDArray ExecuteNextNodeGetOutput(int index, int eid) {
- ICHECK_LT(static_cast<size_t>(index), op_execs_.size());
- if (op_execs_[index]) op_execs_[index]();
- return data_entry_[entry_id(index, eid)];
+ NDArray ExecuteNextNodeGetOutputs(int node_ind, int output_ind) {
+ ICHECK_LT(static_cast<size_t>(node_ind), op_execs_.size());
+ if (op_execs_[node_ind]) op_execs_[node_ind]();
Review comment:
it would still be great to split this line and the following line into separate functions. we should be able to do:
1. run node layer
2. get layer output j
as separate things
--
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] areusch merged pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #7903:
URL: https://github.com/apache/tvm/pull/7903
--
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] mehrdadh commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825184703
@elvin-nnov sorry for the confusion. I meant `debug_get_output` in python API. You're right about the c++ api.
I'm renaming the names here to make it more clear.
--
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] mehrdadh commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825112220
@tkonolige I'll try to add a C++ api that gets all outputs at one call.
--
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] mehrdadh commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r618738242
##########
File path: src/runtime/crt/host/main.cc
##########
@@ -110,7 +110,7 @@ tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) {
}
}
-uint8_t memory[512 * 1024];
+uint8_t memory[1024 * 1024];
Review comment:
This is specific for crt runtime. I'm running few models and it crashes in those cases.
--
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] mehrdadh commented on pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#issuecomment-825045911
cc @tkonolige
--
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] elvin-nnov commented on a change in pull request #7903: [Graph Executor Debugger] Fix parameter dump
Posted by GitBox <gi...@apache.org>.
elvin-nnov commented on a change in pull request #7903:
URL: https://github.com/apache/tvm/pull/7903#discussion_r620878239
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -207,6 +200,42 @@ class GraphExecutorDebug : public GraphExecutor {
return -1;
}
+ /*!
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
Review comment:
forgotten function during refactoring? should be removed?
##########
File path: src/runtime/graph_executor/debug/graph_executor_debug.cc
##########
@@ -207,6 +200,42 @@ class GraphExecutorDebug : public GraphExecutor {
return -1;
}
+ /*!
+ * \brief Get number of outputs of the node.
+ * \param index The index of the node.
+ * \return The number of outputs.
+ */
+ size_t NodeGetNumOutputs(size_t index) {
+ if (nodes_[index].op_type != "tvm_op") return 1;
+ return static_cast<size_t>(nodes_[index].param.num_outputs);
+ }
+
+ /*!
+ * \brief Execute next node in the network.
+ *
+ * This method will execute next node assuming
+ * previous nodes has been executed and return output of index-th node.
+ *
+ * \param node_ind: The index of the node.
+ * \param output_ind: The output index of this node.
+ * \return Node outputs array.
+ */
+ NDArray ExecuteNextNodeGetOutputs(int node_ind, int output_ind) {
+ ICHECK_LT(static_cast<size_t>(node_ind), op_execs_.size());
+
+ // Reset execution.
+ if (node_ind < 0) {
+ last_executed_node_ = -1;
+ return NDArray();
+ }
+
+ if (node_ind > last_executed_node_) {
Review comment:
The PR looks almost good from my point of view, at the same time I would fix something in the logic of this function.
There are two corner cases that might fail with current logic
1. if we pass index less than currently executed node, nothing will be executed, but tensor value might be wrong because it might be overwritten by next ones already
2. if we pass index bigger than `last_executed_node_+1`, wrong results can be returned as well
I would recommend either return index from function's arguments and just has internal mechanism of calculating what is a current index and which next layer will be executed and returned. In this case we will have to care about stop conditions and getting start from beginning for example by adding couple more functions. That might be not elegant. Or we might not verify if desired layer is following just after executed and make caller responsible for all verification. And in this case the name of the function just should not contain `next`. It should be something like `ExecuteNodeGetOutput`/`execute_node_get_output`. And need to remove all verification of `last_executed_node_`.
The second approach is simplier
--
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