-
Notifications
You must be signed in to change notification settings - Fork 326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Graph Visualization: Added hierarchies for commands and subcommands. Added flag to get the graph format in pbtxt , proto or dot. #2524
Conversation
@@ -289,3 +321,7 @@ func (g *graph) getGraphInPbtxtFormat() string { | |||
} | |||
return output | |||
} | |||
|
|||
func (g *graph) getGraphInProtoFormat() string { | |||
return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see much point in including this skeleton definition. I would probably rip out all the pieces of this PR related to the "proto" format, and put them into a separate branch with the code that actually dumps the raw dependency graph. I think having both "pbtxt" and "proto" is kind of confusing, particularly since only one of them is actually related to "graph visualization".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in fact the idea to consider 'proto' format was because we wanted some fast way to get the output for the graph visualization, but the 'proto' format does not look exactly like 'pbtxt' (Tensorboard can not read this), and also in a later commit using a fast concatenation (bytes.Buffer) is possible get a similar performance to get the 'pbtxt' format of the graph. So I removed all code in regard to 'proto' format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, in general I would prefer this rpc to return a proto, and generate the pbtext/dot in a command that calls into this rpc. I understand that it is a huge refactor, and we can leave it to later, but putting external format-specific code into GAPIS is generally something to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does seem like a better solution, once we have proper serialization of the dependency graph. This serialization is a bit messy since the dependency graph currently holds onto a reference to the capture, which is how we get information about the commands associated with the nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how I would probably implement this is the dependency graph would contain
path.Command (s) that can be used by the external tool, to get the command information if it wants it.
9b7d33d
to
d386b1e
Compare
gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go
Outdated
Show resolved
Hide resolved
gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go
Show resolved
Hide resolved
gapis/resolve/dependencygraph2/graph_visualization/graph_visualization.go
Show resolved
Hide resolved
based on CommandBuffer and QueueSubmit. Added flag to get the graph visualization in formats: 'pbtxt' (tensorboard) or 'dot' (graphviz). Subcommand Name: To get the name for subcommands, it is used the initialization commands (no-real commands).
d386b1e
to
cd5cbf3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No description provided.