data_provider: add GetExperiment RPC
#4750
Conversation
Summary: The [`prost-types`] crate provides bindings for the [well-known types], notably including `google.protobuf.Timestamp`. As with bindings for other languages, some of these types have additional methods, like `Timestamp::normalize`. [`prost-types`]: https://crates.io/crates/prost-types [well-known types]: https://developers.google.com/protocol-buffers/docs/reference/google.protobuf Test Plan: It builds: `bazel build //tensorboard/data/server/cargo:prost_types`. wchargin-branch: rust-dep-prost-types wchargin-source: 6cdfa10ead9892fd794050779434b42ea9ce8b1a
Summary: This new RPC is intended to back both the `data_location` and the `experiment_metadata` data provider methods (which really should be combined into one). It takes an experiment ID and returns all available metadata. If needed, we could add a fieldmask, but we leave that out until we have a need for it. The Rust implementation of the data server will only populate the data location field, but we still include the rest to mirror the Python interface. This is the first use of `google.protobuf.Timestamp` or any of the well-known types, so some build tweaks are needed. Test Plan: The protos and Rust code all build, and the newly updated instructions in `DEVELOPMENT.md` for still work. With `grpc_cli`, the new RPC can be seen to successfully return a non-OK `"not yet implemented"` status. wchargin-branch: data-getexperiment-rpc wchargin-source: 0ae0da093f373565cb431620329bdc102d3c8336
| // Time that the experiment was created. | ||
| // | ||
| // This does not necessarily have any relation to times of events within an | ||
| // experiment. A new experiment may contain old data, or data may be freshly | ||
| // written to an experiment after it is created. | ||
| // | ||
| // May be unset if no creation time is known. | ||
| google.protobuf.Timestamp creation_time = 4; |
There was a problem hiding this comment.
@nfelt: Could you take a quick look at this use of Timestamp in light
of the build changes in this PR and its diffbase, #4749? I recall that
in #4314 you lobbied for using Timestamp rather than double, and in
this case I agree that it’s the right tradeoff as far as the proto
definition goes. It incurs a bit of build complexity; IMHO it’s worth
it, but I would also not mind just using double if we prefer that.
There was a problem hiding this comment.
I'm ok with the build complexity since it seems fairly contained, although boy do I wish com_google_protobuf were a nicer structured Bazel dependency to have.
tensorboard/data/server/BUILD
Outdated
| done | ||
| [ -n "$${f}" ] || die 'no well-known protos in fileset' | ||
| dir="$$(dirname "$${f}")" | ||
| while true; do |
There was a problem hiding this comment.
This while loop feels like a bit of a complicated way to effectively just remove the suffix /src/.*? and replace it with /src. Maybe consider just doing string munging for that?
There was a problem hiding this comment.
I hadn’t realized that ${f%/src/*} was a sufficient building block
here. Much nicer; thanks.
| // Time that the experiment was created. | ||
| // | ||
| // This does not necessarily have any relation to times of events within an | ||
| // experiment. A new experiment may contain old data, or data may be freshly | ||
| // written to an experiment after it is created. | ||
| // | ||
| // May be unset if no creation time is known. | ||
| google.protobuf.Timestamp creation_time = 4; |
There was a problem hiding this comment.
I'm ok with the build complexity since it seems fairly contained, although boy do I wish com_google_protobuf were a nicer structured Bazel dependency to have.
| import "tensorboard/compat/proto/summary.proto"; | ||
| import "tensorboard/compat/proto/tensor.proto"; | ||
|
|
||
| option go_package = "github.com/tensorflow/tensorboard/tensorboard/data/proto/data_provider_proto"; | ||
|
|
||
| service TensorBoardDataProvider { | ||
| // Get metadata about an experiment. |
There was a problem hiding this comment.
I don't object to evolving this interface a little away from Python DataProvider but FWIW we should anticipate that we might want to change this further depending on the outcome of experiment-ifying Python DataProvider as part of upcoming work.
There was a problem hiding this comment.
Yeah. I don’t mind it at all, since this moves it closer to what the
Python data provider should be. Happy to keep this interface malleable.
wchargin-branch: data-getexperiment-rpc wchargin-source: 075d525b0287c870d6cbb3ece1b61435c6b13d13
wchargin-branch: data-getexperiment-rpc wchargin-source: 161842c20de3592839dbd9684e397d8b0dcfb7f4
wchargin-branch: data-getexperiment-rpc wchargin-source: 161842c20de3592839dbd9684e397d8b0dcfb7f4
Summary:
This new RPC is intended to back both the
data_locationand theexperiment_metadatadata provider methods (which really should becombined into one). It takes an experiment ID and returns all available
metadata. If needed, we could add a fieldmask, but we leave that out
until we have a need for it.
The Rust implementation of the data server will only populate the data
location field, but we still include the rest to mirror the Python
interface.
This is the first use of
google.protobuf.Timestampor any of thewell-known types, so some build tweaks are needed.
Test Plan:
The protos and Rust code all build, and the newly updated instructions
in
DEVELOPMENT.mdfor still work. Withgrpc_cli, the new RPC can beseen to successfully return a non-OK
"not yet implemented"status.wchargin-branch: data-getexperiment-rpc