Skip to content

Conversation

@wchargin
Copy link
Contributor

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.

wchargin-branch: rust-audio

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
@wchargin wchargin added the core:rustboard //tensorboard/data/server/... label Jan 16, 2021
@google-cla google-cla bot added the cla: yes label Jan 16, 2021
@wchargin wchargin requested a review from psybuzz January 16, 2021 01:35
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.

neat use of chunks_exact_mut

/// this is audio data, `tensor` may also be a string tensor of shape `[k, 2]`, in which case
/// the second axis is assumed to represent string labels and is dropped entirely.
pub fn into_blob_sequence(
self,
Copy link
Contributor

Choose a reason for hiding this comment

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

(no action needed) Initially, I thought it was curious that we don't have to write mut self in the signature, but since the function takes ownership, it is implied that 'self' can be mutated. Is this understanding correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind of. The mut in x: mut self is a property of the variable
binding, not the type. To demonstrate the orthogonality, consider:

let mut xs = vec![0, 1, 2];

let mut x: &mut i32 = &mut xs[0];
//  ^^^     ~~~

The first mut means that it’s legal to write x = &mut xs[1]:
reassigning to the variable binding. This is entirely up to the call
site, like const/let in JavaScript.

The second mut means that it’s legal to write *x = 0, mutating the
value behind the binding. This is a property of the type, like
ReadonlyArray/Array in TypeScript: if I give you a read-only array,
you can’t just choose to interpret it as an Array.

It can often be useful to have a mutable binding even if the value is at
an immutable reference type:

fn parse_number(mut buf: &str) -> Option<i32> {
    let mut sign = 1;
    if buf.starts_with('-') {
        sign = -1;
        buf = &buf[1..];
        //  ^^^^^^^^^^^  reassigns to `buf`, even though the underlying
        //               buffer's *contents* are immutable
    }
    Some(buf.parse::<i32>().ok()? * sign)
}

The mutability of the binding is always at the discretion of the caller,
and doesn’t affect types or APIs. The caller can change it at will:

let xs = vec![0, 1, 2];
// `xs.push(3)` is illegal here; `xs` is not `mut`

let mut ys = xs; // same value (moved, not copied); binding is now mutable
ys.push(3); // legal

let zs = ys;
// `zs.push(4)` is illegal again

The code in this PR is analogous. We bind self immutably, and we don’t
modify self. But in the Tensor clause of the match expression, we
bind tp mutably, so that we can call chunks_exact_mut.

So your claim that

since the function takes ownership, it is implied that 'self' can be mutated

is correct, in a way. It’s implied that self can be changed to a
mutable binding since the binding is always “owned” by the function.

@wchargin
Copy link
Contributor Author

neat use of chunks_exact_mut

Yeah; chunks_exact_mut and chunks_mut (which gives you the partial
remainder of the last chunk rather than dropping it) are super cozy.
I often find myself wishing that Python’s itertools had an equivalent.
The Rust iterator API provides a lot of nice methods, similar to Java’s
streams, and often they’re just what’s wanted.

Base automatically changed from wchargin-rust-blob-sequence-metadata to master January 19, 2021 20:49
wchargin-branch: rust-audio
wchargin-source: 3d9f38cedb09614ca1aeb63469d8e93a7be032ce

# Conflicts:
#	tensorboard/data/server/data_compat.rs
wchargin-branch: rust-audio
wchargin-source: 3d9f38cedb09614ca1aeb63469d8e93a7be032ce
@wchargin wchargin merged commit c0a649a into master Jan 19, 2021
@wchargin wchargin deleted the wchargin-rust-audio branch January 19, 2021 21:14
@wchargin wchargin mentioned this pull request Jan 19, 2021
34 tasks
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/... plugin:audio

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants