Replace `Session` with `RecordingContext` #1983

teh-cmc · 2023-04-26T22:10:14Z

Session has been replaced by RecordingStream, and SessionBuilder with RecordingStreamBuilder.

This introduces RecordStream, which we've all talked about in leeeeeeength this afternoon so I won't dwell on it too much.

A RecordStream is in charge of the entire data pipeline for a single recording, and comes with well-defined ordering, durability and multithreading guarantees/semantics.

RecordStream replaces the previous Session stuff in the Rust SDK, and will become the basis of the Python SDK too in the next PR.

Try it!

api_demo on row at a time:

RERUN_FLUSH_TICK_SECS=9999999999 RERUN_FLUSH_NUM_BYTES=9999999999 RERUN_FLUSH_NUM_ROWS=0 cargo r -p api_demo

api_demo in one big single packet:

RERUN_FLUSH_TICK_SECS=9999999999 RERUN_FLUSH_NUM_BYTES=9999999999 RERUN_FLUSH_NUM_ROWS=9999999999 cargo r -p api_demo

TODO:

update https://www.rerun.io/docs/reference/control-logging with new batching vars
open issue for estimated_bytes_size going crazy in some instances...

On top of #1980
Part of #1619

Future work:

BeginRecordingMsg is a misnomer #1996

crates/re_sdk/src/msg_sender.rs

crates/rerun/src/clap.rs

crates/re_sdk/src/recording_stream.rs

…vamp

jleibs · 2023-04-28T16:45:27Z

crates/re_data_store/src/log_db.rs

@@ -61,7 +61,7 @@ impl EntityDb {
        let mut table = DataTable::from_arrow_msg(msg)?;
        table.compute_all_size_bytes();

-        // TODO(#1619): batch all of this
+        // TODO(cmc): batch all of this


Oooh.... even more performance gains to be had?

Yeah, we only batch during transport at the moment, everything else happens on a row-by-row basis.

Though I'm not sure it's ever going to be worth it in this instance: implementing the write path in terms of individual rows is so flexible, simple and easy to extend... and things are already so fast even with just the transport-level batching...

jleibs · 2023-04-28T17:16:02Z

crates/re_sdk/src/log_sink.rs

+    /// Only applies to sinks that maintain a backlog.
+    #[inline]


The fact that all sinks implement this but don't support it strikes me as awkward. It is possible to be explicit about tracking when our Sink is a BufferedSink and only implement drain_backlog there?

At a minimum maybe signature should be Option<Vec<LogMsg>> to disambiguate between "The sink doesn't support a backlog" and "The backlog was empty."

I mean sure we could return an option but I don't see how that'd be an improvement? We can always add it later on if we ever need to disambiguate between no backlog vs. empty backlog, but there's no such need at the moment.

if we ever need to disambiguate between no backlog vs. empty backlog

Yeah, we can talk about this more concretely in a future context. I'm thinking about cases like: "I tried to use a Tcp sink, but the tcp end-point wasn't there, so now instead I try to switch my sink to a FileSink, expecting it to drain the backlog of buffered tcp messages, but even though the tcp sink told me there was no backlog, I actually just lost my data"

crates/re_sdk/src/log_sink.rs

crates/rerun/src/clap.rs

jleibs · 2023-04-28T17:41:35Z

crates/re_sdk/src/recording_stream.rs

+    ///
+    /// This can be used to then construct a [`RecordingStream`] manually using
+    /// [`RecordingStream::new`].
+    pub fn finalize(self) -> (bool, RecordingInfo, DataTableBatcherConfig) {


Logically, returning enabled from the RecordingStreamBuilder feels a bit out of place to me.

Actually the fact that this doesn't return a RecordingStream at all is even more unexpected... maybe this struct should be named something like RecordstringStreamConstructionArgs (but shorter)?

I don't know, I'm not sure adding yet another type will help... The doc already makes it pretty clear what's going on here.

Also this code is just a copy-paste of the old SessionBuilder::finalize code which never really gave us any problems 🤷

jleibs · 2023-04-28T17:51:12Z

crates/re_sdk/src/recording_stream.rs

+pub enum RecordingStreamError {
+    /// Error within the underlying file sink.
+    #[error("Failed to create the underlying file sink: {0}")]
+    FileSink(#[from] re_log_encoding::FileSinkError),


No need to fix this now but curious how we would make this Sink-agnostic for extensibility reasons in the future? Maybe we don't need extensible anyways so I'm over-thinking it.

Something like

/// Error within the underlying file sink. #[error("Failed to create the underlying file sink: {0}")] Sink(#[from] Box<dyn std::error::Error + Send + Sync>),

would do the job, wouldn't it?

Yeah, something along those lines.

crates/re_sdk/src/recording_stream.rs

…vamp

…_revamp

teh-cmc · 2023-05-03T21:11:44Z

Updated with some (hopefully) clarifications

crates/re_sdk/src/recording_stream.rs

jleibs

Looks good!

Co-authored-by: Jeremy Leibs <jeremy@rerun.io>

teh-cmc changed the title ~~SDK batching/revamp 1: introduce RecordingContext & revamp Rust SDK~~ SDK batching/revamp 2: introduce RecordingContext & revamp Rust SDK Apr 26, 2023

teh-cmc mentioned this pull request Apr 26, 2023

SDK batching/revamp 1: impl DataTableBatcher #1980

Merged

teh-cmc added 🦀 Rust API Rust logging API do-not-merge Do not merge this PR labels Apr 26, 2023

teh-cmc changed the base branch from main to cmc/sdk_revamp/1_batcher April 26, 2023 22:11

teh-cmc force-pushed the cmc/sdk_revamp/2_rust_revamp branch 3 times, most recently from fb1ea44 to 36dc901 Compare April 26, 2023 22:17

teh-cmc changed the title ~~SDK batching/revamp 2: introduce RecordingContext & revamp Rust SDK~~ SDK batching/revamp 2: introduce RecordingContext / sunset Rust's Session Apr 26, 2023

teh-cmc force-pushed the cmc/sdk_revamp/2_rust_revamp branch 3 times, most recently from ac0ce2e to a3ed552 Compare April 27, 2023 09:07

teh-cmc added 10 commits April 27, 2023 11:11

version crossbeam at the workspace level

68d8b0c

more DataRow size helpers

cb74038

DataTableBatcher

a0d9d39

lints

f46ac72

lints

5440f76

self review

c1088c5

don't expose shutdown to make errors impossible

cbf17be

doc

e7b42bf

backport

573de98

backport

67dc616

teh-cmc force-pushed the cmc/sdk_revamp/1_batcher branch from eba29b7 to 67dc616 Compare April 27, 2023 09:11

teh-cmc force-pushed the cmc/sdk_revamp/2_rust_revamp branch from a3ed552 to ac1fac2 Compare April 27, 2023 09:12

teh-cmc changed the title ~~SDK batching/revamp 2: introduce RecordingContext / sunset Rust's Session~~ SDK batching/revamp 2: introduce RecordingContext / sunset Session Apr 27, 2023

teh-cmc mentioned this pull request Apr 27, 2023

SDK batching/revamp 3: sunset PythonSession #1985

Merged

6 tasks

introduce RecordingStream

14130c5

teh-cmc force-pushed the cmc/sdk_revamp/2_rust_revamp branch from ac1fac2 to 14130c5 Compare April 27, 2023 16:30

teh-cmc commented Apr 27, 2023

View reviewed changes

clean up old stuff from the before time

71a31bd

even more tests

72685fa

teh-cmc removed the do-not-merge Do not merge this PR label Apr 28, 2023

teh-cmc marked this pull request as ready for review April 28, 2023 08:13

teh-cmc added 3 commits April 28, 2023 10:40

rogue todo

067168f

batching is now a reality

b8e0065

some extra peace of mind

0e69707

This was referenced Apr 28, 2023

SDK batching/revamp 2.1: clock example for Rust #2000

Merged

SDK batching/revamp 2.2: homegrown arrow size estimation routines #2002

Merged

teh-cmc added 4 commits April 28, 2023 17:55

revert

a7f84c8

Merge branch 'main' into cmc/sdk_revamp/1_batcher

ead5883

Merge branch 'cmc/sdk_revamp/1_batcher' into cmc/sdk_revamp/2_rust_re…

3f0ec73

…vamp

lock shenanigans

6e348db

jleibs reviewed Apr 28, 2023

View reviewed changes

crates/re_sdk/src/log_sink.rs Outdated Show resolved Hide resolved

jleibs reviewed Apr 28, 2023

View reviewed changes

crates/rerun/src/clap.rs Show resolved Hide resolved

jleibs reviewed Apr 28, 2023

View reviewed changes

teh-cmc added 2 commits May 3, 2023 10:30

Merge branch 'main' into cmc/sdk_revamp/1_batcher

a31285b

Merge branch 'cmc/sdk_revamp/1_batcher' into cmc/sdk_revamp/2_rust_re…

ecb7ce5

…vamp

Base automatically changed from cmc/sdk_revamp/1_batcher to main May 3, 2023 20:11

teh-cmc added 3 commits May 3, 2023 22:28

Merge remote-tracking branch 'origin/main' into cmc/sdk_revamp/2_rust…

8580773

…_revamp

merge shenanigans

4af3342

address PR comments

d1e5c19

jleibs reviewed May 4, 2023

View reviewed changes

crates/re_sdk/src/recording_stream.rs Outdated Show resolved Hide resolved

jleibs approved these changes May 4, 2023

View reviewed changes

typos

c121325

Co-authored-by: Jeremy Leibs <jeremy@rerun.io>

teh-cmc merged commit b2b3064 into main May 4, 2023

teh-cmc deleted the cmc/sdk_revamp/2_rust_revamp branch May 4, 2023 09:16

emilk changed the title ~~SDK batching/revamp 2: introduce RecordingContext / sunset Session~~ Replace Session with RecordingContext May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `Session` with `RecordingContext` #1983

Replace `Session` with `RecordingContext` #1983

teh-cmc commented Apr 26, 2023 •

edited by emilk

Loading

jleibs Apr 28, 2023

teh-cmc May 3, 2023

jleibs Apr 28, 2023

teh-cmc May 3, 2023 •

edited

Loading

jleibs May 3, 2023

jleibs Apr 28, 2023

jleibs Apr 28, 2023

teh-cmc May 3, 2023

jleibs Apr 28, 2023

teh-cmc May 3, 2023

jleibs May 3, 2023

teh-cmc commented May 3, 2023

jleibs left a comment

Replace Session with RecordingContext #1983

Replace Session with RecordingContext #1983

Conversation

teh-cmc commented Apr 26, 2023 • edited by emilk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teh-cmc May 3, 2023 • edited Loading

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

teh-cmc commented May 3, 2023

jleibs left a comment

Choose a reason for hiding this comment

Replace `Session` with `RecordingContext` #1983

Replace `Session` with `RecordingContext` #1983

teh-cmc commented Apr 26, 2023 •

edited by emilk

Loading

teh-cmc May 3, 2023 •

edited

Loading