Skip to content

Conversation

@nfelt
Copy link
Contributor

@nfelt nfelt commented Feb 25, 2021

Partial work towards rustboard tensors support (subtask of #4422).

This just adds a tensors field to RunData inside Commit. We use pb::TensorProto as the value type since that's what we will end up reading off disk and sending over the wire, although it's not terribly ergonomic to manipulate in Rust since we lack Numpy and the tf.make_tensor_proto() and tf.make_ndarray() conversion methods. (I looked briefly at ndarray which seems nice but probably overkill at this point? We can see if we end up needing to do enough tensor manipulation to make it worthwhile.)

That in and of itself is a one-line change, but we also extend CommitBuilder to support constructing commits that contain tensor data. The code there is borrowed from a combination of the scalars and blobs CommitBuilder logic.

@nfelt nfelt added the core:rustboard //tensorboard/data/server/... label Feb 25, 2021
@nfelt nfelt requested a review from wchargin February 25, 2021 22:42
@google-cla google-cla bot added the cla: yes label Feb 25, 2021
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

although it's not terribly ergonomic to manipulate in Rust since we lack Numpy and the tf.make_tensor_proto() and tf.make_ndarray() conversion methods

True. I think that we’ll be okay. The most complex thing we need to do
is histogram datacompat, where you have to futz with a [k, 3] matrix,
but it’s still not that bad imho.

metadata: Option<Box<pb::SummaryMetadata>>,
/// Tensor evaluation function, called for each point in the series.
///
/// By default, this maps every step to the empty float32 tensor.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually a valid tensor? Its shape is not specified, and if it’s
inferred to be an empty list of dimensions, then it has the wrong number
of elements (0 instead of 1).

For hparams, we use a length-0 float vector instead. Might that be
more appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, you're right, thanks. Changed to use length-0 float vector.

}

/// Constructs a scalar time series from the state of this builder.
/// Constructs a blob sequence time series from the state of this builder.
Copy link
Contributor

Choose a reason for hiding this comment

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

oops; thanks :-)

@nfelt nfelt merged commit ca8117f into master Feb 26, 2021
@nfelt nfelt deleted the nfelt-rust-tensors-commit branch February 26, 2021 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants