Skip to content

rust: add data_compat module #4340

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.

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

Merged
merged 2 commits into from
Nov 19, 2020
Merged

rust: add data_compat module #4340

merged 2 commits into from
Nov 19, 2020

Conversation

wchargin
Copy link
Contributor

Summary:
This module performs conversions from legacy on-disk data formats. It
will correspond to both the data_compat and dataclass_compat modules
from Python TensorBoard. For now, we have just one function, to
determine the initial summary metadata of a time series. And, for now,
we only implement this for scalars, to keep things simple.

Test Plan:
Unit tests included.

wchargin-branch: rust-data-compat

Summary:
This module performs conversions from legacy on-disk data formats. It
will correspond to both the `data_compat` and `dataclass_compat` modules
from Python TensorBoard. For now, we have just one function, to
determine the initial summary metadata of a time series. And, for now,
we only implement this for scalars, to keep things simple.

Test Plan:
Unit tests included.

wchargin-branch: rust-data-compat
wchargin-source: ef510b422b00a66a23efc1406947844fa8d1a798
@wchargin wchargin added the core:rustboard //tensorboard/data/server/... label Nov 17, 2020
@wchargin wchargin requested a review from psybuzz November 17, 2020 21:40
@google-cla google-cla bot added the cla: yes label Nov 17, 2020
wchargin-branch: rust-data-compat
wchargin-source: 467974fa4e0e55dea0872bb1394a768a38811ac5
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.

Code and tests lgtm

match (md, value) {
// 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),
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I did not know that the if <condition> could be part of the arm's matching pattern (on the left side of the =>)! The examples in the Rust reference / book are less advanced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! This is called a match guard. It is occasionally quite useful
when you want to continue to match multiple kinds of other patterns when
the guard fails, as in this example.

}
}

mod unknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so these mods are used like describe(... blocks in Jasmine in JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I’m using them as such, since they show up in the output like this:

test data_compat::tests::scalars::test_tf1x_simple_value ... ok
test data_compat::tests::scalars::test_tf2x_scalar_tensor_without_dataclass ... ok
test data_compat::tests::unknown::test_custom_plugin_with_dataclass ... ok
test data_compat::tests::unknown::test_empty ... ok
test data_compat::tests::unknown::test_unknown_plugin_no_dataclass ... ok

I haven’t done this before and don’t have strong opinions about it, but
it seemed pretty reasonable, so I figured that I’d try it.

Copy link
Contributor Author

@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.

thanks!

match (md, value) {
// 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),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! This is called a match guard. It is occasionally quite useful
when you want to continue to match multiple kinds of other patterns when
the guard fails, as in this example.

}
}

mod unknown {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I’m using them as such, since they show up in the output like this:

test data_compat::tests::scalars::test_tf1x_simple_value ... ok
test data_compat::tests::scalars::test_tf2x_scalar_tensor_without_dataclass ... ok
test data_compat::tests::unknown::test_custom_plugin_with_dataclass ... ok
test data_compat::tests::unknown::test_empty ... ok
test data_compat::tests::unknown::test_unknown_plugin_no_dataclass ... ok

I haven’t done this before and don’t have strong opinions about it, but
it seemed pretty reasonable, so I figured that I’d try it.

@wchargin wchargin merged commit d354c76 into master Nov 19, 2020
@wchargin wchargin deleted the wchargin-rust-data-compat branch November 19, 2020 01:56
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