Skip to content

Conversation

@wchargin
Copy link
Contributor

@wchargin wchargin commented Mar 15, 2021

Summary:
This patch replaces the multiplexer code in the custom scalars plugin
with data provider code. It is not gated behind a flag.

We treat the layout protos as tensors, since they are of scalar shape
and are expected to be small. A case could be made for representing them
as blob sequences with a datacompat shape conversion, but this is
simpler and the plugin doesn’t get a ton of use.

Test Plan:
The dashboard still works with the standard demo data, both with and
without --load_fast=true.

wchargin-branch: custom-scalars-generic

Summary:
This patch replaces the multiplexer code in the custom scalars plugin
with data provider code. It is not gated behind a flag. We treat the
layout protos as tensors, since they are of scalar shape and are
expected to be small. A case could be made for representing them as blob
sequences with a datacompat shape conversion, but this is simpler and
the plugin doesn’t get a ton of use.

Test Plan:
The dashboard still works with the standard demo data, both with and
without `--load_fast`.

wchargin-branch: custom-scalars-generic
wchargin-source: 91001190a543e99ba73c407df3bc801d81eaa2cf
@wchargin wchargin added plugin:custom-scalars core:rustboard //tensorboard/data/server/... labels Mar 15, 2021
@google-cla google-cla bot added the cla: yes label Mar 15, 2021
@wchargin wchargin requested a review from psybuzz March 15, 2021 18:01
@wchargin wchargin mentioned this pull request Mar 15, 2021
34 tasks
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.

I realize this is simpler and that it's not worth spending much extra effort here, but more so than mesh, this feels a bit strange to call this a tensor data? Since there isn't really even useful substructure there (it's just a scalar bytestring) unlike the mesh or text plugins, so it really feels clearly semantically a blob rather than a tensor IMO. (I suppose one could even argue that in some ways this blob should be in summary metadata like we do for hparams since it's not really meant to function like a time series at all; it's more of a run-level or experiment-level config than a time series.)

Anyway, no action needed here, but let's maybe consider this provisional for the purposes of whether we'd persist this as a tensor time series anywhere (beyond the status quo of summaries in event files).

@wchargin
Copy link
Contributor Author

Since there isn't really even useful substructure there […] it really feels clearly semantically a blob rather than a tensor IMO

Yes, agreed. I was moderately on the fence here, and would be willing to
embed this into a blob sequence if we’d prefer that. There’s precedent
with audio and hparams, I think, so it’s not out of the question.

wchargin-branch: custom-scalars-generic
wchargin-source: 4cc80e219d7fe19c1986763d19eee493073cb4ba

# Conflicts:
#	tensorboard/data/server/data_compat.rs
#	tensorboard/dataclass_compat.py
wchargin-branch: custom-scalars-generic
wchargin-source: 4cc80e219d7fe19c1986763d19eee493073cb4ba
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.

In case you decided to switch to the blob approach in this PR, please re-request review. Otherwise, lgtm to use tensors provisionally, as nfelt@ said.

@wchargin wchargin merged commit 11c42f5 into master Mar 16, 2021
@wchargin wchargin deleted the wchargin-custom-scalars-generic branch March 16, 2021 20:01
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.

3 participants