rust: support graph sub-plugins #4569
Conversation
Summary: We now support TF 1.x images, TF 2.x images, and any tensors that have an explicit blob sequence data class. Test Plan: Unit tests included. As an end-to-end test, images now appear in TensorBoard with `--load_fast`, in both the images and time series dashboards. wchargin-branch: rust-images-generic-blobs wchargin-source: 614b7b48a326e3303af05b5790da402ba5f72af3
Summary: For some data types, data-compat conversions of _values_ depend on the time series _metadata_. For instance, we need to adjust the shapes of audio tensors to remove labels, and we need to update graph sub-plugins (Keras models, etc.) to convert scalars to tensors. This patch updates the `EventValue::into_blob_sequence` interface to require a reference to the initial summary metadata so that we can add support for such data types. It’s not yet used. Test Plan: Existing unit tests suffice; for an end-to-end test the graphs and images plugins still work. wchargin-branch: rust-blob-sequence-metadata wchargin-source: 318204dbc3eaedf339f13b4d166b9b823248a6af
Summary: We now support TF 1.x and TF 2.x audio summaries. This includes TF 2.x audio summaries with labels; the labels will be stripped. This means that the audio dashboard now works under `--load_fast`. Test Plan: Unit tests should suffice. As an end-to-end test, checked the audio dashboard against TF 2.x audio summaries both with and without labels (i.e., of shape `[k, 2]` and of shape `[k]`), and against [TF 1.x data]. [TF 1.x data]: https://gist.github.com/wchargin/d9c415fb609119824919de8391d0b8c4 wchargin-branch: rust-audio wchargin-source: 16ec21cd82fb0145b816d2d2b3e01859d4a54511
Summary: This patch adds support for TF 1.x `tagged_run_metadata` events. Because this is a new top-level event type, the change extends into the `run` module as well as `data_compat`, but the changed surface area is still rather small. Test Plan: The graphs demo added in #4469 includes tagged run metadata graphs. They now appear in the graphs dashboard with `--load_fast`, and include compute time information. wchargin-branch: rust-tagged-run-metadata wchargin-source: e8ed2e7af25aba1206bccf77218edaf231b1c858
Summary: This patch adds support for data read by the graphs dashboard but written under the following plugin names: - `graph_run_metadata` - `graph_run_metadata_graph` - `graph_keras_model` (source of truth in `plugins/graph/metadata.py`). Test Plan: Unit tests included. As an end-to-end test, all the data written by the graphs plugin demo now works with `--load_fast`. wchargin-branch: rust-graph-subplugins wchargin-source: 6364e461c0617338db342b2d35ef5cee245c6ccb
wchargin
added
plugin:graph
plugin:graph:profile
| | Some(GRAPH_RUN_METADATA_PLUGIN_NAME) | ||
| | Some(GRAPH_RUN_METADATA_WITH_GRAPH_PLUGIN_NAME) | ||
| | Some(GRAPH_KERAS_MODEL_PLUGIN_NAME) => { |
There was a problem hiding this comment.
Future plan question: are we going to trust the data_class property on the proto in the future? I believe the graph_run* and graph_keras* data are all annotated correctly with the data_class.
There was a problem hiding this comment.
We already trust the data class: the first clause of this match is
match (md, &*self.0) {
// Any summary metadata that sets its own data class is expected to already be in the right
// form.
(Some(md), _) if md.data_class != i32::from(pb::DataClass::Unknown) => Box::new(md),(Likewise for Python TensorBoard.)
But I’m not sure if the graph data does set data classes? The data
written by :graphs_demo doesn’t seem to have them, and I don’t see
data classes in the writing code. In any case, data classes were
introduced more recently than the graph_* writing code, so old data
must not have them, I think.
So eventually I would probably like plugins to write summaries with data
class explicitly set, but I expect to maintain this compatibility layer
indefinitely, and that seems okay to me. WDYT?
| let initial_metadata = v.initial_metadata(Some(md)); | ||
| assert_eq!( | ||
| *initial_metadata, | ||
| pb::SummaryMetadata { | ||
| plugin_data: Some(PluginData { | ||
| plugin_name: plugin_name.to_string(), | ||
| content: b"1".to_vec(), | ||
| ..Default::default() | ||
| }), | ||
| data_class: pb::DataClass::BlobSequence.into(), | ||
| ..Default::default() | ||
| }, | ||
| ); |
There was a problem hiding this comment.
No AI required: this test feels too mechanical :\
There was a problem hiding this comment.
Like, too much of “assert that f(x) equals the result of manually
computing f(x)”? I agree; it does. I think that I feel okay with it
since it is actually meaningful coverage—dropping a single line in the
match would cause parts of the graph plugin to silently not render.
Or maybe you meant that the test implementation spends a lot of lines on
proto manipulation? Agreed there, too. We could refactor to reuse proto
sub-structures, but I generally like optimizing for readability in tests
like this. In #4552, for example, I made the tests more explicit because
if they had been written like that in the first place, the bug would
likely have been caught in code review: new Date(1610613017) is an
obvious bug—too few digits—whereas new Date(fakePoints1[0].x) is not
obviously correct and just repeats the implementation code, which was
also wrong.
Noted that you said “no AI required”, so feel free to reply or not, as
you like. Glad that this passes your minimum bar, but also always
interested in doing better than that.
wchargin-branch: rust-graph-subplugins wchargin-source: 18ab56494a673282f35985487df79a72a1ab7b41 # Conflicts: # tensorboard/data/server/data_compat.rs
wchargin-branch: rust-graph-subplugins wchargin-source: 18ab56494a673282f35985487df79a72a1ab7b41
|
Merging, but happy to discuss more. |
2 participants
Summary:
This patch adds support for data read by the graphs dashboard but
written under the following plugin names:
graph_run_metadatagraph_run_metadata_graphgraph_keras_model(source of truth in
plugins/graph/metadata.py).Test Plan:
Unit tests included. As an end-to-end test, all the data written by the
graphs plugin demo now works with
--load_fast.wchargin-branch: rust-graph-subplugins