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 histogram coloring for xla clusters similarly to devices. #1336

Merged
merged 3 commits into from
Aug 8, 2018

Conversation

r4nt
Copy link
Contributor

@r4nt r4nt commented Aug 3, 2018

No description provided.

Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

One quick question, not being very familiar with XLA - is there an easy way to generate demo/test data we could use to test this functionality?

@@ -650,32 +676,21 @@ export function getFillForNode(templateIndex, colorBy,
// Return the hue for unknown device.
return colorParams.UNKNOWN;
}
let id = renderInfo.node.name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to also deduplicate the code for the TPU compatibility logic below - it looks like it was essentially copy-pasted between the device color and TPU compat color cases previously, so now that you've factored it out it would ideally be factored out in all three places.

I think it should be pretty easy to adapt, but if you'd rather not bother that's fine and one of us can clean it up later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@r4nt
Copy link
Contributor Author

r4nt commented Aug 8, 2018

  • adapted the demo test data with XLA cluster and TPU op compatibility test cases
  • fixed the demo app to support loading graphs, which makes my other patch unnecessary

@nfelt
Copy link
Contributor

nfelt commented Aug 8, 2018

LGTM, thanks for making those extra changes!

Re: the other PR, yeah I'm not sure why the tf_graph_app index.html only shows the documentation view, I think this is one of the only places in TensorBoard that's used, perhaps for some historical reason. We could probably change it but if the demo works for you for now that's great.

@nfelt nfelt merged commit 416c794 into tensorflow:master Aug 8, 2018
@r4nt r4nt deleted the add-xla-cluster-histogram branch August 9, 2018 09:06
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