New OpGraph #5485
New OpGraph #5485
Conversation
|
CI MESSAGE: [15353368]: BUILD STARTED |
|
CI MESSAGE: [15353368]: BUILD FAILED |
dali/pipeline/graph/op_graph2.h
Outdated
| * The string is `const` because it's view is used as a key. | ||
| */ | ||
| const std::string instance_name; | ||
| /** The specification of an operato */ |
There was a problem hiding this comment.
| /** The specification of an operato */ | |
| /** The specification of an operator */ |
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
| /** A visit marker for various graph processing algorithms. */ | ||
| mutable bool visited = false; | ||
| /** A visit marker for cycle detection. */ | ||
| mutable bool visit_pending = false; |
There was a problem hiding this comment.
Not sure how I feel about those, especially if there is no encapsulation (all in public interface).
I am not an expert in designing graph implementations, but shouldn't the processing metadata be separated from the graph representation/information? Especially if we may end up with some processing step that requires other metadata, we probably don't want to add it all here.
There was a problem hiding this comment.
I know.... on the other hand, there's no way to implement it that's efficient and the elegance of the alternatives is also questionable. I know we're not writing a compiler and the size of the graph is going to be small to moderate (thousands of nodes, not millions), but even then looking up helper structures for extremely common type of metadata, such as visit markers, seems silly.
Another option:
union {
void *meta_ptr;
uint64_t meta_int;
uint8_t meta_bytes[8];
};If an algorithm needs more then 8 bytes, it can create a list of metadata items and use the meta_ptr to associate it with the nodes.
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
dali/pipeline/graph/op_graph2.cc
Outdated
| explicit Visit(Node *n) : node_(n) { | ||
| if (node_->visit_pending) | ||
| throw std::logic_error("Cycle detected."); | ||
| node_->visit_pending = true; |
There was a problem hiding this comment.
Do we clear visit_pending before entering the visiting phase? I only see that we clear visited markers, as there is no encapsulation, I think we should clear all at the start.
There was a problem hiding this comment.
visit_pending is cleared by the Visit helper. Perhaps I could move it to the header to make it available to other graph algorithms in the future.
dali/pipeline/graph/op_graph2.cc
Outdated
| public: | ||
| explicit Visit(Node *n) : node_(n) { | ||
| if (node_->visit_pending) | ||
| throw std::logic_error("Cycle detected."); |
There was a problem hiding this comment.
Side question, are we considering loops? With AutoGraph it's doable to possibly have cycles with for or while loop.
There was a problem hiding this comment.
No, that's not supported and was expressly stated as a non-goal for the new executor. It's perhaps just my opinion, but I see diminishing returns in supporting graphs with backend-side flow control.
| OpGraph g = std::move(b).GetGraph(); | ||
|
|
||
| auto &nodes = g.OpNodes(); | ||
| ASSERT_EQ(nodes.size(), 3_uz) << "The nodes were nod added properly - abandoning test"; |
There was a problem hiding this comment.
| ASSERT_EQ(nodes.size(), 3_uz) << "The nodes were nod added properly - abandoning test"; | |
| ASSERT_EQ(nodes.size(), 3_uz) << "The nodes were not added properly - abandoning test"; |
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
dali/pipeline/graph/op_graph2.cc
Outdated
| graph_.op_nodes_.splice(graph_.op_nodes_.end(), out_ops); | ||
| graph_.data_nodes_.splice(graph_.data_nodes_.end(), out_data); |
There was a problem hiding this comment.
Just a note, you didn't traverse the nodes that are not reachable from the outputs so they may be listed here in non-topological order.
There was a problem hiding this comment.
Nice catch. Perhaps I'll just traverse everything when not pruning and then skip this splicing.
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
|
CI MESSAGE: [15522865]: BUILD STARTED |
|
CI MESSAGE: [15522865]: BUILD PASSED |
Category:
New feature (non-breaking change which adds functionality)
Refactoring (Redesign of existing code that doesn't affect functionality)
Description:
This change moves the old OpGraph to the executor directory and creates a new, simple graph with much less semantic assumptions involved.
Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-3971