rust: implement tensor RPCs and client #4726
Conversation
wchargin
left a comment
wchargin
left a comment
There was a problem hiding this comment.
LGTM—a couple passing comments, but no suggestions.
| for (step, wall_time, value) in points { | ||
| steps.push(step.into()); | ||
| wall_times.push(wall_time.into()); | ||
| // Clone the TensorProto to get a copy to send in the response. |
There was a problem hiding this comment.
Cool, this seems fine. If we wanted to be really tight with the locks,
we could change the commit’s tensor values to hold Arc<TensorProto>s
and then clone them only after dropping the lock—and maybe one day Prost
and Tonic might learn about Arc<_>s and remove the need to clone
at all. But certainly no action required.
There was a problem hiding this comment.
Yeah, I realize it's not exactly ideal to be cloning this here, but SGTM to defer further optimization for now. I'd found myself wondering whether TensorProto was even the right type for the commit to hold in the first place (since we're forcing it into an ndarray ultimately, it could be slimmed down quite a bit, and maybe made more ergonomic to manipulate). If we revisit that, we could also consider reference counting to avoid the clone-inside-critical-section here.
tensorboard/data/server/server.rs
Outdated
|
|
||
| #[tokio::test] | ||
| async fn test_read_tensors() { | ||
| fn make_string_tensor_proto<T: Into<Bytes>>(value: T) -> pb::TensorProto { |
There was a problem hiding this comment.
This is perfectly fine; it’s also idiomatic to write
fn make_string_tensor_proto(value: impl Into<Bytes>) -> ...using an existential instead of a type parameter. These are mostly
equivalent; I don’t have a strong preference here.
There was a problem hiding this comment.
Done; thanks for the tip.
2 participants
Implements RPC-level support for the tensor data class in Rustboard (part of #4422). In particular we include tensor time series when generating the
ListPluginsresponse, and we wire upListTensorsandReadTensorson both the server and client. The latter RPCs are almost identical to the scalars ones; the only nontrivial differences involve handling the actualTensorProtovalues, where we have to explicitly clone them out of theCommiton the server side, and convert them into numpy arrays on the client side.This doesn't yet render any data on its own, since nothing populates the
tensorsfields in the commit (which were introduced by #4710), but with coming PRs that implement the population side, I've tested end-to-end that this correctly renders the expected dashboards. (There are also unit tests for server and client modeled on those for scalars.)