feat: combine graph by prefixing with unique name #4334
Conversation
Previously, graph plugin combined multiple graphs traced by creating one monolith of a GraphDef. In doing so, we checked whether, for example, node names are unique to detect a case when our graph vis can result in faulty UI. To alleviate the poor UX, we decided, instead, to duplicate all nodes in one gaint GraphDef container prefixing all names. While this creates some bloat, (1) users should see the confusing error less and (2) combined graphs make it very clear that we have traced multiple graphs.
stephanwlee
force-pushed
the
multi_grap
branch
from
0bf07f5
to
eacf107
Compare
| if dst_graph_def.versions.producer != graph_def.versions.producer: | ||
| raise ValueError("Cannot combine GraphDefs of different versions.") | ||
|
|
||
| mapped_graph_def = _prepend_names("graph_%d" % (index + 1), graph_def) |
There was a problem hiding this comment.
You might consider a function signature more like
def _add_with_prepended_names(prefix, graph_to_add, destination_graph)That way we can avoid the overhead of constructing the temporary graphdef that's returned here, only to then copy its nodes and functions into the destination graph def. Since proto python objects aren't immutable, the python code has to do a deep copy each time we move the nodes around, so it would be nice not to do that twice when we could do it just once.
| for func in orig_graph_def.library.function: | ||
| new_func = mut_graph_def.library.function.add() | ||
| new_func.CopyFrom(func) | ||
| # Not creating a structure out of factored out function. They already |
There was a problem hiding this comment.
Does this comment refer to the fact that we don't use a / in the function name prefix, just an underscore? If so, the comment might make more sense as a comment on _prefixed_func_name() itself.
|
While I am cautiously optimistic that this change would alleviate much of #1929, I suspect this change has not covered few cases such as differing GraphDef version and gradient function support (while I have some code to handle it, I could not get my hands on a realistic example and had to just go by the comments on the definition). I will keep the issue for the time being and maybe remove the UI instruction to refer to #1929 when I truly think it is solved. |
Previously, graph plugin combined multiple graphs traced by creating one
monolith of a GraphDef. In doing so, we checked whether, for example,
node names are unique to detect a case when our graph vis can result in
faulty UI.
To alleviate the poor UX, we decided, instead, to duplicate all nodes in
one giant GraphDef container prefixing all names. While this creates
some bloat, (1) users should see the confusing error less and (2)
combined graphs make it very clear that we have traced multiple graphs.