Feature/per node log level setting #1217
Conversation
| @@ -15,6 +15,8 @@ | |||
| benchmarkIn = p.create(dai.node.BenchmarkIn) | |||
| benchmarkIn.setRunOnHost(True) # The node can run on host or on device | |||
| benchmarkIn.sendReportEveryNMessages(100) | |||
| benchmarkIn.logReportsAsWarnings(False) | |||
There was a problem hiding this comment.
We can remove that now and set the default warning level to info (I think it's trace now)
There was a problem hiding this comment.
A clarifying question: Do we want benchmark to report to a single log level? In my view, we should be able to set that level dynamically, through a setting entry. Nevertheless, I believe that per-node log level and what level benchmark nodes report to is two independent features. If we are to change the way benchmark nodes work, I would do it in a separate, smaller PR. I certainly can take that task.
There was a problem hiding this comment.
Not relevant to the PR I think?
There was a problem hiding this comment.
Not sure how these changes got into the PR. Removed a2cfcf8
| @@ -17,6 +18,7 @@ struct NodeObjInfo { | |||
|
|
|||
| std::vector<std::uint8_t> properties; | |||
|
|
|||
| LogLevel logLevel = LogLevel::WARN; | |||
There was a problem hiding this comment.
Does this play well?
I was thinking we'll need a tri-state here, to be able to tell if the log level was explicitly specified per node or not.
For example setting it the node level to WARN when the default level is set to trace would mean that that specific node is set to WARN and it should actually remain at that level. However, if default level is set to trace with DEPTHAI_LEVEL and the specific node log level wasn't touched, the node should be logging with the trace level.
There was a problem hiding this comment.
The way it works right now is as follows:
If no log level is specified, we use the default value for all the nodes.
If DEPTHAI_LEVEL is set, all nodes log at given level.
If DEPTHAI_LEVEL is set and we set the log level for the entire device, say to INFO, then all the nodes log at the INFO log level.
If DEPTHAI_LEVEL is set and we specify a log level for a single node, say to INFO, then for that particular node, DEPTHAI_LEVEL is overriden.
I am not sure what the desired behavior should be, this really depends on our preferences. Changing it is not big deal.
Also, a question regarding NodeObjInfo instances. Correct me if I am wrong here. These are essentially part of the a pipeline schema, defining basic per-node properties. We are not using the logLevel anywhere on the FW side, so it is useless from this perspective. However, I also thought schemas are used for pipeline diagrams and my thinking was this could be a useful piece of information, especially for debugging purposes.
| @@ -57,4 +57,14 @@ void DeviceNode::run() { | |||
| // } | |||
| } | |||
|
|
|||
| void DeviceNode::setLogLevel(dai::LogLevel level) { | |||
| int64_t myid = id; | |||
| device->setNodeLogLevel(myid, level); | |||
There was a problem hiding this comment.
Even before pipeline.start()? Intentional?
There was a problem hiding this comment.
Yes, setting the log level before starting the pipeline is a feature, not something unintended.
| @@ -205,7 +205,7 @@ PipelineSchema PipelineImpl::getPipelineSchema(SerializationType type) const { | |||
| throw std::invalid_argument(fmt::format("Node '{}' should subclass DeviceNode or have hostNode == true", info.name)); | |||
| } | |||
| deviceNode->getProperties().serialize(info.properties, type); | |||
|
|
|||
| info.logLevel = deviceNode->getLogLevel(); | |||
There was a problem hiding this comment.
Is this used? I guess the current implementation stores the log level on the device anyhow?
There was a problem hiding this comment.
See my comment above: #1217 (comment)
This PR adds the functionality of log-level setting for individual nodes. The functionality added allows us to set log-levels for individual nodes, both before and after starting the pipeline (
pipeline.start()) call.Note that if one sets the
DEPTHAI_LEVELenvironment variable, it will get overridden by the correspondingsetLogLevelmethod call for a node. Moreover, if one sets the log level for the entire device (device.setLogLevel(...)), the per-node log levels get overridden.Thus we have the following hierarchy, where greater means precedence.
DEPTHAI_LEVEL<Node<DeviceAs part of this PR, I have also refactored the test for
log-leveltesting, trying to avoid copy-pasted code.Clickup task: https://app.clickup.com/t/86c1gb36k