Skip to content
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

Add a "Functions" scene group #394

Merged
merged 7 commits into from
Aug 24, 2017
Merged

Add a "Functions" scene group #394

merged 7 commits into from
Aug 24, 2017

Conversation

chihuahua
Copy link
Member

This change adds a "Functions" scene group if the graph being visualized has a function library, which contains definitions for functions (templates for subgraphs) used in the TensorFlow graph.

As part of that effort, this change also strips the arbitrary hexadecimal suffix from names of functions in the graph. That suffix is not user-generated - it merely serves to distinguish between functions with the same name but different signatures.

This refactors the implementation to prefix metanodes representing functions with FUNCTION_LIBRARY_NODE_PREFIX instead of lumping them under a metanode named FUNCTION_LIBRARY_NODE. We now also remove the function metanodes from the core graph later when building the sub-hierarchy instead of excluding them in the first place from the __root__ metagraph. That lets us use existing paths to render the metanodes for functions in the scene group.

The metanodes in the scene group (which represent function definitions) are not yet linked to individual calls throughout the graph - that will happen in a later change.

This change is complex and will be tested via internal Selenium-based integration tests.

Screenshot:

zyn4vsdjbv4

@dsmilkov
Copy link
Contributor

:lgtm:


Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.


tensorboard/plugins/graph/tf_graph_common/render.ts, line 1458 at r1 (raw file):

const functionNode = node as Metanode;

Does it make sense to do the cast inside the if statement, after you test that type == NodeType.META?


Comments from Reviewable

@chihuahua
Copy link
Member Author

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.


tensorboard/plugins/graph/tf_graph_common/render.ts, line 1458 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

const functionNode = node as Metanode;

Does it make sense to do the cast inside the if statement, after you test that type == NodeType.META?

Yes! Done. I restarted a TensorBoard, and the scene group WAI (same screenshot as before).


Comments from Reviewable

@chihuahua chihuahua merged commit 407806a into master Aug 24, 2017
@chihuahua chihuahua deleted the chizeng-function-library branch August 24, 2017 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants