Rust SDK: introduce `MsgSender` builder-like interface for logging data #1037

teh-cmc · 2023-02-01T17:01:53Z

Introduce MsgSender, a builder-like API for easily & safely logging components in Rust
- The "safely" part is the important / tricky one as the Rust SDK has full access to the raw data model
- Update raw_mesh example to use these new APIs
Introduce SerializableComponent trait
Add crystal clear (hopefully) nb_components, nb_rows & nb_instances helpers where required

Summary listing of important TODOs for my later self when I'll start opening issues:

// TODO(cmc): remove traces of previous APIs & examples

// TODO(cmc): if this is an InstanceKey and it contains u64::MAX, fire IllegalInstanceKey.

// TODO(cmc): The sanity checks we do in here can (and probably should) be done in
// `MsgBundle` instead so that the python SDK benefits from them too... but one step at a
// time.

// TODO(cmc): The Rust SDK needs a higher-level `init()` method, akin to what the python SDK
// does... which they can probably share.
// This needs to take care of the whole `official_example` thing, and also keeps track of
// whether we're using the rust or python sdk.

// TODO(cmc): arg parsing and arg interpretation helpers
// TODO(cmc): missing flags: save, serve
// TODO(cmc): expose an easy to use async local mode.

crates/re_log_types/src/msg_bundle.rs

jondo2010 · 2023-02-06T14:38:56Z

crates/rerun_sdk/src/msg_sender.rs

+///         .map_err(Into::into)
+/// }
+/// ```
+pub struct MsgSender {


Maybe MsgBuilder, or LogBuilder?

It used to be called that way, but really it cannot give you back a message, all it can do is send one (more than one actually because the message is likely to get splitted into parts internally)

jondo2010 · 2023-02-06T14:40:37Z

crates/rerun_sdk/src/msg_sender.rs

+
+    /// All the different timestamps for this message.
+    ///
+    /// The logging time is automatically inserted during creation ([`Self::new`]).


Do we want log time set with new(), or send()? What if they create a builder and hang on to it for a while before calling send()?

There's no go solution besides logging and/or making it configurable; but really in practice this won't matter for the foreseeable future as there's really not any reason to hold on to one of these as I can think of.

crates/rerun_sdk/src/msg_sender.rs

jondo2010 · 2023-02-06T15:13:51Z

crates/rerun_sdk/src/msg_sender.rs

+    }
+}
+
+fn bundle_from_iter<'a, C: SerializableComponent>(


Why not use ComponentBundle::try_from?

We don't have such an impl and I cannot define it due to orphan rules 😞

I'll leave the commented out impl there for future reference.

examples/raw_mesh/src/main.rs

emilk · 2023-02-06T15:33:53Z

crates/re_log_types/src/msg_bundle.rs

+
+    /// Returns the number of _instances_ for a given `row` in the bundle, i.e. the length of a
+    /// specific row within the bundle.
+    pub fn nb_instances(&self, row: usize) -> Option<usize> {


This is a bit of a style bikeshed, but I find num_instances a lot more clear, and my wild guess, with zero data to back it up, is that it is less likely to confuse others.

I'll make a note to unify our code styles here at some point (currently conducting a twitter poll to gauge popular opinion)

Oh yeah I never actually thought about it until now; it's a very french thing actually 🥲

I'll keep it as-is for now because it's consistent with how I've written it everywhere in the store, but I definitely think this warrants a big s/nb_/num_ PR.

emilk · 2023-02-06T15:35:05Z

crates/re_arrow_store/src/store_write.rs

-                .iter()
-                .map(|bundle| bundle.name)
-                .all_unique(),
+            msg.components.iter().map(|bundle| bundle.name).all_unique(),


it's a bit weird to refer both to msg.components and to its alias bundles

TL;DR: this is all in dire need of a terminology pass; at the moment I don't know of any good solution here.

msg.components.iter().map(|components| ...) is technically correct but plain confusing.

msg.components.iter().map(|component| ...) looks like you'd expect but is plain wrong.

msg.components.iter().map(|bundle| ...) looks awful but at least it makes you look twice, which you should because there's more going on than meets the eye.

The root of the issue is that msg.components is too vague: this is in fact an array (component types) of arrays (rows) of actual components (instances/columns).

I guess we'll clean all of this when we implement support for batch insertions.

emilk · 2023-02-06T15:35:57Z

crates/re_arrow_store/src/store_write.rs

        // TODO(cmc): kind & insert_id need to somehow propagate through the span system.
        self.insert_id += 1;

        let MsgBundle {
            msg_id,
            entity_path: ent_path,
            time_point,
-            components,
-        } = bundle;
+            components: bundles,


isn't components a more descriptive name?

Not really, see above.

examples/raw_mesh/src/main.rs

emilk · 2023-02-06T15:45:04Z

examples/raw_mesh/src/main.rs

+    /// If specified, connects and sends the logged data to a remote Rerun viewer.
+    #[clap(long)]
+    #[allow(clippy::option_option)]
+    addr: Option<Option<String>>,


haha, that's… interesting

🙃 I guess I should rename it to connect though, that would flow more naturally

yeah, agree

crates/rerun_sdk/src/msg_sender.rs

teh-cmc force-pushed the cmc/rust_sdk_msg_builder branch 8 times, most recently from 527eb79 to 6c1b5fc Compare February 5, 2023 22:30

teh-cmc added 4 commits February 6, 2023 09:36

fixing the whole nb_components/nb_rows/nb_instances situation

b6b35c6

introduce SerializableComponent trait

49dff86

implement MsgSender API

9ff9882

port raw_mesh example to MsgSender API

060dc01

teh-cmc force-pushed the cmc/rust_sdk_msg_builder branch from 6c1b5fc to 060dc01 Compare February 6, 2023 08:38

teh-cmc added 4 commits February 6, 2023 10:14

clean up raw_mesh example

15b669f

clean up SDK imports

58b205c

sanity checks and tests

b04cee1

Merge branch 'main' into cmc/rust_sdk_msg_builder

048520c

teh-cmc changed the title ~~rust SDK: builder-like interface for logging data~~ Rust SDK: introduce MsgSender builder-like interface for logging data Feb 6, 2023

teh-cmc added 2 commits February 6, 2023 11:52

formatters & linters

4613d96

talk about vicious

257470d

teh-cmc marked this pull request as ready for review February 6, 2023 11:34

teh-cmc mentioned this pull request Feb 6, 2023

Rust SDK: remove deprecated APIs & examples #1107

Merged

jondo2010 self-requested a review February 6, 2023 13:46

jondo2010 reviewed Feb 6, 2023

View reviewed changes

crates/re_log_types/src/msg_bundle.rs Show resolved Hide resolved

jondo2010 reviewed Feb 6, 2023

View reviewed changes

crates/rerun_sdk/src/msg_sender.rs Outdated Show resolved Hide resolved

jondo2010 reviewed Feb 6, 2023

View reviewed changes

crates/rerun_sdk/src/msg_sender.rs Show resolved Hide resolved

jondo2010 reviewed Feb 6, 2023

View reviewed changes

jondo2010 approved these changes Feb 6, 2023

View reviewed changes

teh-cmc commented Feb 6, 2023

View reviewed changes

examples/raw_mesh/src/main.rs Outdated Show resolved Hide resolved

emilk reviewed Feb 6, 2023

View reviewed changes

teh-cmc commented Feb 6, 2023

View reviewed changes

examples/raw_mesh/src/main.rs Show resolved Hide resolved

jondo2010 mentioned this pull request Feb 6, 2023

Better ComponentBundle invariants #1112

Merged

3 tasks

emilk reviewed Feb 6, 2023

View reviewed changes

crates/rerun_sdk/src/msg_sender.rs Outdated Show resolved Hide resolved

teh-cmc added 2 commits February 6, 2023 22:04

Merge remote-tracking branch 'origin/main' into cmc/rust_sdk_msg_builder

43c0b5e

addressed PR comments

6403be7

teh-cmc merged commit f3dc81a into main Feb 6, 2023

teh-cmc deleted the cmc/rust_sdk_msg_builder branch February 6, 2023 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust SDK: introduce `MsgSender` builder-like interface for logging data #1037

Rust SDK: introduce `MsgSender` builder-like interface for logging data #1037

teh-cmc commented Feb 1, 2023 •

edited

jondo2010 Feb 6, 2023 •

edited

teh-cmc Feb 6, 2023

jondo2010 Feb 6, 2023 •

edited

teh-cmc Feb 6, 2023

jondo2010 Feb 6, 2023

teh-cmc Feb 6, 2023

emilk Feb 6, 2023

emilk Feb 6, 2023

teh-cmc Feb 6, 2023

emilk Feb 6, 2023

teh-cmc Feb 6, 2023

emilk Feb 6, 2023

teh-cmc Feb 6, 2023

emilk Feb 6, 2023

teh-cmc Feb 6, 2023

emilk Feb 7, 2023

Rust SDK: introduce MsgSender builder-like interface for logging data #1037

Rust SDK: introduce MsgSender builder-like interface for logging data #1037

Conversation

teh-cmc commented Feb 1, 2023 • edited

jondo2010 Feb 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jondo2010 Feb 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rust SDK: introduce `MsgSender` builder-like interface for logging data #1037

Rust SDK: introduce `MsgSender` builder-like interface for logging data #1037

teh-cmc commented Feb 1, 2023 •

edited

jondo2010 Feb 6, 2023 •

edited

jondo2010 Feb 6, 2023 •

edited