SDK `DataLoader`s 5: customizable (external) loaders for Rust #5351

teh-cmc · 2024-02-29T11:53:33Z

Adds new RecommendedLoadSettings that gets passed to all DataLoaders -- builtin and external -- in order to customize their behaviors.

This includes:

A recommended recording ID
The ID of the currently opened recording in the viewer (not implemented)
- Related: Send currently opened app & recording ID to data-loaders #5350
A recommended entity path prefix
A recommended timepoint

cargo r -p rerun-loader-rust-file -- run_wasm/src/main.rs  --recording-id this-one --entity-path-prefix a/b/c  --time sim_time=1000 --time wall_time=1709204046 --sequence sim_frame=42 | rerun -

Checks:

external loader ran manually (loader | rerun)
external loader via rerun (rerun xxx.rs)
log_file with external loader (log_file xxx.rs)

Part of series of PR to expose configurable DataLoaders to our SDKs:

Checklist

I have read and agree to Contributor Guide and the Code of Conduct
I've included a screenshot or gif (if applicable)
I have tested the web demo (if applicable):
- Using newly built examples: app.rerun.io
- Using examples from latest main build: app.rerun.io
- Using full set of examples from nightly build: app.rerun.io
The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
If applicable, add a new check to the release checklist!

Exposes raw `DataLoader`s to the Python SDK through 2 new methods: `RecordingStream.log_file_from_path` & `RecordingStream.log_file_from_contents`. Everything streams asynchronously, as expected. There's no way to configure the behavior of the `DataLoader` at all, yet. That comes next. ```python rr.log_file_from_path(filepath) ``` ![image](https://github.com/rerun-io/rerun/assets/2910679/0fe2d39c-f069-44a6-b836-e31001b3adaa) --- Part of series of PR to expose configurable `DataLoader`s to our SDKs: - #5327 - #5328 - #5330 - #5337 - #5351 - #5355

Wumpf

the "would you kindly" nature of the arg struct is annoying me a bit, but what can we do other than "handling whatever real users throw at it" :) 🤷

Wumpf · 2024-02-29T14:26:38Z

examples/rust/external_data_loader/src/main.rs

+fn timepoint_from_args(args: &Args) -> anyhow::Result<rerun::TimePoint> {
+    let mut timepoint = rerun::TimePoint::default();
+    {
+        for time_str in &args.time {
+            let Some((timeline_name, time)) = time_str.split_once('=') else {
+                continue;
+            };
+            timepoint.insert(
+                rerun::Timeline::new_temporal(timeline_name),
+                time.parse::<i64>()?.into(),
+            );
+        }
+
+        for seq_str in &args.sequence {
+            let Some((seqline_name, seq)) = seq_str.split_once('=') else {
+                continue;
+            };
+            timepoint.insert(
+                rerun::Timeline::new_sequence(seqline_name),
+                seq.parse::<i64>()?.into(),
+            );
+        }
+    }
+    Ok(timepoint)
+}


feels like somthing we may want to provide as a public utility? 🤔

I'd like to keep the whole external DataLoader API surface as small as possible for now -- we know we're going to break things a million times; I'd rather break less...

Wumpf · 2024-02-29T14:27:39Z

examples/rust/external_data_loader/src/main.rs

+    /// optional prefix for all entity paths
+    #[argh(option)]
+    entity_path_prefix: Option<String>,
+
+    /// optional mark data to be logged as timeless
+    #[argh(switch)]
+    timeless: bool,
+
+    /// optional timestamps to log at (e.g. `--time sim_time=1709203426`) (repeatable)
+    #[argh(option)]
+    time: Vec<String>,
+
+    /// optional sequences to log at (e.g. `--sequence sim_frame=42`) (repeatable)
+    #[argh(option)]
+    sequence: Vec<String>,


wondering if there's a way for data loader authors to "inherit" these arguments. Can clap work with nested arg strucs..?

Yes, we can do inheritance with clap (we already do so for connect, spawn, etc).

But again I want to keep this whole thing as small and quick-n-dirty as possible, at least for now -- the whole point is to show that's it's just a dumb CLI app and there's no magic involved and one could do the same using any language / CLI library.

Wumpf · 2024-02-29T14:31:37Z

crates/re_data_source/src/data_loader/mod.rs


 // ---

+/// Recommended settings for the [`DataLoader`].


I get the recommended method on this, but I'm confused/not convinced that this thing as a whole is "recommended settings". Why not just DataLoaderSettings and then each field elaborates what effect it expects (already the case)? I get that we're defensive here and want to make clear that a data loader that ignores the settings is still a valid data loader and these are all "would you kindly please"-arguments, but I find baking that into the name goes a bit too far and is more confusing than helping.

... anyways, not a hill I gonna die on so if you prefer this as is keep it :)

I wonder if there's something more we could write about the (lack of) consequences if a data loader ignores the args

Sure we can rename to DataLoaderSettings.

I wonder if there's something more we could write about the (lack of) consequences if a data loader ignores the args

We've already done so in a bunch of places, including the guide.

Wumpf · 2024-02-29T14:39:31Z

examples/rust/external_data_loader/src/main.rs

            rec = rec.recording_id(recording_id);
        };

        // The most important part of this: log to standard output so the Rerun Viewer can ingest it!
        rec.stdout()?
    };

-    rec.log_timeless(
-        rerun::EntityPath::from_file_path(&args.filepath),
+    // TODO(#3841): really need those send APIs


since this is example code to be consumed by users it would be nice(r) to write about what they can expect to improve here in the future inead :)

Exposes raw `DataLoader`s to the C++ SDK through 2 new methods: `RecordingStream::log_file_from_path` & `RecordingStream::log_file_from_contents`. Everything streams asynchronously, as expected. There's no way to configure the behavior of the `DataLoader` at all, yet. That comes next. :warning: I'm carrying bytes around in a `string_view` (see `rr_bytes`) and related, and the internet has been unhelpful to tell e whether I should or shouldn't. ```cpp rec.log_file_from_path(filepath); ``` ![image](https://github.com/rerun-io/rerun/assets/2910679/1933f08b-1b2b-483b-9576-ce2e0421cb62) --- Part of series of PR to expose configurable `DataLoader`s to our SDKs: - #5327 - #5328 - #5330 - #5337 - #5351 - #5355 --------- Co-authored-by: Andreas Reich <andreas@rerun.io>

Introduces `RecordingStream::clone_weak`, so `DataLoader` threads started from the SDK don't prevent the recording from flushing once `main` goes out of scope, all the while making sure that `DataLoader`s run to completion. ![image](https://github.com/rerun-io/rerun/assets/2910679/94bf7b16-cf9b-40ce-a190-d255328af3f8) - Mitigates #5335 --- Part of series of PR to expose configurable `DataLoader`s to our SDKs: - #5327 - #5328 - #5330 - #5337 - #5351 - #5355

Introduces the new `DataLoaderSettings` business to Python and update examples accordingly (`external_data_loader` & `log_file`). ```bash python examples/python/external_data_loader/main.py --recording-id this-one --entity-path-prefix a/b/c --time sim_time=1000 --time wall_time=1709204046 --sequence sim_frame=42 examples/python/dna/main.py | rerun - ``` ![image](https://github.com/rerun-io/rerun/assets/2910679/bfda567d-3d16-42cd-be8e-8b1a0767a784) Checks: - [x] external loader ran manually (`python loader | rerun`) - [x] external loader via rerun (`rerun xxx.py`) - [x] log_file with external loader (`log_file xxx.py`) --- Part of series of PR to expose configurable `DataLoader`s to our SDKs: - #5327 - #5328 - #5330 - #5337 - #5351 - #5355 - #5361

This makes the `log_file` APIs behave more like the standard `log` APIs; i.e. they inherit the state of their associated `RecordingStream` (app_id, rec_id, timepoint, etc...). Also inherit the application ID while we're at it. Makes the API much nicer to use _and_ much more consistent with the rest. Checks: - [x] external loader ran manually (`python loader | rerun`) - [x] external loader via rerun (`rerun xxx.py`) - [x] log_file with external loader (`log_file xxx.py`) - [x] external loader ran manually (`loader | rerun`) - [x] external loader via rerun (`rerun xxx.rs`) - [x] log_file with external loader (`log_file xxx.rs`) --- Part of series of PR to expose configurable `DataLoader`s to our SDKs: - #5327 - #5328 - #5330 - #5337 - #5351 - #5355 - #5379 - #5361 - #5388 --------- Co-authored-by: Andreas Reich <andreas@rerun.io>

Introduces the new `DataLoaderSettings` business to C++ and update examples accordingly (`external_data_loader` & `log_file`). ```bash ./build/debug/examples/cpp/log_file/example_log_file --recording-id this-one --entity-path-prefix a/b/c --time sim_time=1000 --time wall_time=1709204046 --sequence sim_frame=42 rerun_cpp/tests/main.cpp | rerun - ``` ![image](https://github.com/rerun-io/rerun/assets/2910679/b979a24c-29b6-473b-91b1-de3832bea436) Checks: - [x] external loader ran manually (`loader.exe | rerun`) - [x] external loader via rerun (`rerun xxx.cpp`) - [x] log_file with external loader (`log_file xxx.cpp`) --- Part of series of PR to expose configurable `DataLoader`s to our SDKs: - #5327 - #5328 - #5330 - #5337 - #5351 - #5355 - #5379 - #5361 - #5388

I guess that's good enough 🤷. I don't know, my brain has been completely friend by C++ non-sense all day. This includes a fix to make sure that a viewer that was spawned from the python SDK is still allowed to spawn dataloaders implemented in python (`RERUN_APP_ONLY` shenaniganeries). - Fixes #4526 --- Part of series of PR to expose configurable `DataLoader`s to our SDKs: - #5327 - #5328 - #5330 - #5337 - #5351 - #5355 - #5379 - #5361 - #5388

teh-cmc added 🦀 Rust API Rust logging API do-not-merge Do not merge this PR include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages labels Feb 29, 2024

teh-cmc changed the base branch from main to cmc/sdk_dataloader_3_weak_recordings February 29, 2024 11:53

teh-cmc changed the title ~~Cmc/sdk dataloader 5 rust settings~~ SDK DataLoaders 5: customizable (external) loaders for Rust Feb 29, 2024

teh-cmc marked this pull request as draft February 29, 2024 11:59

teh-cmc force-pushed the cmc/sdk_dataloader_3_weak_recordings branch from 1489296 to 33c9161 Compare February 29, 2024 12:04

teh-cmc force-pushed the cmc/sdk_dataloader_5_rust_settings branch from 6aac240 to d61342b Compare February 29, 2024 12:05

teh-cmc marked this pull request as ready for review February 29, 2024 12:28

Wumpf self-requested a review February 29, 2024 14:19

This was referenced Feb 29, 2024

SDK DataLoaders 6: customizable (external) loaders for Python #5355

Merged

SDK DataLoaders 2: barebones Python support #5328

Merged

Wumpf approved these changes Feb 29, 2024

View reviewed changes

This was referenced Feb 29, 2024

SDK DataLoaders 3: barebones C and C++ support #5330

Merged

SDK DataLoaders 4: working around shutdown brittleness #5337

Merged

Base automatically changed from cmc/sdk_dataloader_3_weak_recordings to main February 29, 2024 15:36

teh-cmc added 7 commits February 29, 2024 16:38

introduce dataloading settings

738728d

update external_data_loader example

b599fbe

expose load settings to new log_file APIs

ff0bc94

update log_file example with some load settings

da4f28d

clean up

07b0355

fixing teh internet

5a5eabb

bruh

c220ab0

teh-cmc force-pushed the cmc/sdk_dataloader_5_rust_settings branch from e50f9d3 to c220ab0 Compare February 29, 2024 15:38

teh-cmc added 2 commits February 29, 2024 16:45

fix requirements

105dc4c

review

a27b321

teh-cmc added 2 commits February 29, 2024 16:57

temporary C shenanigans while we wait for the APIs to land

d774ea7

temporary python shenanigans while we wait for the APIs to land

2a6a432

teh-cmc removed the do-not-merge Do not merge this PR label Feb 29, 2024

teh-cmc added 3 commits February 29, 2024 17:12

trying to fix cpp

b71f368

fmt

b6cad12

missed one

ba9a2d2

teh-cmc merged commit 9e3ffc2 into main Feb 29, 2024
28 of 31 checks passed

teh-cmc deleted the cmc/sdk_dataloader_5_rust_settings branch February 29, 2024 16:33

teh-cmc mentioned this pull request Feb 29, 2024

SDK DataLoaders 8: customizable (external) loaders for C++ #5361

Merged

8 tasks

This was referenced Mar 4, 2024

SDK DataLoaders 7: stateful file logging #5379

Merged

SDK DataLoaders 9: polish, docs, etc #5388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDK `DataLoader`s 5: customizable (external) loaders for Rust #5351

SDK `DataLoader`s 5: customizable (external) loaders for Rust #5351

teh-cmc commented Feb 29, 2024 •

edited

Loading

Wumpf left a comment

Wumpf Feb 29, 2024

teh-cmc Feb 29, 2024

Wumpf Feb 29, 2024

teh-cmc Feb 29, 2024

Wumpf Feb 29, 2024

Wumpf Feb 29, 2024

teh-cmc Feb 29, 2024

Wumpf Feb 29, 2024

SDK DataLoaders 5: customizable (external) loaders for Rust #5351

SDK DataLoaders 5: customizable (external) loaders for Rust #5351

Conversation

teh-cmc commented Feb 29, 2024 • edited Loading

Checklist

Wumpf left a 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

SDK `DataLoader`s 5: customizable (external) loaders for Rust #5351

SDK `DataLoader`s 5: customizable (external) loaders for Rust #5351

teh-cmc commented Feb 29, 2024 •

edited

Loading