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 toggle that turns automatic high-degree node extraction off #4722

Merged
merged 21 commits into from
Mar 5, 2021

Conversation

martinwicke
Copy link
Member

This adds a toggle (next to "trace inputs") which turns automatic high-degree node extraction off.

  • Motivation for features / changes
    Automatic aux node creation can break the graph up in unwanted ways. For some graphs, the high-degree heuristic is not useful (graphs that have high-degree internal nodes, e.g., if you concatenate lots of towers or some such)

  • Technical description of changes
    This adds an argument to the hierarchy constructor which if passed, avoids calling the high-degree node extraction code. It also removes the hard-coded PARAM entry for node extraction (which turned off all node extraction, but wasn't user settable).

  • Detailed steps to verify changes work correctly (as executed by you)

    • build
    • upload graph known to be broken apart verify it's broken apart
    • toggle "auto-extract high-degree nodes"
    • verify graph changes, and high-degree nodes are not extracted
  • Alternate designs / implementations considered

    • Make the in/out degree triggering extraction configurable (probably not worth it)
    • Turn off high-degree extraction altogether instead of providing the option (would work for me, but I'm hesitant to break others in this manner)
    • Rely more heavily on op list extraction (this might work, but I lack the expertise to come up with a good op list)

@martinwicke martinwicke requested a review from psybuzz March 3, 2021 21:24
@google-cla google-cla bot added the cla: yes label Mar 3, 2021
Copy link
Contributor

@psybuzz psybuzz 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 contributing this PR!

tensorboard/plugins/graph/tf_graph/tf-graph-scene.ts Outdated Show resolved Hide resolved
tensorboard/plugins/graph/tf_graph/tf-graph.ts Outdated Show resolved Hide resolved
tensorboard/plugins/graph/tf_graph/tf-graph.ts Outdated Show resolved Hide resolved
tensorboard/plugins/graph/tf_graph_common/render.ts Outdated Show resolved Hide resolved
tensorboard/plugins/graph/tf_graph/tf-graph.ts Outdated Show resolved Hide resolved
tensorboard/plugins/graph/tf_graph/tf-graph-scene.ts Outdated Show resolved Hide resolved
@martinwicke
Copy link
Member Author

There's some lint left, I'll fix this when it's otherwise ready.

Can you take a look whether this is better?

Copy link
Contributor

@psybuzz psybuzz left a comment

Choose a reason for hiding this comment

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

LGTM % minor comments, I'm pleased to see this happen!

If the Prettier linting ends up being too painful, let me know and I can push a commit from my end as well.

@martinwicke
Copy link
Member Author

Happy to run prettier. It does propose some changes that are unrelated to my edits though -- do you want those?

@martinwicke
Copy link
Member Author

I didn't add the unrelated lint things for now.

@psybuzz
Copy link
Contributor

psybuzz commented Mar 4, 2021

It does propose some changes that are unrelated to my edits though -- do you want those?

I'm not sure why there would be unrelated edits, unless maybe Prettier is run from another directory (running from "tensorboard_repo_dir/tensorboard/" might not pickup our settings).

That said, I checked out your latest patch, ran Prettier locally, and it passed the linter locally, so it seems good to me

@martinwicke
Copy link
Member Author

Cool. Merge at will then. I can, for historical reasons, but I don't think I should.

@psybuzz psybuzz merged commit f4dcc42 into tensorflow:master Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants