Skip to content

graph: use DataProvider-compatible APIs #4470

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.

Sign up for GitHub

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

Jump to bottom
Merged
wchargin merged 5 commits into from
Dec 15, 2020
Merged

graph: use DataProvider-compatible APIs #4470

wchargin merged 5 commits into from
Dec 15, 2020

Conversation

@wchargin
Copy link
Contributor

@wchargin wchargin commented Dec 14, 2020 •
edited
Loading

Summary:
Events with top-level tagged_run_metadata are now transformed at read
time into blob sequence summaries. As a result, the graph plugin can get
all its data via tensor summaries. This also includes replacing calls to
multiplexer.Graph with reads of the __run_graph__ tensor.

Since there’s an existing PLUGIN_NAME_RUN_METADATA, I had initially
hoped to use that for the summaries. But the graphs plugin actually
expects that data to include a graph (even though there is a separate
plugin called PLUGIN_NAME_RUN_METADATA_WITH_GRAPH) and there’s no way
to tell it apart from just the metadata. Thus, we implement this via a
new plugin name, which means that there are few structural changes to
the graph plugin code.

As written, the code still uses the multiplexer, but is set up to make
it easy to read from the data provider instead. We make that change in a
follow-up PR (see #4473).

Test Plan:
All data from the graphs demo (added in #4469) still works. Grepping for
_multiplexer in the graphs plugin code shows that the only calls are
to PluginRunToTagToContent and Tensors, both of which have
equivalents in the data provider API.

wchargin-branch: graph-data-provider-compatible

@wchargin
graph: add demo for exotic graph types
fd72802 
Summary:
The graphs plugin is most frequently used to visualize run graphs, but
supports more than that. This patch adds a demo that generates data for
all the data types that the graphs plugin currently supports.

Test Plan:
Run `bazel run //tensorboard/plugins/graph:graphs_demo`, then point
TensorBoard at `/tmp/graphs_demo`. Open the graphs dashboard and inspect
the contents of the `/data/plugin/graphs/info` response. Note that it
contains:

  - tags with op graph but no other data (`keras/train:batch_2`)
  - tags with both profile and op graphs (`profile:prof_f`)
  - tags with conceptual graph only (`keras/train:keras`)
  - runs with `run_graph` set to `true` (`tagged`)
  - tags with profile data only (`tagged:step_0000`)

…corresponding to the five cases of `info_impl`.

wchargin-branch: graph-demo
wchargin-source: 7af67c0eb106780fffa76fffe805b9953a85958c
@wchargin
graph: use DataProvider-compatible APIs
39943d4 
Summary:
Events with top-level `tagged_run_metadata` are now transformed at read
time into blob sequence summaries. As a result, the graph plugin can get
all its data via tensor summaries. This also includes replacing calls to
`multiplexer.Graph` with reads of the `__run_graph__` tensor.

Since there’s an existing `PLUGIN_NAME_RUN_METADATA`, I had initially
hoped to use that for the summaries. But the graphs plugin actually
expects that data to include a graph (even though there is a separate
plugin called `PLUGIN_NAME_RUN_METADATA_WITH_GRAPH`) and there’s no way
to tell it apart from just the metadata. Thus, we implement this via a
new plugin name, which means that there are few structural changes to
the graph plugin code.

Test Plan:
All data from the graphs demo (added in #4469) still works. Grepping for
`_multiplexer' in the graphs plugin code shows that the only calls are
to `PluginRunToTagToContent` and `Tensors`, both of which have
equivalents in the data provider API.

wchargin-branch: graph-data-provider-compatible
wchargin-source: e9f45a6529f7d07a725d26e60bef13914ae68a41
@wchargin wchargin added plugin:graph core:rustboard //tensorboard/data/server/... labels Dec 14, 2020
@google-cla google-cla bot added the cla: yes label Dec 14, 2020
Base automatically changed from wchargin-graph-demo to master December 14, 2020 22:38
@wchargin
[graph-data-provider-compatible: update diffbase]
0bd91c5 
wchargin-branch: graph-data-provider-compatible
wchargin-source: fecc8a191077c0229f7969b2108cf185f51a98b0

# Conflicts:
#	tensorboard/plugins/graph/graphs_demo.py
@wchargin
[graph-data-provider-compatible: update patch]
a7f48b9 
wchargin-branch: graph-data-provider-compatible
wchargin-source: fecc8a191077c0229f7969b2108cf185f51a98b0
@wchargin wchargin requested a review from stephanwlee December 14, 2020 22:51
stephanwlee
stephanwlee approved these changes Dec 15, 2020
View reviewed changes
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

I see some repetition with #4473. Am I reading it right?

@wchargin
Copy link
Contributor Author

wchargin commented Dec 15, 2020

I see some repetition with #4473. Am I reading it right?

Not sure exactly what you mean? That PR is diffbased against this one.
They certainly are related. I separated the two PRs out because the
second one is pretty straightforward given this one, whereas I figured
that you might have input about the “new plugin name” approach or other
details in this PR. In other words, this PR makes the hard change easy,
and the next PR makes the easy change.

Does this help clarify?

@stephanwlee
Copy link
Contributor

stephanwlee commented Dec 15, 2020

I guess I still don't understand how to read your commits; some commit labels/names feel arbitrary and I cannot always make the correlation.

@wchargin
Copy link
Contributor Author

wchargin commented Dec 15, 2020 •
edited
Loading

Ah. The names are somewhat arbitrary (but imho yours are, too, like
sync or pp). The easiest way to see the diffbase is just to look at
the base branch:

Screenshot of the base branch view for PRs 4470 and 4473, showing that 4470 merges branch foo into master and 4473 merges branch bar into foo, thus 4473 depends on 4470

But, if it would be helpful, I can try to more consistently call out the
dependencies in the description text (“builds on #xyz”/“in a follow-up, …”).
Or, if you have any suggestions or examples of things that you find hard
to understand, please let me know; I’d be more than happy to adjust my
workflow to make it easier to grok.

@stephanwlee
Copy link
Contributor

stephanwlee commented Dec 15, 2020 •
edited
Loading

To be clear, I am not talking about the branch name. Rather, I am referring to your commit titles.

For instance, I see this bit repeated in this PR but looking at the commits, it is impossible to infer that #4473 was built on top of #4470 (or they share a common root)
https://github.com/tensorflow/tensorboard/pull/4473/files#diff-65f6808e40d634101e802940b9c39d45301ea75826c4016f8156b658e6130d3dR129-R136

#4470
image

#4473
image

It looks like I am confusing myself since I was reading your PRs in a backwards order (4473 -> 4470) and was reading a piece of code that is to be changed in 4470 (PluginRunToTagToContent -> list_blob_sequence).

@wchargin
Copy link
Contributor Author

wchargin commented Dec 15, 2020 •
edited
Loading

Got it. The commits for #4473 don’t show that it’s built on top of this
PR because the base branch is PR’s head branch, so it’s comparing
wchargin-graph-data-provider-compatible...wchargin-graph-data-provider-only,
which indeed does not contain any commits from this PR. So I recommend
looking at the base branch to determine the order. I also try to open
the PRs in topological order, so #4470 should precede #4473 logically
just because it does so numerically.

As to the commit titles, my “update patch” is your “cr address” or
other people’s “ac” (address changes). I agree that they are mostly
devoid of information. (“Update diffbase” is just “merge changes from
parent branch into this branch”, or “rebase”, or “sync”. If you have a
suggestion for a clearer wording, let me know; ideally these wouldn’t
require much special thought on your part.)

And the extra commits in this PR are just because it hasn’t yet been
updated since its parent #4469 was merged, so master has an extra
commit (the squashed version of #4469) that this branch doesn’t have yet
and vice versa (the unsquashed version).

@wchargin
[graph-data-provider-compatible: update diffbase]
4302d88 
wchargin-branch: graph-data-provider-compatible
wchargin-source: 2dee832ff7fc0a741c0836222bb0dfae60857235
@stephanwlee
Copy link
Contributor

stephanwlee commented Dec 15, 2020

Ah, wchargin-graph-data-provider-compatible...wchargin-graph-data-provider-only... I totally missed that. My bad!

@wchargin
Copy link
Contributor Author

wchargin commented Dec 15, 2020

No problem! :-) there are lots of small fixes that I would like to make
to these UIs, like showing the diffbase graph à la Phabricator/Gerrit…

@wchargin wchargin merged commit 6b0b3e0 into master Dec 15, 2020
@wchargin wchargin deleted the wchargin-graph-data-provider-compatible branch December 15, 2020 06:25
@wchargin wchargin mentioned this pull request Mar 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephanwlee stephanwlee stephanwlee approved these changes

Assignees

No one assigned

Labels

cla: yes core:rustboard //tensorboard/data/server/... plugin:graph

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wchargin @stephanwlee