rust: implement ListRuns RPC

[wchargin]

Summary:
This patch connects the //tensorboard/data/server main module to the
Commit and LogdirLoader infrastructure, and implements ListRuns as
a proof of concept.

For now, we just take a logdir path as a positional command line
argument. A future patch will bring nicer command-line argument parsing.

Test Plan:
Unit tests included. Note that testing these data provider methods is
nice and easy: you just call them. No mocking or networking required.

For a manual grpc_cli test, start just the data server, and run:

$ grpc_cli --channel_creds_type=insecure \
>   --protofiles tensorboard/data/proto/data_provider.proto \
>   call localhost:6806 TensorBoardDataProvider.ListRuns '' 2>/dev/null
runs {
  name: "lr_1E-03,conv=1,fc=2"
  start_time: 1563406327
}
// etc.

For an end-to-end test, cherry-pick #4356, then run both the data server
and a copy of TensorBoard concurrently:

bazel run //tensorboard/data/server -- ~/tensorboard_data/mnist &
bazel run //tensorboard -- \
    --bind_all \
    --generic_data true \
    --grpc_data_provider localhost:6806 \
    ;

Then navigate to localhost:6006/data/runs and note that the response
includes a correct list of runs.

wchargin-branch: rust-listruns

[wchargin]

Cf.: #4387 for grpc_cli tips; #4356 to cherry-pick in if you want to
run the end-to-end test.

[nfelt]

I gather the point of this line is to be future-proof in case we add more fields to Run? Is there any mechanism that enforces this by default or is just a convention we'll need to uphold ourselves?

[wchargin]

Yeah—specifically, so that proto updates that add new fields don’t
introduce compilation errors.

I was considering adding a tiny “style guide” to DEVELOPMENT.md with
things like this, so I’ll probably go ahead and do that.

There is a #[non_exhaustive] attribute that you can add to structs to
indicate that they may have more fields later, preventing callers
outside the crate from initializing them directly but still permitting
them to access public fields. This doesn’t suffice because (a) we’re in
the same crate (though we could change that) and (b) it doesn’t permit
functional record updates (..Default::default()):

error[E0639]: cannot create non-exhaustive struct using struct expression
 --> main.rs:3:13
  |
3 |       let s = sub::S {
  |  _____________^
4 | |         a: 1,
5 | |         b: 2,
6 | |         ..Default::default()
7 | |     };
  | |_____^

The RFC threads have a lot of comments, and from skimming them it’s not
obvious to me whether this was “intended, by design” or merely “artifact
of how #[non_exhaustive] desugars to a hidden private field”. (It was
definitely known at the time.)

Another potential check for this would be in Clippy: “lint if you
construct a value at a type that implements prost::Message without
using a functional record update”. There is no such check currently.

[nfelt]

Reading through the next PR, it's really rather noisy to have the ..Default::default() everywhere we create a proto, especially in tests :/ Given the fact that we control these protos ourselves, I wonder if on net maybe it's easier for places where we don't expect to add fields to just relax this and accept that we'll need to make modifications to the code if we update the proto?

The other alternative that came to mind would be to initialize structs to default-empty, then mutate to populate fields; needing mutation is a downside although I think in some of the more convoluted multi-layer struct cases this may be significantly more concise.

I can see why neither of these is ideal, it just feels distracting IMO to have every single proto struct need the extra syntax. Would be nice if there was some way to specify on the struct definition that they can be initialized a subset of fields if the rest are Default (basically like optional arguments), or if at least maybe the functional update syntax supported plain .. as sugar for ..Default::default().

[wchargin]

Just seeing this. For posterity, further discussion at #4399.

[nfelt]

I'll leave it up to you (and would also be fine to do in a follow up or punt for now), but I try to avoid data fixtures unless it's really pretty complicated or expensive to set up the right data. In this case it seems like it wouldn't be so bad to 1) create a commit, 2) explicitly populate it with the data we want via test helpers, and then 3) wrap a handler around it and do the assertions. I just find the assertions to be a lot more meaningful when I can see the data they depend on in the same test method vs having to go dig up the fixture.

[wchargin]

Okay; noted, thanks!. I’m certainly sympathetic to this. I think I might
punt it for now since a shared fixture suffices for this set of changes.
But I’m happy to go back and reswizzle things after that.

[wchargin]

Done in #4433.